Bug 106474 – [CSS Grid Layout] Add support for min-content

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

We currently don't implement min-content used in a minmax function, regardless of whether it is a max or a min.

This change will introduce support for min-content only in the column direction as the information is already available using minPreferredLogicalWidth. The row direction will need some special handling as we don't have the concept of preferred logical heights and thus will be punted to a follow-up change. The orthogonal writing mode between the grid element and the grid item case will also suffer from this absence of information.

[Julien Chaffraix]

Created attachment 181958 [details]
RFC patch.

[Julien Chaffraix]

After talking with Ojan, min-content and max-content in the row direction should resolve to the logical height (per http://dev.w3.org/csswg/css3-sizing/). Following this, I went ahead and extended the change for logical height too. Re-purposing the bug to cover both directions.

[Julien Chaffraix]

Created attachment 182667 [details]
Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug.

[Early Warning System Bot]

Comment on attachment 182667 [details]
Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug.

Attachment 182667 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15884359

[Early Warning System Bot]

Comment on attachment 182667 [details]
Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug.

Attachment 182667 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15838729

[WebKit Review Bot]

Comment on attachment 182667 [details]
Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug.

Attachment 182667 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15869711

[Build Bot]

Comment on attachment 182667 [details]
Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug.

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 182667 [details]
Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug.

Attachment 182667 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15870757

[Julien Chaffraix]

Created attachment 182684 [details]
Proposed fix 2: Fixed the release build.

[Build Bot]

Comment on attachment 182684 [details]
Proposed fix 2: Fixed the release build.

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

[Julien Chaffraix]

Created attachment 182808 [details]
Proposed implementation 3: Fixed the build for real this time.

[Tony Chang]

Comment on attachment 182808 [details]
Proposed implementation 3: Fixed the build for real this time.

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

I just have nits, seems like a fine first pass.  I'll let Ojan set the r? flag after his review.

> Source/WebCore/rendering/RenderGrid.cpp:51
> +        if (m_maxBreadth == infinity)
> +            return m_usedBreadth;
> +
> +        return m_maxBreadth;

Nit: I would probably use a ternary here, but this is also OK.

> Source/WebCore/rendering/RenderGrid.cpp:196
> +    // Per the spec, we return 0 if the track length is min-content, max-content or a fraction.

Nit: Rather than a comment, I would put an assert here that track length is min-content, max-content or a fraction (well, I guess we don't support fraction yet).

> Source/WebCore/rendering/RenderGrid.cpp:204
> +    if (trackLength.isFixed() || trackLength.isPercent() || trackLength.isViewportPercentage()) {
> +        LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(direction, trackLength);
> +        ASSERT(computedBreadth != infinity);

If a large fixed value or percent is used, couldn't the computedBreadth == infinity?

> Source/WebCore/rendering/RenderGrid.cpp:214
> +    // FIXME: We still need to support calc() here (bug 103761).

Nit: Using a full link for the bug would be nice.

> Source/WebCore/rendering/RenderGrid.cpp:253
> +        // FIXME: Implement 3.a.ii and 3.b.ii.

Nit: I would replace 3.a.ii and 3.b.ii with a textual description of what should happen.

> LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:12
> +    display: -webkit-grid;
> +    background-color: grey;
> +    -webkit-grid-columns: minmax(-webkit-min-content, 40px);
> +    -webkit-grid-rows: 50px;

This would be a great time to add a resources/grid.css file and put the -webkit-grid values in it so we can handle new values easier.

> LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:45
> +<p>Test that resolving minmax function with min-content on grid items works properly with different writing modes.</p>

You mention different writing modes, but all the tests seem to be the same writing mode (verticalRL is unused).  I think it's fine to either check in failing tests or add them in a follow up.

> LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-rows.html:45
> +<p>Test that resolving minmax function with min-content on grid items works properly with different writing modes.</p>

Same here.

[Ojan Vafai]

Comment on attachment 182808 [details]
Proposed implementation 3: Fixed the build for real this time.

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

Overall design looks good. Some questions on the details.

> Source/WebCore/rendering/RenderGrid.cpp:209
> +    if (trackLength.isFixed() || trackLength.isPercent() || trackLength.isViewportPercentage()) {
> +        LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(direction, trackLength);
> +        ASSERT(computedBreadth != infinity);
> +        return computedBreadth;
> +    }
> +
> +    // Per the spec we return Infinity if the track length is min-content, max-content or a fraction.
> +    return infinity;

FWIW, the way RenderBox handles code like this is to do something like the following:
LayoutUnit RenderGrid::computeUsedBreadthOfSpecifiedLength(...)
{
    if (trackLength.isFixed() || ...)
        return valueForLength(...);
    return -1;
}

LayoutUnit RenderGrid::computeUsedBreadthOfMaxLength()
{
    LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(...);
    if (computedBreadth == -1)
        return infinity;
    return computedBreadth;
}

The nice thing about doing it this way is that you share more code and as you add other value types to computeUsedBreadthOfSpecifiedLength, you don't need to modify computeUsedBreadthOfMaxLength and computeUsedBreadthOfMinLength.

> Source/WebCore/rendering/RenderGrid.cpp:229
> +        return LayoutUnit();

Nit: I'd just return 0 here.

> Source/WebCore/rendering/RenderGrid.cpp:232
> +        return child->minPreferredLogicalWidth();

What should this return if the child has a fixed width/min-width/max-width set directly on it? If it should ignore them, then you may need to use minIntrsinsicLogicalWidth here.

> Source/WebCore/rendering/RenderGrid.cpp:239
> +        // FIXME: We shouldn't unconditionally relayout the child but detect a size change.
> +        child->setNeedsLayout(true, MarkOnlyThis);

I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout).

> Source/WebCore/rendering/RenderGrid.cpp:257
> +            // FIXME: We should share this logical with the other branches.

This sentence is missing some words. Not sure what's it's trying to say.

> Source/WebCore/rendering/RenderGrid.cpp:259
> +                size_t cellIndex = resolveGridPosition((direction == ForColumns) ? child->style()->gridItemColumn() : child->style()->gridItemRow());

Can we make a version of resolveGridPosition that takes a direction and a RenderObject* child? I think that would make this code here and below a bit more readable.

[Julien Chaffraix]

Comment on attachment 182808 [details]
Proposed implementation 3: Fixed the build for real this time.

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

>> Source/WebCore/rendering/RenderGrid.cpp:204
>> +        ASSERT(computedBreadth != infinity);
> 
> If a large fixed value or percent is used, couldn't the computedBreadth == infinity?

It's entirely possible indeed and probably will occur more frequently with saturated arithmetic enabled on some platforms.

It's difficult to use other values (e.g the magic constant -1) as the spec allows a <length> (an arbitrary number with a unit per http://www.w3.org/TR/2012/CR-css3-values-20120828/#lengths) which mean that any values can be potentially hit. We could potentially bring this back to the specification as I couldn't see of a good use case for negative values.

Also AFAICT the code would still behave correctly as we still check the Length's type in the layout code. The ASSERT can be removed as it can be hit and doesn't provide much coverage.

>> Source/WebCore/rendering/RenderGrid.cpp:209
>> +    return infinity;
> 
> FWIW, the way RenderBox handles code like this is to do something like the following:
> LayoutUnit RenderGrid::computeUsedBreadthOfSpecifiedLength(...)
> {
>     if (trackLength.isFixed() || ...)
>         return valueForLength(...);
>     return -1;
> }
> 
> LayoutUnit RenderGrid::computeUsedBreadthOfMaxLength()
> {
>     LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(...);
>     if (computedBreadth == -1)
>         return infinity;
>     return computedBreadth;
> }
> 
> The nice thing about doing it this way is that you share more code and as you add other value types to computeUsedBreadthOfSpecifiedLength, you don't need to modify computeUsedBreadthOfMaxLength and computeUsedBreadthOfMinLength.

Sounds good, as long as we never expose -1, it should be fine (some of the code uses MaxBreadth as-is which requires us to have a really infinite value).

>> Source/WebCore/rendering/RenderGrid.cpp:239
>> +        child->setNeedsLayout(true, MarkOnlyThis);
> 
> I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout).

This was the purpose of the FIXME here. We should probably check the sizes when we constrain the child and check if there is some difference before doing the layout. Let me see if I can find some better check.

>> Source/WebCore/rendering/RenderGrid.cpp:253
>> +        // FIXME: Implement 3.a.ii and 3.b.ii.
> 
> Nit: I would replace 3.a.ii and 3.b.ii with a textual description of what should happen.

Those steps are just a call to ResolveContentBasedTrackSizingFunctionsForItems with different arguments so it's pretty difficult to present them.

I could probably sum it up as: // FIXME: Implement MaxContent handling.

>> Source/WebCore/rendering/RenderGrid.cpp:257
>> +            // FIXME: We should share this logical with the other branches.
> 
> This sentence is missing some words. Not sure what's it's trying to say.

It's a typo: s/logical/logic/

Maybe in better English: // FIXME: We should share the growing / iterating logic as done with ResolveContentBasedTrackSizingFunctionsForItems in the specification.

>> Source/WebCore/rendering/RenderGrid.cpp:259
>> +                size_t cellIndex = resolveGridPosition((direction == ForColumns) ? child->style()->gridItemColumn() : child->style()->gridItemRow());
> 
> Can we make a version of resolveGridPosition that takes a direction and a RenderObject* child? I think that would make this code here and below a bit more readable.

Sounds good.

>> LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:12
>> +    -webkit-grid-rows: 50px;
> 
> This would be a great time to add a resources/grid.css file and put the -webkit-grid values in it so we can handle new values easier.

It's the second time you told me about that. The first time went through the crack so I filed bug 107044 about it as it's a super good idea but orthogonal to this bug.

>> LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:45
>> +<p>Test that resolving minmax function with min-content on grid items works properly with different writing modes.</p>
> 
> You mention different writing modes, but all the tests seem to be the same writing mode (verticalRL is unused).  I think it's fine to either check in failing tests or add them in a follow up.

I would prefer to follow-up. The code currently disable orthogonal writing-mode so we will need to add the required writing mode testing.

[Ojan Vafai]

Comment on attachment 182808 [details]
Proposed implementation 3: Fixed the build for real this time.

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

>>> Source/WebCore/rendering/RenderGrid.cpp:239
>>> +        child->setNeedsLayout(true, MarkOnlyThis);
>> 
>> I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout).
> 
> This was the purpose of the FIXME here. We should probably check the sizes when we constrain the child and check if there is some difference before doing the layout. Let me see if I can find some better check.

