Bug 110633 – [CSS Grid Layout] Interaction between auto-placement and column / row spanning

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

This is an open issue on the specification that was raised to www-style:

http://lists.w3.org/Archives/Public/www-style/2013Feb/0468.html

[Manuel Rego Casasnovas]

This is being addressed in https://codereview.chromium.org/196943026/ and should be ported to WebKit.

[Manuel Rego Casasnovas]

Actually there're some related changes in Blink that will be ported, all of the linked from the following bug:
http://code.google.com/p/chromium/issues/detail?id=353789

[Manuel Rego Casasnovas]

Created attachment 233361 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 233690 [details]
Patch

Rebased patch after r170182.

[Sergio Villar Senin]

Comment on attachment 233690 [details]
Patch

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

Awesome job. Please fix the nits before landing.

> Source/WebCore/ChangeLog:44
> +        (WebCore::RenderGrid::growGrid): Deleted. Renamed to ensureGridSize().

Nit no need for Deleted. It's more than enough with Renamed.

> Source/WebCore/rendering/RenderGrid.cpp:120
> +    bool checkEmptyCells(size_t rowSpan, size_t columnSpan) const

I don't like the function name, in this case it should be something like hasEnoughRoomForArea() or isEmptyAreaEnoughForItem() for example. Function name should start with isXXX() or hasXXX() but definitely not checkXXX().

> Source/WebCore/rendering/RenderGrid.cpp:124
> +        size_t maxColumns = std::min(m_columnIndex + columnSpan, m_grid[0].size());

Better use gridColumnCount() and gridRowCount().

> Source/WebCore/rendering/RenderGrid.cpp:149
>          const size_t endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.size() : m_grid[0].size();

I know it was not modified by this patch but use gridColumnCount() and gridRowCount() also here.

> Source/WebCore/rendering/RenderGrid.cpp:635
> +    const size_t oldRowSize = gridRowCount();

Let's use oldColumnCount and oldRowCount for consistency.

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:65
> +

Nit: extra line here.

> LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:19
> +    background-color: maroon;

I'm sure it was useful for testing :) but let's remove the background colors as they're not needed by the test.

> LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:11
> +    background-color: maroon;

Same comment here about colors.

[Manuel Rego Casasnovas]

(In reply to comment #5)
> (From update of attachment 233690 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233690&action=review
> 
> Awesome job. Please fix the nits before landing.

Thanks for the review!

> > Source/WebCore/ChangeLog:44
> > +        (WebCore::RenderGrid::growGrid): Deleted. Renamed to ensureGridSize().
> 
> Nit no need for Deleted. It's more than enough with Renamed.

Fixed.

> > Source/WebCore/rendering/RenderGrid.cpp:120
> > +    bool checkEmptyCells(size_t rowSpan, size_t columnSpan) const
> 
> I don't like the function name, in this case it should be something like hasEnoughRoomForArea() or isEmptyAreaEnoughForItem() for example. Function name should start with isXXX() or hasXXX() but definitely not checkXXX().

I've modified it, thanks for the suggestions.

> > Source/WebCore/rendering/RenderGrid.cpp:124
> > +        size_t maxColumns = std::min(m_columnIndex + columnSpan, m_grid[0].size());
> 
> Better use gridColumnCount() and gridRowCount().

We cannot use them from GridIterator, as we only have the m_grid but not access to the RenderGrid.
Probably it would be nice to explore other options here (like use a friend class).

> > Source/WebCore/rendering/RenderGrid.cpp:149
> >          const size_t endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.size() : m_grid[0].size();
> 
> I know it was not modified by this patch but use gridColumnCount() and gridRowCount() also here.

Ditto.

> > Source/WebCore/rendering/RenderGrid.cpp:635
> > +    const size_t oldRowSize = gridRowCount();
> 
> Let's use oldColumnCount and oldRowCount for consistency.

Modified.

> > Source/WebCore/rendering/style/GridResolvedPosition.cpp:65
> > +
> 
> Nit: extra line here.

Removed.

> > LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:19
> > +    background-color: maroon;
> 
> I'm sure it was useful for testing :) but let's remove the background colors as they're not needed by the test.

As we've discussed I prefer to keep them. When you run the test manually with run-launcher it makes much easier to understand it.
In addition, we're already using background colors in lots of places in grid tests.

> > LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:11
> > +    background-color: maroon;
> 
> Same comment here about colors.

Ditto.

[Manuel Rego Casasnovas]

Created attachment 233988 [details]
Patch for landing

[Manuel Rego Casasnovas]

Comment on attachment 233988 [details]
Patch for landing

Clearing flags on attachment: 233988

Committed r170531: <http://trac.webkit.org/changeset/170531>

[Manuel Rego Casasnovas]

All reviewed patches have been landed.  Closing bug.