Bug 127033 – [CSS Grid Layout] getComputedStyle() is wrong for grid-definition-{columns|rows}

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

[Sergio Villar Senin]

The specs[1] mandate us to return the used value instead of the specified value which was the one we were using. This requires to force a layout to compute the used values.

This means that we'll have to update a lot of test results.

[1] http://dev.w3.org/csswg/css-grid/#resolved-track-list

[Sergio Villar Senin]

Created attachment 222577 [details]
Patch

[Sergio Villar Senin]

Created attachment 222580 [details]
Patch

With fix for the mac EWS

[Andreas Kling]

Comment on attachment 222580 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        According to the specs getComputedStyle() should return the used
> +        values instead of the resolved values for compatibility with early
> +        implementations. This means that grid-definition-{columns|rows}
> +        are now layout dependent as we need to compute the used values for
> +        grid track sizes.

I suppose this is consistent with other implementations of grid layout?

> Source/WebCore/rendering/RenderGrid.cpp:1020
> +    m_columnPositions.resize(sizingData.columnTracks.size() + 1);

If we don't plan to extend past this size, let's use resizeToFit() to avoid padding the vector for growth.

> Source/WebCore/rendering/RenderGrid.cpp:1025
> +    m_rowPositions.resize(sizingData.rowTracks.size() + 1);

Same here.

> Source/WebCore/rendering/RenderGrid.cpp:-1022
> -    // FIXME: |columnTrack| and |rowTrack| should be smaller than our column / row count.

Does this FIXME no longer apply? Should we take it with us to populateGridPositions()?

[Sergio Villar Senin]

Comment on attachment 222580 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        grid track sizes.
> 
> I suppose this is consistent with other implementations of grid layout?

Yes, actually it is not consistent with the rest of the CSS specs but the WG decided to add that exception to grid layout to be consistent mainly with IE's implementation. Regarding Blink, well I landed a similar fix there some months ago so yeah pretty consistent :).

>> Source/WebCore/rendering/RenderGrid.cpp:1020
>> +    m_columnPositions.resize(sizingData.columnTracks.size() + 1);
> 
> If we don't plan to extend past this size, let's use resizeToFit() to avoid padding the vector for growth.

Fair enough

>> Source/WebCore/rendering/RenderGrid.cpp:-1022
>> -    // FIXME: |columnTrack| and |rowTrack| should be smaller than our column / row count.
> 
> Does this FIXME no longer apply? Should we take it with us to populateGridPositions()?

It's an old comment that does no longer apply, even without this patch. Note that we're actually checking what the FIXME mentions in the two ASSERTs above. The cached grid coordinate is guaranteed to respect those limits because the grid grows if needed when the GridCoordinate is stored.

[Sergio Villar Senin]

Comment on attachment 222580 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1035
> +    ASSERT_UNUSED(sizingData, coordinate.rows.initialPositionIndex < rowTracks.size());

Actually this is wrong, the right parts of the conditions should be sizingData.columnTracks.size() and sizingData.rowTracks.size(). Don't know how I could upload a patch that does not build...

[Sergio Villar Senin]

Created attachment 223139 [details]
Patch

[Andreas Kling]

Comment on attachment 223139 [details]
Patch

OK, r=me

[Sergio Villar Senin]

Committed r163547: <http://trac.webkit.org/changeset/163547>

[Manuel Rego Casasnovas]

*** Bug 77566 has been marked as a duplicate of this bug. ***