It looks to me like we're missing a setNeedsLayout in layoutGridItems around line 234.

[Julien Chaffraix]

Comment on attachment 182808 [details]
Proposed implementation 3: Fixed the build for real this time.

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

>>> Source/WebCore/rendering/RenderGrid.cpp:204
>>> +        ASSERT(computedBreadth != infinity);
>> 
>> If a large fixed value or percent is used, couldn't the computedBreadth == infinity?
> 
> It's entirely possible indeed and probably will occur more frequently with saturated arithmetic enabled on some platforms.
> 
> It's difficult to use other values (e.g the magic constant -1) as the spec allows a <length> (an arbitrary number with a unit per http://www.w3.org/TR/2012/CR-css3-values-20120828/#lengths) which mean that any values can be potentially hit. We could potentially bring this back to the specification as I couldn't see of a good use case for negative values.
> 
> Also AFAICT the code would still behave correctly as we still check the Length's type in the layout code. The ASSERT can be removed as it can be hit and doesn't provide much coverage.

I raised http://lists.w3.org/Archives/Public/www-style/2013Jan/0216.html about allowing negative <track-breadth>. As it's an external dependency, I filed bug 107053 to cover changing the value of infinity depending on what the resolution is.

>>>> Source/WebCore/rendering/RenderGrid.cpp:239
>>>> +        child->setNeedsLayout(true, MarkOnlyThis);
>>> 
>>> I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout).
>> 
>> This was the purpose of the FIXME here. We should probably check the sizes when we constrain the child and check if there is some difference before doing the layout. Let me see if I can find some better check.
> 
> It looks to me like we're missing a setNeedsLayout in layoutGridItems around line 234.

