Bug 89993 – [CSS Exclusions] Enable css exclusions for multiple blocks per element

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bear Travis]

Exclusions will initially only support a single render block per element, sharing the same coordinates
Multiple render blocks per element potentially require looking up the parent chain to find the shape inside, and adjusting from the parent to local coordinates when calculating line segments

[Bear Travis]

Created attachment 161344 [details]
Child blocks respect parent's shape-inside

[Bear Travis]

Created attachment 162105 [details]
LayoutState Approach

New code approach adding WrapShapeInfo to LayoutState, rather than always using the parent chain to look up WrapShapeInfo.

[Build Bot]

Comment on attachment 162105 [details]
LayoutState Approach

Attachment 162105 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13734951

[Bear Travis]

Created attachment 162130 [details]
Fixing windows build error

Surrounding a exclusions-specific test with compile flags

[Bear Travis]

Created attachment 162279 [details]
Removing duplicate changelog entry

[Julien Chaffraix]

Comment on attachment 162279 [details]
Removing duplicate changelog entry

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

AFAICT it's OK to hop onto LayoutState in this case (exclusions behaves like multi-column from that perspective). Dave or Simon may have more perspective about that though.

Note that it's not strictly required but it avoids having to walk up the tree to find your enclosing wrapShapeInfo.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1301
> +        logicalTopOffset = logicalTop() + view()->layoutState()->layoutOffset().height();

If layout state is disabled, I don't think this is correct. We may be fine with that for now, but it's definitely worth a FIXME.

> Source/WebCore/rendering/RenderView.h:229
> +            || (renderer->isRenderBlock() && toRenderBlock(renderer)->wrapShapeInfo())

We may want to add a RenderObject::hasWrapShapeInfo()?

[Bear Travis]

Created attachment 162833 [details]
Incorporating feedback

Incorporating Julien's feedback.
Mentioning multi-column layout in changelog
Adding FIXME to the layout offset
Not yet adding hasWrapShape to RenderObject, but if we have more similar tests, I will.

[Dave Hyatt]

Comment on attachment 162833 [details]
Incorporating feedback

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

In general I have concerns here about propagating everything through LayoutState. I'm not sure of this approach. It could be I have a misunderstanding of WrapShapeInfo though. Is this struct used both for being inside and outside, i.e., is it used both to fit lines inside a shape as well as to help lines avoid intruding into a shape from the outside? If it's the latter, I am concerned about mindlessly propagating every last bit of information from ancestors down to children. It seems like you'd run into performance problems if you haven't limited the scope somehow.

Mostly though I'm concerned about writing modes, since connecting to LayoutState is going to get you into trouble.

Another question I have is it looks like an inner WrapShapeInfo will override an outer WrapShapeInfo? Does only one truly apply? Can multiple ones not apply? Do you not have to go up the LayoutState stack to examine all of them?

> Source/WebCore/rendering/LayoutState.cpp:46
> +    , m_wrapShapeInfo(0)

I'd still like to see a rename at some point from wrapShape -> exclusionShape. Especially once the ifdefs eventually fall away, people aren't going to link "wrap' with the exclusions feature.

> Source/WebCore/rendering/LayoutState.h:98
> +    WrapShapeInfo* wrapShapeInfo() { return m_wrapShapeInfo; }

This method should be const.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:83
> +        WrapShapeInfo* wrapShapeInfo = m_block->view()->layoutState()->wrapShapeInfo();

Rather than removing the RenderBlock method, I think you'd be better off having it still exist and return view()->layoutState()->wrapShapeInfo().

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:807
> +    WrapShapeInfo* wrapShapeInfo = view()->layoutState()->wrapShapeInfo();

Ditto here.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1298
> +    LayoutUnit logicalTopOffset;

This is not a good name, since it sounds like you're in local coordinates. With the layout offset applied, though, this is really absolute coordinates.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1302
> +        logicalTopOffset = logicalTop() + view()->layoutState()->layoutOffset().height();

You've got a big writing modes problem here. We don't use LayoutState for flipped blocks writing modes (the LayoutState gets disabled), since right now m_layoutOffset isn't accounting for flipping.

Connecting propagation to m_layoutOffset creates a big problem, since LayoutState basically doesn't work with flipped blocks writing modes.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1305
> +    if (wrapShapeInfo && logicalHeight() + logicalTopOffset < wrapShapeInfo->shapeTop())
> +        setLogicalHeight(wrapShapeInfo->shapeTop() - logicalTopOffset);

Everything seems completely muddled here with writing modes. You can't just add the layout offset's height to a logical top. The layout offset is physical and logicalTop is not.

I still don't know what shapeTop means. Is it logical? Is it physical? I don't know.

> Source/WebCore/rendering/RenderView.h:227
> +            || (renderer->isRenderBlock() && toRenderBlock(renderer)->wrapShapeInfo())

It's a little weird to me that once a parent has wrap shape info, that you then will push that along down to every descendant. It seems like your filtering needs to be smarter than that. Aren't you going to want to limit this scope?

