[CSS Grid Layout] handle undefined RemainingSpace in computeUsedBreadthOfGridTracks algorithm

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Javier Fernandez]

The spec defines a different code path for the computeUsedBreadthOfGridTracks algorithm http://dev.w3.org/csswg/css-grid/#function-ComputeUsedBreadthOfGridTracks.
The current code has a FIXME that mentions this missing piece in our implementation.

Basically the track breadth is different when the available size is undefined and thus, cannot be taken into account during the computations.
The available size is undefined whenever the height is auto or the grid element has a shrink-to-fit behavior.

It would be also a good idea to rename the function to match the specs so the function name will start with compute instead of computed.

[Javier Fernandez]

Created attachment 223649 [details]
Patch

[Dave Hyatt]

Comment on attachment 223649 [details]
Patch

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

r=me, although concerned about the one function I called out.

> Source/WebCore/rendering/RenderGrid.cpp:269
> +static bool gridElementIsShrinkToFit(const RenderStyle& style)
> +{
> +    return style.isFloating() || style.position() == AbsolutePosition;
> +}

This looks suspicious to me. Are you wanting to catch everybody that just sizes intrinsically? This is not enough if so.

[Dave Hyatt]

Comment on attachment 223649 [details]
Patch

Also, do you not have a renderer? Checking styles for floating and absolute is weird, since you don't know for sure that they applied. The RenderObject bits are better, i.e., isFloating() and isPositioned. We even have a helper function to test both at once.

[Dave Hyatt]

Comment on attachment 223649 [details]
Patch

I'm actually going to minus this, since I want clarification on this suspicious function.

[Javier Fernandez]

(In reply to comment #2)
> (From update of attachment 223649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223649&action=review
> 
> r=me, although concerned about the one function I called out.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:269
> > +static bool gridElementIsShrinkToFit(const RenderStyle& style)
> > +{
> > +    return style.isFloating() || style.position() == AbsolutePosition;
> > +}
> 
> This looks suspicious to me. Are you wanting to catch everybody that just sizes intrinsically? This is not enough if so.

Well, the patch tries to implement what it's stated in the Grid Layout specifications:

http://www.w3.org/TR/css-grid-1/#RemainingSpace-definition

"The max of zero and the AvailableSpace less the sum of all Grid track UsedBreadth values. This is undefined if AvailableSpace is undefined (i.e. the Grid element is shrink-to-fit or the height is auto.)"

It wasn't obvious how to implement this shrink-to-fit behavior. so I assumed that it was triggered whenever a floating or positioned object is rendered. 

Would you mind suggesting some alternative approaches ?

[Javier Fernandez]

(In reply to comment #3)
> (From update of attachment 223649 [details])
> Also, do you not have a renderer? Checking styles for floating and absolute is weird, since you don't know for sure that they applied. The RenderObject bits are better, i.e., isFloating() and isPositioned. We even have a helper function to test both at once.

Yeah, you're totally right on this. If it happens that floating or positioned objects are the ones triggering the shrink-to-fit effect, then the patch should use the helper function you mentioned. However, let's clarify that first.

[Javier Fernandez]

Created attachment 224907 [details]
Patch rebased and applied suggested changed.

[Dave Hyatt]

Comment on attachment 224907 [details]
Patch rebased and applied suggested changed.

r=me

[WebKit Commit Bot]

Comment on attachment 224907 [details]
Patch rebased and applied suggested changed.

Clearing flags on attachment: 224907

Committed r164609: <http://trac.webkit.org/changeset/164609>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.