This change would be a lot broader but it would cover dynamic changes to -webkit-grid-rows / -webkit-grid-columns. Unfortunately we don't have dynamic tests that cover this and that's why it went unnoticed. Will add more testing for that.

[Julien Chaffraix]

Comment on attachment 182808 [details]
Proposed implementation 3: Fixed the build for real this time.

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

>>> Source/WebCore/rendering/RenderGrid.cpp:209
>>> +    return infinity;
>> 
>> FWIW, the way RenderBox handles code like this is to do something like the following:
>> LayoutUnit RenderGrid::computeUsedBreadthOfSpecifiedLength(...)
>> {
>>     if (trackLength.isFixed() || ...)
>>         return valueForLength(...);
>>     return -1;
>> }
>> 
>> LayoutUnit RenderGrid::computeUsedBreadthOfMaxLength()
>> {
>>     LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(...);
>>     if (computedBreadth == -1)
>>         return infinity;
>>     return computedBreadth;
>> }
>> 
>> The nice thing about doing it this way is that you share more code and as you add other value types to computeUsedBreadthOfSpecifiedLength, you don't need to modify computeUsedBreadthOfMaxLength and computeUsedBreadthOfMinLength.
> 
> Sounds good, as long as we never expose -1, it should be fine (some of the code uses MaxBreadth as-is which requires us to have a really infinite value).

