Bug 150679

Summary: [css-grid] Improve grid container sizing with size constraints and intrinsic sizes
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, esprehn+autocc, glenn, hyatt, jfernandez, kondapallykalyan, rego, rniwa, simon.fraser, svillar, zalan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Archive of layout-test-results from ews115 for mac-yosemite
none
Patch darin: review+

Description Sergio Villar Senin 2015-10-29 10:39:37 PDT
The current implementation does not comply with what specs say about the size available for the track sizing algorithm when the grid container is sized under min/max size restrictions and/or when it has intrinsic sizes.

Apart from that, we are not properly detecting whether the grid container has definite or indefinite sizes in both inline and block axis.
Comment 1 Sergio Villar Senin 2015-10-29 10:55:47 PDT
Created attachment 264323 [details]
Patch
Comment 2 Build Bot 2015-10-29 11:42:39 PDT
Comment on attachment 264323 [details]
Patch

Attachment 264323 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/354092

New failing tests:
fast/events/key-events-in-editable-gridbox.html
Comment 3 Build Bot 2015-10-29 11:42:44 PDT
Created attachment 264327 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 4 Build Bot 2015-10-29 11:46:56 PDT
Comment on attachment 264323 [details]
Patch

Attachment 264323 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/354111

New failing tests:
fast/events/key-events-in-editable-gridbox.html
Comment 5 Build Bot 2015-10-29 11:46:59 PDT
Created attachment 264329 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 6 Build Bot 2015-10-29 11:55:43 PDT
Comment on attachment 264323 [details]
Patch

Attachment 264323 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/354155

New failing tests:
fast/events/key-events-in-editable-gridbox.html
Comment 7 Build Bot 2015-10-29 11:55:48 PDT
Created attachment 264330 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Darin Adler 2015-10-31 14:34:10 PDT
Comment on attachment 264323 [details]
Patch

The changes in the patch look fine, but it looks like this breaks fast/events/key-events-in-editable-gridbox.html -- I think the breakage is real and just not just an anomaly of some sort.
Comment 9 Sergio Villar Senin 2015-11-02 04:13:18 PST
Created attachment 264573 [details]
Patch

Fixed the contenteditable behavior. I took the chance to improve a bit that test and check more cases (different kinds of heights)
Comment 10 Darin Adler 2015-11-02 21:53:42 PST
Comment on attachment 264573 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:210
>  class RenderGrid::GridSizingData {

Something that has this much public data should probably be a struct, rather than a class.

> Source/WebCore/rendering/RenderGrid.cpp:235
> +    void setFreeSpaceForDirection(GridTrackSizingDirection direction, Optional<LayoutUnit> freeSpace)
> +    {
> +        if (direction == ForColumns)
> +            freeSpaceForColumns = freeSpace;
> +        else
> +            freeSpaceForRows = freeSpace;
> +    }

Not a big fan of functions like this in the class definition. I think it would be more elegant to put the function definition outside the GridSizingData class.

> Source/WebCore/rendering/RenderGrid.cpp:418
> +    const_cast<RenderGrid*>(this)->computeUsedBreadthOfGridTracks(ForColumns, sizingData, minLogicalWidth, maxLogicalWidth);

Not a huge fan of the const_cast idiom used in this class.
Comment 11 Sergio Villar Senin 2015-11-03 02:07:24 PST
(In reply to comment #10)
> Comment on attachment 264573 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264573&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:210
> >  class RenderGrid::GridSizingData {
> 
> Something that has this much public data should probably be a struct, rather
> than a class.

Ack

> > Source/WebCore/rendering/RenderGrid.cpp:235
> > +    void setFreeSpaceForDirection(GridTrackSizingDirection direction, Optional<LayoutUnit> freeSpace)
> > +    {
> > +        if (direction == ForColumns)
> > +            freeSpaceForColumns = freeSpace;
> > +        else
> > +            freeSpaceForRows = freeSpace;
> > +    }
> 
> Not a big fan of functions like this in the class definition. I think it
> would be more elegant to put the function definition outside the
> GridSizingData class.

I'll do it

> > Source/WebCore/rendering/RenderGrid.cpp:418
> > +    const_cast<RenderGrid*>(this)->computeUsedBreadthOfGridTracks(ForColumns, sizingData, minLogicalWidth, maxLogicalWidth);
> 
> Not a huge fan of the const_cast idiom used in this class.

Note that RenderBox also needs them for the preferred logical widths computations. All these renderers need to store the computed values but we don't consider them a modification on state of the object.
Comment 12 Darin Adler 2015-11-03 09:33:20 PST
(In reply to comment #11)
> Note that RenderBox also needs them for the preferred logical widths
> computations. All these renderers need to store the computed values but we
> don't consider them a modification on state of the object.

I believe the preferred style is to mark the data members that store the computed values mutable rather than using const_cast.
Comment 13 Sergio Villar Senin 2015-11-09 04:47:04 PST
(In reply to comment #12)
> (In reply to comment #11)
> > Note that RenderBox also needs them for the preferred logical widths
> > computations. All these renderers need to store the computed values but we
> > don't consider them a modification on state of the object.
> 
> I believe the preferred style is to mark the data members that store the
> computed values mutable rather than using const_cast.

Sure, but if you don't mind I'll do it in a separate patch since this one does not add/modify any existing const_cast.
Comment 14 Sergio Villar Senin 2015-11-09 04:56:51 PST
Committed r192154: <http://trac.webkit.org/changeset/192154>