Bug 113546 – [CSS Grid Layout] Align our grid-line handling with the updated specification

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently:

-webkit-grid-row: 1 / 1;

means that the grid item will span over all the rows. How -webkit-grid-end and -webkit-grid-after is resolved changed in the specification so this syntax now means that the element is in the first row (we compute -webkit-grid-end against the start edge and not the end edge anymore)

To be able to express the same intent, we need to be able to parse negative grid lines as the same code is now written:

-webkit-grid-row: 1 / -1;

[Julien Chaffraix]

Created attachment 195685 [details]
Proposed fix 1: Update our handling, our tests. Doesn't handle the start/before case as it could grow the grid.

[Tony Chang]

Comment on attachment 195685 [details]
Proposed fix 1: Update our handling, our tests. Doesn't handle the start/before case as it could grow the grid.

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

> Source/WebCore/rendering/RenderGrid.cpp:338
> +    // Negative explicit values are considered positive for the purpose of estimating our size.
> +    // See http://lists.w3.org/Archives/Public/www-style/2013Mar/0593.html

I don't think that's what the email is saying.  I think it's saying if there are 3 grid columns and the position is -1, translate the position to 3, not 1.

[Julien Chaffraix]

Comment on attachment 195685 [details]
Proposed fix 1: Update our handling, our tests. Doesn't handle the start/before case as it could grow the grid.

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

>> Source/WebCore/rendering/RenderGrid.cpp:338
>> +    // See http://lists.w3.org/Archives/Public/www-style/2013Mar/0593.html
> 
> I don't think that's what the email is saying.  I think it's saying if there are 3 grid columns and the position is -1, translate the position to 3, not 1.

Confirmed with Tab that you are totally right, I misread the answer. The specification was updated yesterday so let me loop back on it for this patch.

[Julien Chaffraix]

Created attachment 196033 [details]
Proposed change 2: Updated to better match Tab's answers.

[Ojan Vafai]

Comment on attachment 196033 [details]
Proposed change 2: Updated to better match Tab's answers.

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

> Source/WebCore/ChangeLog:10
> +        to always resolve against the start/before edge (the previous code would resolve end/after
> +        grid lines resolve against the end/after edge).

I think you have one too many resolves here.

> Source/WebCore/rendering/RenderGrid.cpp:776
> +        // FIXME: This returns the wrong result for side == StartSide or BeforeSide (off-by-one) but
> +        // that avoids the issue of growing the grid due to e.g. -1 / auto.

I don't really understand this comment. Can you explain it a bit more? Here or in the ChangeLog is fine.

> Source/WebCore/rendering/RenderGrid.cpp:778
> +        // Per http://lists.w3.org/Archives/Public/www-style/2013Mar/0589.html, we should

Typically, we put links to email threads or specs in the ChangeLog. It'll be there in the blame history should someone try to understand why these lines were added. I don't think this line really requires an explanatory comment in addition to the explanation in the ChangeLog.

In the limit, nearly every line of code in this file could point to a bit of the specification, which would obviously be unwieldy.

[Julien Chaffraix]

Comment on attachment 196033 [details]
Proposed change 2: Updated to better match Tab's answers.

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

>> Source/WebCore/rendering/RenderGrid.cpp:776
>> +        // that avoids the issue of growing the grid due to e.g. -1 / auto.
> 
> I don't really understand this comment. Can you explain it a bit more? Here or in the ChangeLog is fine.

Sure, this function converts a grid line number into a grid column or row (ie a grid track which is represented as an index into grid representation).

Let me take the example of -1 to explain the issue: -1 refers to the last grid line, the line that is *after* the last grid column / row. For grid-{end|after}, it is fine as it means we stop at this line, but for grid-{start|before}, it means we start after the last grid line. What this means is that -1 for grid-{start|before} can end up growing the grid, which we currently don't support.

Now, you can apply the previous reasoning to other negative value and will find that our result is actually one less than the expected result (-2 should be the last grid column / row but is the penultimate, etc...).

> Source/WebCore/rendering/RenderGrid.cpp:777
> +        const size_t endOfTrack = (side == StartSide || side == EndSide) ? gridColumnCount() : gridRowCount();

I realized that this is not right: we need to count from the explicit grid's size which doesn't match this line.

>> Source/WebCore/rendering/RenderGrid.cpp:778
>> +        // Per http://lists.w3.org/Archives/Public/www-style/2013Mar/0589.html, we should
> 
> Typically, we put links to email threads or specs in the ChangeLog. It'll be there in the blame history should someone try to understand why these lines were added. I don't think this line really requires an explanatory comment in addition to the explanation in the ChangeLog.
> 
> In the limit, nearly every line of code in this file could point to a bit of the specification, which would obviously be unwieldy.

