Bug 159294

Summary: [css-grid] Handle min-content/max-content with orthogonal flows
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: Layout and RenderingAssignee: Javier Fernandez <jfernandez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, hyatt, jfernandez, kondapallykalyan, rego, simon.fraser, svillar
Priority: P2 Keywords: BlinkMergeCandidate
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60731, 159295    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Missing layout tests
none
Missing Layout tests
none
Missing Layout tests none

Description Javier Fernandez 2016-06-30 03:53:26 PDT
Grid Layout does not support orthogonal flows cases. 
It's not only the grid items are not aligned properly, but tracks are sizing incorrectly as well.
Comment 1 Javier Fernandez 2016-06-30 04:03:25 PDT
Created attachment 282429 [details]
Patch
Comment 2 Darin Adler 2016-07-13 09:16:57 PDT
Comment on attachment 282429 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:245
> +    void nextState()

A function like this that creates side effects and has no return value should normally have a verb phrase as its name, not a noun phrase. So instead of "next state" it would be "advance to next state", or anything else that is a verb, not a noun.

> Source/WebCore/rendering/RenderGrid.cpp:264
> +    bool isValidTransitionForDirection(GridTrackSizingDirection direction)

Not sure this needs "for direction" in its name, although I suppose it’s OK.

> Source/WebCore/rendering/RenderGrid.cpp:268
> +            return direction == ForColumns ? true : false;

No need for the no-op "? true : false".

> Source/WebCore/rendering/RenderGrid.cpp:270
> +            return direction == ForRows ? true : false;

Ditto.

> Source/WebCore/rendering/RenderGrid.cpp:276
> +            return direction == ForRows ? true : false;

Ditto.

> Source/WebCore/rendering/RenderGrid.h:208
> +    bool m_hasAnyOrthogonalChild;

I’m a little concerned that the lifetime of this data member is tricky. It’s valid only after placeItemsOnGrid is called. Is there an obvious guarantee that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid?
Comment 3 Manuel Rego Casasnovas 2016-07-13 23:23:28 PDT
Comment on attachment 282429 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.h:208
>> +    bool m_hasAnyOrthogonalChild;
> 
> I’m a little concerned that the lifetime of this data member is tricky. It’s valid only after placeItemsOnGrid is called. Is there an obvious guarantee that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid?

A quick idea would be to use an ASSERT(!m_gridIsDirty); in RenderGrid::repeatTracksSizingIfNeeded().

Also maybe it's worth to reset m_hasAnyOrthogonalChild in RenderGrid::clearGrid().
Comment 4 Javier Fernandez 2016-07-14 10:23:22 PDT
(In reply to comment #3)
> Comment on attachment 282429 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282429&action=review
> 
> >> Source/WebCore/rendering/RenderGrid.h:208
> >> +    bool m_hasAnyOrthogonalChild;
> > 
> > I’m a little concerned that the lifetime of this data member is tricky. It’s valid only after placeItemsOnGrid is called. Is there an obvious guarantee that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid?
> 
> A quick idea would be to use an ASSERT(!m_gridIsDirty); in
> RenderGrid::repeatTracksSizingIfNeeded().

I like this idea.

> 
> Also maybe it's worth to reset m_hasAnyOrthogonalChild in
> RenderGrid::clearGrid().

I think there is no need to clear the value, as it's initialized every time the placeItemsOnGrid function is called.
Comment 5 Javier Fernandez 2016-07-14 10:25:28 PDT
(In reply to comment #2)
> Comment on attachment 282429 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282429&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:245
> > +    void nextState()
> 
> A function like this that creates side effects and has no return value
> should normally have a verb phrase as its name, not a noun phrase. So
> instead of "next state" it would be "advance to next state", or anything
> else that is a verb, not a noun.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:264
> > +    bool isValidTransitionForDirection(GridTrackSizingDirection direction)
> 
> Not sure this needs "for direction" in its name, although I suppose it’s OK.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:268
> > +            return direction == ForColumns ? true : false;
> 
> No need for the no-op "? true : false".
> 
> > Source/WebCore/rendering/RenderGrid.cpp:270
> > +            return direction == ForRows ? true : false;
> 
> Ditto.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:276
> > +            return direction == ForRows ? true : false;
> 
> Ditto.
> 
> > Source/WebCore/rendering/RenderGrid.h:208
> > +    bool m_hasAnyOrthogonalChild;
> 
> I’m a little concerned that the lifetime of this data member is tricky. It’s
> valid only after placeItemsOnGrid is called. Is there an obvious guarantee
> that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid?

As rego suggested, I'll add an ASSERT to be sure the grid is not dirty, hence the
placeItemsOnGrid completed its execution.
Comment 6 Javier Fernandez 2016-07-14 10:38:55 PDT
Created attachment 283659 [details]
Patch
Comment 7 Javier Fernandez 2016-07-14 15:28:26 PDT
Created attachment 283692 [details]
Patch
Comment 8 WebKit Commit Bot 2016-07-14 16:34:02 PDT
Comment on attachment 283692 [details]
Patch

Clearing flags on attachment: 283692

Committed r203252: <http://trac.webkit.org/changeset/203252>
Comment 9 WebKit Commit Bot 2016-07-14 16:34:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Javier Fernandez 2016-07-21 02:41:17 PDT
The last patch I've submitted for landing didn't have the layout tests, which were already reviewed in the first patch. 

I'll reopen this bug to land the tests.
Comment 11 Javier Fernandez 2016-07-21 02:46:10 PDT
Created attachment 284200 [details]
Missing layout tests

This patch complements the previous one, which somehow didn't contain the proper layout tests.
Comment 12 Javier Fernandez 2016-07-21 02:53:05 PDT
Created attachment 284201 [details]
Missing Layout tests
Comment 13 WebKit Commit Bot 2016-07-21 03:51:49 PDT
Comment on attachment 284201 [details]
Missing Layout tests

Rejecting attachment 284201 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 284201, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog is not at the top of the file.

Full output: http://webkit-queues.webkit.org/results/1723440
Comment 14 Javier Fernandez 2016-07-21 04:05:12 PDT
Created attachment 284204 [details]
Missing Layout tests
Comment 15 WebKit Commit Bot 2016-07-21 04:37:06 PDT
Comment on attachment 284204 [details]
Missing Layout tests

Clearing flags on attachment: 284204

Committed r203501: <http://trac.webkit.org/changeset/203501>
Comment 16 WebKit Commit Bot 2016-07-21 04:37:12 PDT
All reviewed patches have been landed.  Closing bug.