[Bear Travis]

Created attachment 162919 [details]
Incorporating feedback

Fixing up patch
WrapShapeInfo only deals with shape-inside, which is why one RenderBlock's shape-inside affects all descendants, and a new shape-inside overrides the previous one.
I have added bug 96160 to rename WrapShapeInfo to something more useful, ExclusionShapeInfo or ShapeInsideInfo.
I have also logged bug 96161 to refactor WrapShapeInfo to be more straightforward when dealing with logical vs physical coordinates. WrapShapes should be sized physically (based on the width/height of their block), but passed logical coordinates when computing line segments. Other fixes:
LayoutState::wrapShapeInfo was made const
Factored out block->view()->layoutState()->wrapShapeInfo() to a layoutWrapShapeInfo() function. RenderBlock::wrapShapeInfo() is still necessary to tell whether or not a given block has a shape-inside from CSS
Renamed logicalTopOffset to absoluteLogicalTop
absoluteLogicalTop will now use layoutOffset width or height depending on writing mode
shapeTop will be renamed to logicalTop as part of bug 96161

[Bear Travis]

Created attachment 163241 [details]
Updating patch

Merging with recent changes.

[Dave Hyatt]

Comment on attachment 163241 [details]
Updating patch

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

Just some minor suggestions at this point. Make sure to follow up on the rename of wrapShapeInfo, if it really is only about "inside".

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1313
> +        absoluteLogicalTop = logicalTop();
> +        // FIXME: If layout state is disabled, the offset will be incorrect
> +        if (isHorizontalWritingMode())
> +            absoluteLogicalTop += view()->layoutState()->layoutOffset().height();
> +        else
> +            absoluteLogicalTop += view()->layoutState()->layoutOffset().width();

Minor nitpick, but I think this would read better if you cache in a local. Consider changing this block of code to:

LayoutSize layoutOffset = view()->layoutState()->layoutOffset();
absoluteLogicalTop = logicalTop() + (isHorizontalWritingMode() ? layoutOffset.height() : layoutOffset.width());

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1317
> +    if (wrapShapeInfo && logicalHeight() + absoluteLogicalTop < wrapShapeInfo->shapeLogicalTop())
> +        setLogicalHeight(wrapShapeInfo->shapeLogicalTop() - absoluteLogicalTop);

Can't this go inside the previous if statement that assigned to wrapShapeInfo? Then it would be inside that if and just turn into:

if (logicalHeight() + absoluteLogicalTop < wrapShapeInfo->shapeLogicalTop())

since you already know wrapShapeInfo isn't null.

[Bear Travis]

Created attachment 163984 [details]
Incorporating feedback

Fixing up based on Dave Hyatt's comments.

[Julien Chaffraix]

Comment on attachment 163984 [details]
Incorporating feedback

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

Could you add some testing with vertical writing modes? The previous patch was wrong because it confused physical coordinates and logical ones which would have been caught by such a test.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1307
> +    WrapShapeInfo* wrapShapeInfo = layoutWrapShapeInfo(this);
> +    if (wrapShapeInfo) {

Nit: maybe it would be better just: if (WrapShapeInfo* wrapShapeInfo = layoutWrapShapeInfo(this)) {

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1313
> +        // Move to the top of the shape inside to begin layout

This comment is not super clear IMO. How about:

// Layout start at the logical top of our shape inside.

(feel free to adapt the prose)

[Bear Travis]

Created attachment 165463 [details]
Incorporating feedback

Addressing Julien's feedback.
Leaving wrapShapeInfo in place, as it is subsequently used in the while loop.
Modifying comment in RenderBlockLineLayout to be more clear.
Adding a vertical writing mode test.

[Julien Chaffraix]

Comment on attachment 165463 [details]
Incorporating feedback

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

> Source/WebCore/rendering/LayoutState.cpp:116
> +        RenderBlock* renderBlock = toRenderBlock(renderer);
> +        m_wrapShapeInfo = renderBlock->wrapShapeInfo();

Unless you are sure you need |renderBlock| in a follow-up patch, I would write it on one line (as it is not used outside this scope):

m_wrapShapeInfo = toRenderBlock(renderer)->wrapShapeInfo();

> LayoutTests/fast/exclusions/shape-inside/shape-inside-multiple-blocks.html:12
> +        -webkit-shape-inside: rectangle(10px, 10px, 180px, 280px);

It's usually better to select different values for each direction (as you did for the vertical writing mode test). That makes it easier to spot the wrong size being used.

[Bear Travis]

Created attachment 165507 [details]
Updated patch

Incorporating Julien's feedback.
Removed the RenderBlock variable.
Changed the shape-inside to be asymmetrical.

[WebKit Review Bot]

Comment on attachment 165507 [details]
Updated patch

Clearing flags on attachment: 165507

Committed r129530: <http://trac.webkit.org/changeset/129530>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.