Bug 110633 - [CSS Grid Layout] Interaction between auto-placement and column / row spanning
Summary: [CSS Grid Layout] Interaction between auto-placement and column / row spanning
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 103316
  Show dependency treegraph
 
Reported: 2013-02-22 12:57 PST by Julien Chaffraix
Modified: 2014-06-27 09:41 PDT (History)
18 users (show)

See Also:


Attachments
Patch (47.96 KB, patch)
2014-06-19 08:10 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (47.78 KB, patch)
2014-06-24 01:18 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (47.79 KB, patch)
2014-06-27 09:27 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 Julien Chaffraix 2013-02-22 12:57:18 PST
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
Comment 1 Manuel Rego Casasnovas 2014-04-30 02:11:57 PDT
This is being addressed in https://codereview.chromium.org/196943026/ and should be ported to WebKit.
Comment 2 Manuel Rego Casasnovas 2014-06-19 02:41:20 PDT
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
Comment 3 Manuel Rego Casasnovas 2014-06-19 08:10:19 PDT
Created attachment 233361 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2014-06-24 01:18:15 PDT
Created attachment 233690 [details]
Patch

Rebased patch after r170182.
Comment 5 Sergio Villar Senin 2014-06-27 07:43:34 PDT
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.
Comment 6 Manuel Rego Casasnovas 2014-06-27 09:25:36 PDT
(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.
Comment 7 Manuel Rego Casasnovas 2014-06-27 09:27:38 PDT
Created attachment 233988 [details]
Patch for landing
Comment 8 Manuel Rego Casasnovas 2014-06-27 09:41:40 PDT
Comment on attachment 233988 [details]
Patch for landing

Clearing flags on attachment: 233988

Committed r170531: <http://trac.webkit.org/changeset/170531>
Comment 9 Manuel Rego Casasnovas 2014-06-27 09:41:54 PDT
All reviewed patches have been landed.  Closing bug.