Sounds good, this hasn't made it into the specification yet but it will probably at some point.

[Julien Chaffraix]

Created attachment 196193 [details]
Updated change: Same as previously but properly resolve against the explicit grid.

[Ojan Vafai]

Comment on attachment 196193 [details]
Updated change: Same as previously but properly resolve against the explicit grid.

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

Would like to understand why we're adding the member variables before r+.

> Source/WebCore/rendering/RenderGrid.cpp:348
> +void RenderGrid::computeExplicitGridSizes()
>  {
> -    const Vector<GridTrackSize>& trackStyles = (direction == ForColumns) ? style()->gridColumns() : style()->gridRows();
> +    m_explicitGridRowCount = style()->gridRows().size();
> +    m_explicitGridColumnCount = style()->gridColumns().size();
> +}

Why do we need this? Why not just use style()->gridRows().size() and style()->gridColumns().size() directly?

> Source/WebCore/rendering/RenderGrid.cpp:783
> +        // FIXME: This returns one less than the expected result for side == StartSide or BeforeSide as we don't properly convert
> +        // the grid line to its grid track. However this avoids the issue of growing the grid when inserting the item (e.g. -1 / auto).

I now understand this...I think the spec is dumb. I've nagged Tab and fantasai about this on IRC. In either case, this is fine for now.

[Julien Chaffraix]

Comment on attachment 196193 [details]
Updated change: Same as previously but properly resolve against the explicit grid.

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

>> Source/WebCore/rendering/RenderGrid.cpp:348
>> +}
> 
> Why do we need this? Why not just use style()->gridRows().size() and style()->gridColumns().size() directly?

Currently it is actually unneeded.

In the longer term, we will need to aggregate the result of 2 style properties: grid-definition-{rows|columns} and grid-template. Thinking again, it's probably better to just add wrappers and wait until we need to cache this information (if at all).

[Kangil Han]

Created attachment 204757 [details]
Patch

[Kangil Han]

Seems this patch has been landed in blink so I would like to merge here.

[Build Bot]

Comment on attachment 204757 [details]
Patch

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

New failing tests:
webaudio/convolution-mono-mono.html

[Build Bot]

Created attachment 204759 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

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

[Kangil Han]

(In reply to comment #12)
> (From update of attachment 204757 [details])
> Attachment 204757 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/916056
> 
> New failing tests:
> webaudio/convolution-mono-mono.html

I don't think this patch affects to webaudio stuff.

[Sergio Villar Senin]

Any chance to get this reviewed? Looks like the test failure in EWS is totally unrelated.

[Sergio Villar Senin]

(In reply to comment #15)
> Any chance to get this reviewed? Looks like the test failure in EWS is totally unrelated.

If I am not wrong the patch just need to replace StartSide and EndSide by ColumnStartSide and ColumnEndSide which were recently renamed to fit latest spec changes.

[Sergio Villar Senin]

Comment on attachment 204757 [details]
Patch

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

Added a couple of comments to help rebasing the patch against the latest renames in the names of some properties.

> Source/WebCore/rendering/RenderGrid.cpp:784
> +        const size_t endOfTrack = (side == StartSide || side == EndSide) ? explicitGridColumnCount() : explicitGridRowCount();

These are now ColumnStartSide and ColumnEndSide

> LayoutTests/fast/css-grid-layout/grid-auto-flow-resolution.html:29
> +/* These 2 classes forces the grid to be sized after the grid-{end|after}, thus end up in the

Replace grid-{end|after} by the new names, i.e., grid-{column|row}-end

> LayoutTests/fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html:11
> +    /* -webkit-grid-rows is left unset so that the grid items' row is implicit. */

These are now -webkit-grid-definition-columns and -webkit-grid-definition-rows.

> LayoutTests/fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html:17
> +    /* -webkit-grid-columns is left unset so that the grid items' column is implicit. */

Ditto.

[Kangil Han]

@svillar:
FOA, thanks for having interest on this bug.
Unfortunately, I don't have plan to continue this work in nearest future.
Furthermore, seems you are working on grid layout these days.
Therefore, considering my personal reason and keep consistency on implementation, would you take this bug and continue work on patch? :)

[Sergio Villar Senin]

(In reply to comment #18)
> @svillar:
> FOA, thanks for having interest on this bug.
> Unfortunately, I don't have plan to continue this work in nearest future.
> Furthermore, seems you are working on grid layout these days.
> Therefore, considering my personal reason and keep consistency on implementation, would you take this bug and continue work on patch? :)

Sure, I can take over it

[Sergio Villar Senin]

Created attachment 208611 [details]
Patch

[Sergio Villar Senin]

Created attachment 208612 [details]
Patch

Now rebased against master

[Sergio Villar Senin]

Committed r154044: <http://trac.webkit.org/changeset/154044>