Bug 112929 - [CSS Exclusions] Increasing padding does not correctly layout child blocks
Summary: [CSS Exclusions] Increasing padding does not correctly layout child blocks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on:
Blocks: 89256
  Show dependency treegraph
 
Reported: 2013-03-21 10:26 PDT by Bear Travis
Modified: 2013-04-10 11:52 PDT (History)
6 users (show)

See Also:


Attachments
Test case (3.78 KB, text/html)
2013-03-21 10:26 PDT, Bear Travis
no flags Details
proposed patch (7.73 KB, patch)
2013-04-04 11:48 PDT, Zoltan Horvath
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Simplified testcase using setTimeout (2.11 KB, text/html)
2013-04-04 13:53 PDT, Bear Travis
no flags Details
Immediate testcase (without setTimeout) (2.30 KB, text/html)
2013-04-04 13:59 PDT, Bear Travis
no flags Details
proposed patch (8.39 KB, patch)
2013-04-09 14:54 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (8.38 KB, patch)
2013-04-10 09:34 PDT, Zoltan Horvath
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
proposed patch, header fixed (8.39 KB, patch)
2013-04-10 11:11 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bear Travis 2013-03-21 10:26:32 PDT
Created attachment 194289 [details]
Test case

See the attached test file. Increasing the vertical offset does not affect subsequent blocks.
Comment 1 Zoltan Horvath 2013-04-04 11:48:55 PDT
Created attachment 196499 [details]
proposed patch
Comment 2 Dave Hyatt 2013-04-04 13:03:23 PDT
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.
Comment 3 Zoltan Horvath 2013-04-04 13:22:56 PDT
(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?
Comment 4 Dave Hyatt 2013-04-04 13:38:20 PDT
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.
Comment 5 Bear Travis 2013-04-04 13:46:40 PDT
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.
Comment 6 Bear Travis 2013-04-04 13:53:33 PDT
Created attachment 196521 [details]
Simplified testcase using setTimeout
Comment 7 Bear Travis 2013-04-04 13:59:02 PDT
Created attachment 196523 [details]
Immediate testcase (without setTimeout)

Based off of shape-inside-dynamic-nested.html testcase.
Comment 8 Zoltan Horvath 2013-04-09 14:54:10 PDT
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.
Comment 9 Zoltan Horvath 2013-04-10 09:34:07 PDT
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.
Comment 10 Dave Hyatt 2013-04-10 11:08:02 PDT
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.
Comment 11 Zoltan Horvath 2013-04-10 11:11:38 PDT
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.
Comment 12 Dave Hyatt 2013-04-10 11:15:20 PDT
Comment on attachment 197317 [details]
proposed patch, header fixed

r=me
Comment 13 WebKit Commit Bot 2013-04-10 11:52:24 PDT
Comment on attachment 197317 [details]
proposed patch, header fixed

Clearing flags on attachment: 197317

Committed r148120: <http://trac.webkit.org/changeset/148120>
Comment 14 WebKit Commit Bot 2013-04-10 11:52:27 PDT
All reviewed patches have been landed.  Closing bug.