Bug 112929 – [CSS Exclusions] Increasing padding does not correctly layout child blocks

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

[Bear Travis]

Created attachment 194289 [details]
Test case

See the attached test file. Increasing the vertical offset does not affect subsequent blocks.

[Zoltan Horvath]

Created attachment 196499 [details]
proposed patch

[Dave Hyatt]

Comment on attachment 196499 [details]
proposed patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1423
> +        // If there is an animation on the block, we need to relayout the shape to apply the shape on the animated content
> +        if (block->animation())
> +            info->setNeedsLayout(true);

Should this be more specific? Just wondering if you want to narrow it to certain properties animating? In general we haven't had to special case animation, so I'm trying to understand why this would be needed just for animation.

[Zoltan Horvath]

(In reply to comment #2)
> (From update of attachment 196499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196499&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:1423
> > +        // If there is an animation on the block, we need to relayout the shape to apply the shape on the animated content
> > +        if (block->animation())
> > +            info->setNeedsLayout(true);
> 
> Should this be more specific? Just wondering if you want to narrow it to certain properties animating? In general we haven't had to special case animation, so I'm trying to understand why this would be needed just for animation.

The bug appeared with transitions. I found that requesting relayout only for animation is the smallest subset of cases. By requesting the relayout we force RenderBlock::llayoutRunsAndFloatsInRange to use an updated segments set for the lines. 

Is this an answer to you question?

[Dave Hyatt]

Comment on attachment 196499 [details]
proposed patch

All the animation code does is repeatedly change padding-top, so it seems like you're just working around a bug here. If you re-write the test case to manually set padding-top over and over using setTimeout instead of using an animation, won't it just still fail? I don't see why animation has anything to do with the bug. It's just how you happened to build the test case.

[Bear Travis]

The bug is not animation specific, but has to do with children not laying out when a previous sibling has changed dimensions. I will upload a simplified testcase shortly.

[Bear Travis]

Created attachment 196521 [details]
Simplified testcase using setTimeout

[Bear Travis]

Created attachment 196523 [details]
Immediate testcase (without setTimeout)

Based off of shape-inside-dynamic-nested.html testcase.

[Zoltan Horvath]

Created attachment 197179 [details]
proposed patch

Patch is updated.

I wanted to do a little refactoring and use layoutExclusionShapeInsideInfo(...) from RenderBlockLineLayout instead of calling view()->layoutState()->exclusionShapeInsideInfo(), but it and logicalHeightForLine are used by other static functions as well, so the refactoring is out of scope for this bug.

[Zoltan Horvath]

Created attachment 197307 [details]
proposed patch

Please ignore my last comment, I can just use layoutExclusionShapeInsideInfo in updateRegionsAndExclusionsAfterChildLayout, since it's not a static anymore.

[Dave Hyatt]

Comment on attachment 197307 [details]
proposed patch

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

Looks good, just fix the header file.

> Source/WebCore/rendering/RenderBlock.h:582
> +    void updateRegionsAndExclusionsAfterChildLayout(RenderFlowThread*, bool = false);

You need to include the parameter name here, since it's not clear what the boolean means without it.

[Zoltan Horvath]

Created attachment 197317 [details]
proposed patch, header fixed

> > Source/WebCore/rendering/RenderBlock.h:582
> > +    void updateRegionsAndExclusionsAfterChildLayout(RenderFlowThread*, bool = false);
> 
> You need to include the parameter name here, since it's not clear what the boolean means without it.

Fixed.

[Dave Hyatt]

Comment on attachment 197317 [details]
proposed patch, header fixed

r=me

[WebKit Commit Bot]

Comment on attachment 197317 [details]
proposed patch, header fixed

Clearing flags on attachment: 197317

Committed r148120: <http://trac.webkit.org/changeset/148120>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.