Bug 174247 – Containing block overrides not cleared for absolutely positioned items

• Component: Layout and Rendering
• Status: NEW

[Sergio Villar Senin]

Containing block overrides not cleared for absolutely positioned items

[Sergio Villar Senin]

Created attachment 314825 [details]
Patch

[Build Bot]

Comment on attachment 314825 [details]
Patch

Attachment 314825 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4069373

New failing tests:
fast/workers/dedicated-worker-lifecycle.html

[Build Bot]

Created attachment 314828 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sergio Villar Senin]

(In reply to Build Bot from comment #3)
> Created attachment 314828 [details]
> Archive of layout-test-results from ews115 for mac-elcapitan
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

Seems totally unrelated

[Sergio Villar Senin]

Ping reviewers

[Manuel Rego Casasnovas]

Comment on attachment 314825 [details]
Patch

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

r=me

Some nitpicky comments about the tests. Forgive me about that. O:-)

> Source/WebCore/ChangeLog:16
> +        In particular this affects grid items which always get a containing block override size
> +        (which represent the grid areas) in case their containing block switches from the grid
> +        container to a grid ancestor. It also happens when a grid item with relative size
> +        (i.e. percentage) becomes an absolutely positioned block as it needs to be sized against the
> +        grid container not the grid area.

The tests are only for the case of grid items with percentage sizes.
I'm not sure I understand properly the 2 cases explained here
or if we're missing more tests.

> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block-expected.html:24
> +<p>This test checks that absolutelly positioned children of a grid are properly sized when the containing block switches between the grid container and a grid ancestor.</p>

Typo: absolutelly

> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block-expected.html:25
> +<p>The test PASS if you see an orange box inside a purple box on top and a small orange box inside a light blue box inside a purple box on bottom.</p>

Wow, the pass condition is pretty complex :-)

> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block.html:14
> +     grid-template: 10px / 10px;

Nit: You don't need this line.

> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block.html:33
> +	<div class="item"></div>

Nit: Wrong indentation.

> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block.html:38
> +	<div class="item"></div>

Nit: Ditto.

> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block.html:51
> + setTimeout(test, 0);

Do we need the timeout and the testRunner?

> LayoutTests/fast/css-grid-layout/grid-item-absolute-positioning-dynamic.html:12
> +     grid: 100px / 100px;

We don't need this, as the item is not placed in just 1 cell.
For abspos items "auto" lines are resolved to the padding edges,
so this has no effect.

> LayoutTests/fast/css-grid-layout/grid-item-absolute-positioning-dynamic.html:22
> +<p>This test checks that a grid item which becomes an absolutelly positioned children of a grid.</p>

Typo: "absolutelly"
Nit: The sentence is missing something at the end.

> LayoutTests/fast/css-grid-layout/grid-item-absolute-positioning-dynamic.html:27
> +      <div id="item"></div>

Nit: Wrong indentation.

> LayoutTests/fast/css-grid-layout/grid-item-absolute-positioning-dynamic.html:39
> + setTimeout(test, 0);

Again, do we need this?

[Sergio Villar Senin]

Comment on attachment 314825 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +        grid container not the grid area.
> 
> The tests are only for the case of grid items with percentage sizes.
> I'm not sure I understand properly the 2 cases explained here
> or if we're missing more tests.

So the 2 cases are:
1- grid child with grid container as a containing block. Then an ancestor of the grid container becomes the grid child new containing block
1- grid item becomes an absolutely positioned item with grid container as containing block

>> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block.html:14
>> +     grid-template: 10px / 10px;
> 
> Nit: You don't need this line.

Why not? I want to specify a grid area different to the other sizes in order to visually identify the results.

>> LayoutTests/fast/css-grid-layout/absolute-positioning-changing-containing-block.html:51
>> + setTimeout(test, 0);
> 
> Do we need the timeout and the testRunner?

Yes because otherwise the rendering won't be the final one.

>> LayoutTests/fast/css-grid-layout/grid-item-absolute-positioning-dynamic.html:12
>> +     grid: 100px / 100px;
> 
> We don't need this, as the item is not placed in just 1 cell.
> For abspos items "auto" lines are resolved to the padding edges,
> so this has no effect.

OK

>> LayoutTests/fast/css-grid-layout/grid-item-absolute-positioning-dynamic.html:22
>> +<p>This test checks that a grid item which becomes an absolutelly positioned children of a grid.</p>
> 
> Typo: "absolutelly"
> Nit: The sentence is missing something at the end.

ACK.

>> LayoutTests/fast/css-grid-layout/grid-item-absolute-positioning-dynamic.html:39
>> + setTimeout(test, 0);
> 
> Again, do we need this?

Again just to let the final rendering take place

[zalan]

Comment on attachment 314825 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Whenever a position:absolute block gets a new containing block the previously set containing
> +        block overrides are not cleared. This causes the block not to be properly layout for its new
> +        containing block (for example when using relative sizes).

This sounds very grid specific problem. I'd be surprised if this statement was correct in the generic block formatting context. Shouldn't this code live somewhere in the grid layout classes?

[Simon Fraser (smfr)]

Comment on attachment 314825 [details]
Patch

r- until Zalan's concern is addressed.

[Sergio Villar Senin]

(In reply to zalan from comment #8)
> Comment on attachment 314825 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314825&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Whenever a position:absolute block gets a new containing block the previously set containing
> > +        block overrides are not cleared. This causes the block not to be properly layout for its new
> > +        containing block (for example when using relative sizes).
> 
> This sounds very grid specific problem. I'd be surprised if this statement
> was correct in the generic block formatting context. Shouldn't this code
> live somewhere in the grid layout classes?

It's a grid specific problem in the sense that only grid is using the override containing block mechanism. But this mechanism is general purpose and implemented in RenderBox so I don't see any reason why we should not make the solution general. Any future user of the containing block overrides will face the same problem, you need to clear the overrides otherwise the block will never be properly laid out against its new containing block.