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.
Created attachment 223649 [details] Patch
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.
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.
Comment on attachment 223649 [details] Patch I'm actually going to minus this, since I want clarification on this suspicious function.
(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 ?
(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.
Created attachment 224907 [details] Patch rebased and applied suggested changed.
Comment on attachment 224907 [details] Patch rebased and applied suggested changed. r=me
Comment on attachment 224907 [details] Patch rebased and applied suggested changed. Clearing flags on attachment: 224907 Committed r164609: <http://trac.webkit.org/changeset/164609>
All reviewed patches have been landed. Closing bug.