Bug 95479 – [CSS Exclusions] shape-inside line segment layout should be based on line position and height

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

Currently the CSS exclusions shape-inside support in WrapShapeInfo::computeSegmentsForLine(), WrapShapeInfo::lineState(), and the LineWidth constructor (RenderBlockLineLayout.cpp) only reflect a line's top edge.  A line only "fits" inside an exclusion shape if its entire bounding rectangle fits within the shape.

[Bear Travis]

Created attachment 164778 [details]
Initial patch

Adding line bottom to segment and line state calculations.

[Bear Travis]

*** Bug 97042 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 164778 [details]
Initial patch

Attachment 164778 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13903679

New failing tests:
fast/exclusions/shape-inside/shape-inside-rounded-rectangle.html

[Hans Muller]

(In reply to comment #1)
> Created an attachment (id=164778) [details]
> Initial patch
> 
> Adding line bottom to segment and line state calculations.

Source/WebCore/rendering/ExclusionRectangle.cpp:76
The patch for https://bugs.webkit.org/show_bug.cgi?id=96156 also includes the ExclusionRectangle change.  


Source/WebCore/rendering/RenderBlockLineLayout.cpp:85
    (wrapShapeInfo && wrapShapeInfo->lineState() & WrapShapeInfo::LINE_INSIDE_SHAPE && wrapShapeInfo->hasSegments())

An inline function like bool wrapShapeInfoHasSegments(WrapShapeInfo*) might be a useful simplification. Also: not clear why you're checking the lineState() is needed, isn't hasSegments() enough?


Source/WebCore/rendering/RenderBlockLineLayout.cpp:86,87
            // All interior shape positions should have at least one segment,
            // unless they extend beyond the edges of the shape.

Maybe "shape positions" isn't the right term here.  I think that what you mean is that if both the top and bottom edges of a line are within a shape, then there should be at least one segment.  However that's definitely not true for polygons, and it will not be true when ExclusionShape objects correspond to sets of primitive shapes.  I wouldn't characterize these exceptions as "they extend beyond the edges of the shape".  Maybe just letting the code speak for itself here would be enough.

Source/WebCore/rendering/RenderBlockLineLayout.cpp:809,810

        logicalLeft = max<float>(roundToInt(wrapShapeInfo->segments()[0].logicalLeft), logicalLeft);
        logicalRight = min<float>(floorToInt(wrapShapeInfo->segments()[0].logicalRight), logicalRight);

A pair of inlines whose names clarified what the min/max round/floor int/float means in these expressions, might improve readability.


Source/WebCore/rendering/WrapShapeInfo.h

Maybe, instead of making the enums and lineState() public, it would be better to just provide a set of inline functions that cover the tests needed elsewhere?

bool lineOverlapsShapeBoundsTop()
bool lineOverlapsShapeBoundsBottom()
bool lineIsWithinShapeBounds()
etc..

[Bear Travis]

Created attachment 164945 [details]
Updated patch

Incorporating Hans' feedback, and fixing the test failure.
Line positions within a shape are no longer guaranteed to have line segments. Removing the appropriate comments and tests.
Changing lineState tests to use boolean inlines, right now just lineOverlapsShapeBounds.
Removing lineState.
Going to leave the pixel snapping logicalLeft/Right code in place and file a bug for it (bug 97236).

[Bear Travis]

Created attachment 165476 [details]
Updating patch

Merging with master branch

[Levi Weintraub]

Comment on attachment 165476 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165476&action=review

> Source/WebCore/rendering/WrapShapeInfo.cpp:112
> +    if (lineOverlapsShapeBounds()) {
>          ASSERT(m_shape);
> -        m_shape->getIncludedIntervals(lineTop, lineTop, m_segments); // FIXME: Bug 95479, workaround for now
> +        m_shape->getIncludedIntervals(lineTop, std::min(lineBottom, shapeLogicalBottom()), m_segments);

So this will call getIncludedIntervals when the line intersects but is outside of the shape. Help me understand why?

[Bear Travis]

Linking to bug 96125, which is the bug for correctly laying out lines that overflow the shape-inside.
Overflow for shape-inside is under active specification. This patch allows the last line to overflow the shape-inside, but that behavior may need to change as the specification develops.

See
http://dev.w3.org/csswg/css3-exclusions/#shape-inside-property

[Bear Travis]

Created attachment 165660 [details]
Updating changelog

Making the changelog more descriptive.

[Levi Weintraub]

Comment on attachment 165660 [details]
Updating changelog

Apparently I'm a ChangeLog reviewer :) Please add a link to that spec on overflow to the ChangeLog as well so someone glancing can find this without reading the bug. Then I think we're good to go.

[Bear Travis]

Created attachment 165667 [details]
Adding links to changelog

[WebKit Review Bot]

Comment on attachment 165667 [details]
Adding links to changelog

Clearing flags on attachment: 165667

Committed r129590: <http://trac.webkit.org/changeset/129590>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.