Bug 150679 – [css-grid] Improve grid container sizing with size constraints and intrinsic sizes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Created attachment 264323 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Sergio Villar Senin]

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)

[Darin Adler]

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.

[Sergio Villar Senin]

(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.

[Darin Adler]

(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.

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

Committed r192154: <http://trac.webkit.org/changeset/192154>