Bug 156793

Summary: [css-grid] Fix grid-template-columns|rows computed style with content alignment
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jfernandez, rego, simon.fraser, svillar
Priority: P2 Keywords: BlinkMergeCandidate
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Manuel Rego Casasnovas
Reported Wednesday, April 20, 2016 12:48:55 PM UTC
Since we support content alignment (space-between and so on) the computed style for grid-template-columns and grid-template-rows is wrong as it's including the distribution offset. However it doesn't include the gaps as we're already subtracting them, we should do the same for the distribution offset. To fix this we'll port this patch from Blink: https://codereview.chromium.org/1903183002/
Attachments
Patch (12.83 KB, patch)
2016-04-20 04:51 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (13.94 KB, patch)
2016-04-21 01:54 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch for landing (14.31 KB, patch)
2016-04-25 02:06 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 Wednesday, April 20, 2016 12:51:39 PM UTC
Created attachment 276818 [details] Patch
Manuel Rego Casasnovas
Comment 2 Thursday, April 21, 2016 9:54:43 AM UTC
Created attachment 276903 [details] Patch
Darin Adler
Comment 3 Sunday, April 24, 2016 5:55:15 PM UTC
Comment on attachment 276903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276903&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1073 > + const auto& grid = downcast<RenderGrid>(*renderer); I don’t think the const here is helpful. Just auto& would be fine. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1074 > + const Vector<LayoutUnit>& trackPositions = direction == ForColumns ? grid.columnPositions() : grid.rowPositions(); I think this would be better if it said auto& instead of writing out the type. > Source/WebCore/rendering/RenderGrid.h:68 > + LayoutUnit offsetBetweenTracks(GridTrackSizingDirection direction) const > + { > + return direction == ForColumns ? m_offsetBetweenColumns : m_offsetBetweenRows; > + } Normally it’s easier to read class definitions if they don’t have multi-line function definitions inside them. Would you consider putting the inline function definition separately after the class definition and just declaring the function here?
Manuel Rego Casasnovas
Comment 4 Monday, April 25, 2016 10:06:40 AM UTC
Created attachment 277235 [details] Patch for landing
Manuel Rego Casasnovas
Comment 5 Monday, April 25, 2016 10:07:49 AM UTC
Thanks for the review. (In reply to comment #3) > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1073 > > + const auto& grid = downcast<RenderGrid>(*renderer); > > I don’t think the const here is helpful. Just auto& would be fine. Done. > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1074 > > + const Vector<LayoutUnit>& trackPositions = direction == ForColumns ? grid.columnPositions() : grid.rowPositions(); > > I think this would be better if it said auto& instead of writing out the > type. Done. > > Source/WebCore/rendering/RenderGrid.h:68 > > + LayoutUnit offsetBetweenTracks(GridTrackSizingDirection direction) const > > + { > > + return direction == ForColumns ? m_offsetBetweenColumns : m_offsetBetweenRows; > > + } > > Normally it’s easier to read class definitions if they don’t have multi-line > function definitions inside them. Would you consider putting the inline > function definition separately after the class definition and just declaring > the function here? Moved the implementation to the .cpp.
WebKit Commit Bot
Comment 6 Monday, April 25, 2016 10:54:23 AM UTC
Comment on attachment 277235 [details] Patch for landing Clearing flags on attachment: 277235 Committed r199981: <http://trac.webkit.org/changeset/199981>
WebKit Commit Bot
Comment 7 Monday, April 25, 2016 10:54:28 AM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.