WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
127033
[CSS Grid Layout] getComputedStyle() is wrong for grid-definition-{columns|rows}
https://bugs.webkit.org/show_bug.cgi?id=127033
Summary
[CSS Grid Layout] getComputedStyle() is wrong for grid-definition-{columns|rows}
Sergio Villar Senin
Reported
2014-01-15 00:59:17 PST
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
Attachments
Patch
(178.25 KB, patch)
2014-01-29 09:49 PST
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Patch
(178.29 KB, patch)
2014-01-29 10:22 PST
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Patch
(178.32 KB, patch)
2014-02-04 10:45 PST
,
Sergio Villar Senin
kling
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1
2014-01-29 09:49:31 PST
Created
attachment 222577
[details]
Patch
Sergio Villar Senin
Comment 2
2014-01-29 10:22:02 PST
Created
attachment 222580
[details]
Patch With fix for the mac EWS
Andreas Kling
Comment 3
2014-02-04 09:10:01 PST
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 4
2014-02-04 10:00:09 PST
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 5
2014-02-04 10:04:28 PST
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
Comment 6
2014-02-04 10:45:59 PST
Created
attachment 223139
[details]
Patch
Andreas Kling
Comment 7
2014-02-06 10:00:34 PST
Comment on
attachment 223139
[details]
Patch OK, r=me
Sergio Villar Senin
Comment 8
2014-02-06 10:08:48 PST
Committed
r163547
: <
http://trac.webkit.org/changeset/163547
>
Manuel Rego Casasnovas
Comment 9
2014-04-30 02:08:16 PDT
***
Bug 77566
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug