Conversation
itlackey
force-pushed
the
feat/shape-outside
branch
from
8a6df91 to
67cdea4
Compare
|
Pushed a branch with a new book example I used for testing: https://github.com/itlackey/weasyprint-samples/tree/dev/shape-outside |
There was a problem hiding this comment.
Thanks for the PR, it’s pretty cool!
I like the choice to avoid path() (and shape()?). Your comment and the documentaton include shape-image-threshold and url(), but they’re supported, right? I think that SVGs are also unsupported, and that’s a good idea if we don’t want to actually spend weeks on that. 😄 That’s a good way to cover most of the common use cases fast.
Many comments are just small details.
The main problem for me is the boundary detection logic. I think that it would be better to let the box handle this complexity (and have not much in float), and that the box API should give the max boundaries for a range instead of a single y value. Do you agree with that?
For the tests, it may be interesting to dispatch them into different files, and to add drawing tests. If you don’t want to spend more time on them, tell me, I’ll do that later.
I’ll review shapes.py and the parsing details later.
|
|
||
| - ``path()`` function | ||
|
|
||
| Example usage: |
There was a problem hiding this comment.
We don’t have many examples in this page, we prefer to keep links to specifications, you can remove this part.
| ========= | ||
|
|
||
|
|
||
| Unreleased |
There was a problem hiding this comment.
We write the Changelog when the release is ready, you can remove this change.
| /docs/_build | ||
| /pytest_cache | ||
| /tests/draw/results | ||
| /venv |
| @@ -0,0 +1,2854 @@ | |||
| """Tests for CSS shape-outside property with box keyword values.""" | |||
There was a problem hiding this comment.
It would be better to keep here tests that are actually related to the layout, ie that test the position of the boxes. These tests should ideally not rely on WeasyPrint internals (*Boundary classes for example).
You can test boundary classes in a separate file, not in layout.
CSS validation tests can go to css/test_validation.py where the assert_invalid and get_value functions will be useful.
It would also be useful to test real rendering in draw.
| # At center y=100, bounds should be at x=50 to x=150 | ||
| bounds = boundary.get_bounds_at_y(100) | ||
| assert bounds is not None | ||
| assert abs(bounds[0] - 50) < 0.001 |
| if len(lines) >= 2: | ||
| # First lines at top of float may have more indent due to curve | ||
| # Lines in middle should have less indent (or end where float ends) | ||
| pass # Test structure verified; exact positioning depends on font metrics |
There was a problem hiding this comment.
It’s possible to use the WeasyPrint font to test text.
| 'right': 'auto', | ||
| 'shape_outside': 'none', | ||
| 'shape_margin': ZERO_PIXELS, | ||
| 'shape_image_threshold': 0.0, |
| elif shape_outside == 'content-box': | ||
| return box.content_box_x(), box.width | ||
| else: | ||
| # Fallback to margin box for any unknown values |
There was a problem hiding this comment.
Any other value is an error, we should remove the first if and assert shape_outside in ('none', 'margin-box').
| # the exclusion area around floated elements. | ||
| left_bounds = [] | ||
| right_bounds = [] | ||
| for shape in colliding_shapes: |
There was a problem hiding this comment.
I think that we should use another logic here.
Instead of testing top/middle/bottom, we could have a function that gives the max left/right bound of the shape between top and bottom y. This function could be available in all boxes (in Box) and create the shape boundary on demand, to avoid the hasattr logic here.
What do you think of that?
|
Just checking in. I found a memory/performance regression when working on large documents with high resolution images. I hope to get back to debugging this weekend. *I will also address the comments/issues highlighted above |
2 participants
Add CSS Shapes Level 1 documentation to API reference
Documents the newly implemented shape-outside feature including:
Enhance shape boundary handling for text wrapping and border-radius support
test_inset_with_margin_text_wrapto clarify shape boundary usage.avoid_collisionsto sample bounds for curved shapes at multiple Y positions._create_base_boundaryto createInsetBoundaryfor elements with border-radius._get_border_radiifunction to extract border-radius values for shape calculations.Tests: 117 tests