Bug 156793 - [css-grid] Fix grid-template-columns|rows computed style with content alignment
Summary: [css-grid] Fix grid-template-columns|rows computed style with content alignment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2016-04-20 04:48 PDT by Manuel Rego Casasnovas
Modified: 2016-04-25 02:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.83 KB, patch)
2016-04-20 04:51 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (13.94 KB, patch)
2016-04-21 01:54 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (14.31 KB, patch)
2016-04-25 02:06 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2016-04-20 04:48:55 PDT
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/
Comment 1 Manuel Rego Casasnovas 2016-04-20 04:51:39 PDT
Created attachment 276818 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2016-04-21 01:54:43 PDT
Created attachment 276903 [details]
Patch
Comment 3 Darin Adler 2016-04-24 09:55:15 PDT
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?
Comment 4 Manuel Rego Casasnovas 2016-04-25 02:06:40 PDT
Created attachment 277235 [details]
Patch for landing
Comment 5 Manuel Rego Casasnovas 2016-04-25 02:07:49 PDT
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.
Comment 6 WebKit Commit Bot 2016-04-25 02:54:23 PDT
Comment on attachment 277235 [details]
Patch for landing

Clearing flags on attachment: 277235

Committed r199981: <http://trac.webkit.org/changeset/199981>
Comment 7 WebKit Commit Bot 2016-04-25 02:54:28 PDT
All reviewed patches have been landed.  Closing bug.