[webkit-reviews] review granted: [Bug 174247] Containing block overrides not cleared for absolutely positioned items : [Attachment 314825] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 174247: Containing block overrides not cleared for absolutely positioned
items
https://bugs.webkit.org/show_bug.cgi?id=174247

Attachment 314825: Patch

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




--- Comment #6 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 314825
  --> https://bugs.webkit.org/attachment.cgi?id=314825
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?