Bug 132991 – [CSS Grid Layout] Children of grid containers must be turned into grid items

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

[Sergio Villar Senin]

We should consider merging https://src.chromium.org/viewvc/blink?view=rev&revision=150472

    This change updates the grid items' 'display' after style resolution
    so that we match the specification's (see section 4. Grid Items).

    Changing 'display' also fixes a bug where we would insert an anonymous
    renderer between the grid element and a grid item. This would break
    the layout logic as we would consider the anonymous renderer during
    layout (thus missing any grid position set on the actual grid item).

    TEST=fast/css-grid-layout/grid-item-display.html

[Sergio Villar Senin]

Created attachment 231576 [details]
Patch

[Darin Adler]

Comment on attachment 231576 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [CSS Grid Layout] Fix grid items' computed 'display' values

This patch doesn’t just fix computed display values, does it? It looks like it actually changes behavior as well. adjustRenderStyle isn’t just used for display.

[Darin Adler]

Comment on attachment 231576 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [CSS Grid Layout] Fix grid items' computed 'display' values
> 
> This patch doesn’t just fix computed display values, does it? It looks like it actually changes behavior as well. adjustRenderStyle isn’t just used for display.

I meant to say adjustRenderStyle isn’t just used for computed values.

[Sergio Villar Senin]

(In reply to comment #3)
> (From update of attachment 231576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231576&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +        [CSS Grid Layout] Fix grid items' computed 'display' values
> > 
> > This patch doesn’t just fix computed display values, does it? It looks like it actually changes behavior as well. adjustRenderStyle isn’t just used for display.
> 
> I meant to say adjustRenderStyle isn’t just used for computed values.

Correct. I used the same title as the original bug report but I should have changed it. It fixes the wrong behavior indeed, that's why the new test case not only checks the value of "display" but also the positions where children are located.

Without this patch things like <span> or <img> are not properly positioned inside the grid.

[Darin Adler]

Please retitle this bug!

[Sergio Villar Senin]

(In reply to comment #5)
> Please retitle this bug!

What do you think about "Children of grid containers must be turned into grid items" ?

[Darin Adler]

Sounds fine.

[Sergio Villar Senin]

Created attachment 231767 [details]
Patch

[Build Bot]

Comment on attachment 231767 [details]
Patch

Attachment 231767 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4900872609857536

New failing tests:
fast/regions/cssom/region-range-for-box-crash.html

[Build Bot]

Created attachment 231771 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Sergio Villar Senin]

(In reply to comment #9)
> (From update of attachment 231767 [details])
> Attachment 231767 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/4900872609857536
> 
> New failing tests:
> fast/regions/cssom/region-range-for-box-crash.html

Seems totally unrelated.

[Darin Adler]

Comment on attachment 231767 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1112
> +static bool isDisplayGridBox(EDisplay display)

Should also mark this inline. While compilers are often smart enough to do that without being asked, I see no reason not to ask explicitly.

[Sergio Villar Senin]

Committed r169334: <http://trac.webkit.org/changeset/169334>