Bug 132991

Summary: [CSS Grid Layout] Children of grid containers must be turned into grid items
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, buildbot, bunhere, cdumez, commit-queue, darin, dino, esprehn+autocc, glenn, gyuyoung.kim, jfernandez, kling, macpherson, menard, rego, rniwa, sergio
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
darin: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion none

Description Sergio Villar Senin 2014-05-16 08:45:09 PDT 
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
Comment 1 Sergio Villar Senin 2014-05-16 09:14:15 PDT 
Created attachment 231576 [details]
Patch
Comment 2 Darin Adler 2014-05-16 15:13:06 PDT 
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.
Comment 3 Darin Adler 2014-05-16 15:13:27 PDT 
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.
Comment 4 Sergio Villar Senin 2014-05-17 02:53:25 PDT 
(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.
Comment 5 Darin Adler 2014-05-18 21:01:57 PDT 
Please retitle this bug!
Comment 6 Sergio Villar Senin 2014-05-19 11:35:59 PDT 
(In reply to comment #5)
> Please retitle this bug!

What do you think about "Children of grid containers must be turned into grid items" ?
Comment 7 Darin Adler 2014-05-19 11:58:29 PDT 
Sounds fine.
Comment 8 Sergio Villar Senin 2014-05-20 07:51:28 PDT 
Created attachment 231767 [details]
Patch
Comment 9 Build Bot 2014-05-20 09:21:05 PDT 
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
Comment 10 Build Bot 2014-05-20 09:21:11 PDT 
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
Comment 11 Sergio Villar Senin 2014-05-20 11:16:52 PDT 
(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.
Comment 12 Darin Adler 2014-05-24 19:27:10 PDT 
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.
Comment 13 Sergio Villar Senin 2014-05-26 02:49:33 PDT 
Committed r169334: <http://trac.webkit.org/changeset/169334>