I have to withdraw this comment. As the spec allows trackLength to be negative, nothing prevents valueForLength to return -1. Under your proposal, the code will misbehave in this case which it doesn't here. This is the same issue as infinity being set to -1 which I postponed to a bug 107053.

>> Source/WebCore/rendering/RenderGrid.cpp:232
>> +        return child->minPreferredLogicalWidth();
> 
> What should this return if the child has a fixed width/min-width/max-width set directly on it? If it should ignore them, then you may need to use minIntrsinsicLogicalWidth here.

That's a very good question. The specification is silent on this point. I would have taken width/min-width/max-width into account but that seems to violate http://dev.w3.org/csswg/css3-sizing/#intrinsic-sizing. As we decided to interpret min-content / max-content as defined in css3 sizing, it would be nice to be consistent or have the spec be precise here so I raised http://lists.w3.org/Archives/Public/www-style/2013Jan/0245.html about that.

[Julien Chaffraix]

Created attachment 183340 [details]
Proposed implementation 4: Updated after Ojan & Tony's comments.

[Tony Chang]

Comment on attachment 183340 [details]
Proposed implementation 4: Updated after Ojan & Tony's comments.

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

LGTM, but I'll let Ojan add any additional comments.

> Source/WebCore/rendering/RenderGrid.cpp:201
> +        ASSERT(computedBreadth != infinity);

Nit: Since it's possible to hit this ASSERT, I would remove it.  Maybe replace it with a FIXME?

> Source/WebCore/rendering/RenderGrid.cpp:256
> +            for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
> +                size_t cellIndex = resolveGridPosition(direction, child);
> +                if (cellIndex != i)
> +                    continue;

This nested loop looks like it could be slow if there are a large number of tracks and children. We may need to make a map, but this is OK for now.

[Ojan Vafai]

Comment on attachment 183340 [details]
Proposed implementation 4: Updated after Ojan & Tony's comments.

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

> Source/WebCore/rendering/RenderGrid.cpp:205
> +    ASSERT(trackLength.isMinContent() || trackLength.isMaxContent() || trackLength.isAuto());

Can it not be fit-content, fill-available, etc?

> Source/WebCore/rendering/RenderGrid.cpp:229
> +    if (direction == ForColumns)
> +        return child->minPreferredLogicalWidth();

Nit: would be prefer to see a FIXME here to track whether this should return the preferred width or the intrinsic width.

[Julien Chaffraix]

Comment on attachment 183340 [details]
Proposed implementation 4: Updated after Ojan & Tony's comments.

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

>> Source/WebCore/rendering/RenderGrid.cpp:201
>> +        ASSERT(computedBreadth != infinity);
> 
> Nit: Since it's possible to hit this ASSERT, I would remove it.  Maybe replace it with a FIXME?

Sounds good.

>> Source/WebCore/rendering/RenderGrid.cpp:205
>> +    ASSERT(trackLength.isMinContent() || trackLength.isMaxContent() || trackLength.isAuto());
> 
> Can it not be fit-content, fill-available, etc?

No, we only allow <length> | <percentage> | min-content | max-content for trackLength (see CSSParser::parseGridBreadth). We also allow 'auto' as we copy the 'auto' value to both of the minmax's argument in StyleResolver (createGridTrackMinMax).

>> Source/WebCore/rendering/RenderGrid.cpp:229
>> +        return child->minPreferredLogicalWidth();
> 
> Nit: would be prefer to see a FIXME here to track whether this should return the preferred width or the intrinsic width.

OK, I will link to the www-style archives.

>> Source/WebCore/rendering/RenderGrid.cpp:256
>> +                    continue;
> 
> This nested loop looks like it could be slow if there are a large number of tracks and children. We may need to make a map, but this is OK for now.

You are obviously right and we are not tackling scalability yet. Adding a map is on the radar as it will be required to implement the grid items' automatic placement algorithm.

[Julien Chaffraix]

Created attachment 183527 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183527 [details]
Patch for landing

Clearing flags on attachment: 183527

Committed r140198: <http://trac.webkit.org/changeset/140198>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.