Bug 127033

Summary: [CSS Grid Layout] getComputedStyle() is wrong for grid-definition-{columns|rows}
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, gyuyoung.kim, jchaffraix, joepeck, kling, koivisto, kondapallykalyan, macpherson, menard, rniwa, sam, simon.fraser, svillar, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 127057    
Bug Blocks: 60731, 103761, 128173    
Attachments:
Description Flags
Patch
none
Patch
none
Patch kling: review+

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
DetailsFormatted DiffDiff
Patch (178.29 KB, patch)
2014-01-29 10:22 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (178.32 KB, patch)
2014-02-04 10:45 PST, Sergio Villar Senin 		kling: review+
DetailsFormatted DiffDiff
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.