1281320 - [css-grid] Implement 'fit-content([ <length> | <percentage> ])' value for <track-size>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

This is a fairly recent spec addition:
https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-fit-content

It's still somewhat unclear exactly how it should be implemented:
https://github.com/w3c/csswg-drafts/issues/209

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

The style system part is clear in the spec, but the layout part isn't.
Implemented it as suggested in the github issue linked above.

Comment 2

[Mats Palmgren (inactive)]

Attachment: tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8140b064b656

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8765679 [details] [diff] [review]
fix

Review of attachment 8765679 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't reviewed the nsGridContainerFrame.cpp bits yet, but here are some notes on the style-system stuff:

::: layout/style/nsCSSParser.cpp
@@ +8757,1 @@
>      return CSSParseResult::NotFound;

You need to add an UngetToken() call before the return here, to put mToken back into the stream for our caller to re-parse as something else.

(In case splinter doesn't show it, this is inside the if-check for LowerCaseEqualsLiteral("minmax").  In the old code, this comparison-failure would produce an UngetToken() call, and we should make sure that it still does in the new code as well.)

If you can think of a regression test that would've caught this bug, it'd be great to add one of those, too.  (i.e. with an expression that *could* include minmax, but has something else instead; where the current patch would incorrectly slurp up a token & not put it back when checking for "minmax", and end up doing the wrong thing in the parser from that point on.)

@@ +8845,1 @@
>                return false;

It's not superficially clear why "fit-content" just produces an immediate "return false" here.  Please add a comment to explain why, between the assert and the return statement.

(Presumably, fit-content() expressions do not count as <fixed-size>?)

::: layout/style/nsComputedDOMStyle.cpp
@@ +2494,5 @@
>  {
> +  if (aMinValue.GetUnit() == eStyleUnit_None) {
> +    // A fit-content() function.
> +    RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
> +    nsAutoString argumentStr, result;

Please rename "result" to something clearer -- perhaps "fitContentFuncStr" or "fitContentExpression"?

"result" is a bit vague, and in many cases it's the name for the thing that we return (but it's not what we return here, which makes it a bit confusing).  Moreover, the data flow is kind of hard to follow here, with data being transferred from val --> argumentStr --> result --> val (full circle!), and it's harder still to follow if there's a variable that's labeled "result" that is not actually the function's result.

::: layout/style/nsRuleNode.cpp
@@ +8037,5 @@
> +      SetGridTrackBreadth(func->Item(2), aResultMax,
> +                          aStyleContext, aPresContext, aConditions);
> +    } else if (funcName == eCSSKeyword_fit_content) {
> +      // We represent fit-content(L) as 'none' min-sizing and L max-sizing.
> +      SetGridTrackBreadth(nsCSSValue(eCSSUnit_None), aResultMin,

This seems like a reasonable representation, but it should perhaps also be documented closer to where this data is actually stored, which is in nsStyleStruct.h "mMinTrackSizingFunctions", here:
  http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#1562

Maybe worth adding a comment there, explaining the representation a bit better. Maybe something like:
"When mMinTrackSizingFunctions[i] and mMaxTrackSizingFunctions[i] differ, it means there's a minmax() expression, but if mMinTrackSizingFunctions[i] is None, then it represents a fit-content() expression instead."

(The minmax part of this is somewhat self-evident, which is probably why it wasn't documented before; but now it's perhaps useful to document both the simple case as well as the new case, now that there are more possibilities.)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8765679 [details] [diff] [review]
fix

Review of attachment 8765679 [details] [diff] [review]:
-----------------------------------------------------------------

Here are my review comments on the changes to nsGridContainerFrame.cpp.  r=me with these & comment 3 addressed.

::: layout/generic/nsGridContainerFrame.cpp
@@ +183,5 @@
>               "track size data is expected to be initialized to zero");
>    auto minSizeUnit = aMinCoord.GetUnit();
> +  auto maxSizeUnit = aMaxCoord.GetUnit();
> +  if (minSizeUnit == eStyleUnit_None) {
> +    // fit-content(size) behaves as minmax(auto,max-content), clamped to 'size'.

Two nits on this comment:
 (1) It feels like a bit of a non-sequitur -- it's indirectly saying we're now dealing with a fit-content() expression, but it's not obvious that we've actually just checked for fit-content (due to the non-obvious/sneaky representation used for fit-content() in the style system, with the "None" unit in a particular spot by convention.)

 (2) (minor) The phrase "clamped to size" is ambiguous on whether it's a lower-bound or upper-bound - might as well add an extra word or two to clarify that, as long as this comment's expanding a bit.

So, consider expanding this comment to something like the following, which (I think) addresses both of my nits:
    // This track is sized using fit-content(size) (represented in style system
    // with minCoord=None,maxCoord=size).  In layout, fit-content(size) behaves
    // as minmax(auto, max-content), with 'size' as an additional upper-bound.

@@ +1382,5 @@
> +        nscoord newBase = sz.mBase + delta;
> +        if (MOZ_UNLIKELY((sz.mState & TrackSize::eFitContent) &&
> +                         aFitContentClamper)) {
> +          // Clamp mLimit to the fit-content() size, for §12.5.2 step 5/6.
> +          if (aFitContentClamper(track, sz.mBase, &newBase)) {

The phrase "Clamp mLimit" is a typo here, I think.  Pretty sure this wants to say "Clamp newBase". (That's the outparam being clamped here.)

@@ +1384,5 @@
> +                         aFitContentClamper)) {
> +          // Clamp mLimit to the fit-content() size, for §12.5.2 step 5/6.
> +          if (aFitContentClamper(track, sz.mBase, &newBase)) {
> +            didClamp = true;
> +            delta = newBase - sz.mBase;

Perhaps we should assert that delta is strictly greater than 0 here, as a sanity-check that the provided aFitContentClamper is behaving properly? (and its outparam & boolean-return-val are agreeing with each other)

::: layout/style/nsCSSParser.cpp
@@ +8757,1 @@
>      return CSSParseResult::NotFound;

You need to add an UngetToken() call before the return here, to put mToken back into the stream for our caller to re-parse as something else.

(In case splinter doesn't show it, this is inside the if-check for LowerCaseEqualsLiteral("minmax").  In the old code, this comparison-failure would produce an UngetToken() call, and we should make sure that it still does in the new code as well.)

If you can think of a regression test that would've caught this bug, it'd be great to add one of those, too.  (i.e. with an expression that *could* include minmax, but has something else instead; where the current patch would incorrectly slurp up a token & not put it back when checking for "minmax", and end up doing the wrong thing in the parser from that point on.)

@@ +8845,1 @@
>                return false;

It's not superficially clear why "fit-content" just produces an immediate "return false" here.  Please add a comment to explain why, between the assert and the return statement.

(Presumably, fit-content() expressions do not count as <fixed-size>?)

::: layout/style/nsComputedDOMStyle.cpp
@@ +2494,5 @@
>  {
> +  if (aMinValue.GetUnit() == eStyleUnit_None) {
> +    // A fit-content() function.
> +    RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
> +    nsAutoString argumentStr, result;

Please rename "result" to something clearer -- perhaps "fitContentFuncStr" or "fitContentExpression"?

"result" is a bit vague, and in many cases it's the name for the thing that we return (but it's not what we return here, which makes it a bit confusing).  Moreover, the data flow is kind of hard to follow here, with data being transferred from val --> argumentStr --> result --> val (full circle!), and it's harder still to follow if there's a variable that's labeled "result" that is not actually the function's result.

::: layout/style/nsRuleNode.cpp
@@ +8037,5 @@
> +      SetGridTrackBreadth(func->Item(2), aResultMax,
> +                          aStyleContext, aPresContext, aConditions);
> +    } else if (funcName == eCSSKeyword_fit_content) {
> +      // We represent fit-content(L) as 'none' min-sizing and L max-sizing.
> +      SetGridTrackBreadth(nsCSSValue(eCSSUnit_None), aResultMin,

This seems like a reasonable representation, but it should perhaps also be documented closer to where this data is actually stored, which is in nsStyleStruct.h "mMinTrackSizingFunctions", here:
  http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#1562

Maybe worth adding a comment there, explaining the representation a bit better. Maybe something like:
"When mMinTrackSizingFunctions[i] and mMaxTrackSizingFunctions[i] differ, it means there's a minmax() expression, but if mMinTrackSizingFunctions[i] is None, then it represents a fit-content() expression instead."

(The minmax part of this is somewhat self-evident, which is probably why it wasn't documented before; but now it's perhaps useful to document both the simple case as well as the new case, now that there are more possibilities.)

Comment 5

[Daniel Holbert [:dholbert]]

Huh, it seems Splinter duplicated my already-posted review feedback (comment 3) when it populated comment 4.

I'll just mark comment 3 as obsolete, since it's duplicated in comment 4 now. (And just be aware that the nsGridContainerFrame.cpp notes are the only thing new pieces that I added today, if you already read/addressed comment 3 locally.)

Comment 6

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> The phrase "Clamp mLimit" is a typo here, I think.

Yep, fixed.

> > +          if (aFitContentClamper(track, sz.mBase, &newBase)) {
> > +            didClamp = true;
> > +            delta = newBase - sz.mBase;
> 
> Perhaps we should assert that delta is strictly greater than 0 here?

That doesn't hold if it clamps newBase so that it equals sz.mBase, e.g.
sz.mBase = 10
newBase = 11
fit-content size = 10
It returns true b/c it did clamp 'newBase', but 'delta' is zero.
I added a "delta >= 0" assertion though, just as a sanity-check.

> ::: layout/style/nsCSSParser.cpp
> >      return CSSParseResult::NotFound;
> 
> You need to add an UngetToken() call before the return here, to put mToken
> back into the stream for our caller to re-parse as something else.

OK.

> If you can think of a regression test that would've caught this bug, it'd be
> great to add one of those, too.  (i.e. with an expression that *could*
> include minmax, but has something else instead; where the current patch
> would incorrectly slurp up a token & not put it back when checking for
> "minmax", and end up doing the wrong thing in the parser from that point on.)

I can't think of any such cases.

> It's not superficially clear why "fit-content" just produces an immediate
> "return false" here.  Please add a comment to explain why, between the
> assert and the return statement.
> 
> (Presumably, fit-content() expressions do not count as <fixed-size>?)

Yes, fit-content() is not a <fixed-size>.  I added a comment.

> ::: layout/style/nsComputedDOMStyle.cpp
> Please rename "result" to something clearer

I renamed it "fitContentStr".

> ::: layout/style/nsRuleNode.cpp
> This seems like a reasonable representation, but it should perhaps also be
> documented closer to where this data is actually stored, which is in
> nsStyleStruct.h "mMinTrackSizingFunctions", here:

OK, I documentated the fit-content() representation there too.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b723d2220298
[css-grid] Implement 'fit-content([ <length> | <percentage> ])' value for <track-size>.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a885bf13b8f
[css-grid] Reftests for fit-content() track sizes.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b723d2220298
https://hg.mozilla.org/mozilla-central/rev/8a885bf13b8f

Comment 9

[Jean-Yves Perrier [:teoli]]

SebastianZ documented it: https://developer.mozilla.org/en-US/docs/Web/CSS/fit-content
and

Comment 10

[Sebastian Zartner [:sebo]]

As Jean-Yves already wrote, I added/updated the related documentation. Mats, could you please have a quick look at the following MDN pages to verify if everything is fine:

https://developer.mozilla.org/en-US/docs/Web/CSS/fit-content
https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template-rows
https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template-columns
https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template

Thank you!

Sebastian

Comment 11

[Mats Palmgren (inactive)]

Thanks for documenting this.  I've filed a few minor issues: bug 1318876, bug 1318880, bug 1318882.

Comment 12

[Sebastian Zartner [:sebo]]

(In reply to Mats Palmgren (:mats) from comment #11)
> Thanks for documenting this.  I've filed a few minor issues: bug 1318876,
> bug 1318880, bug 1318882.

Thank you for the review! I'll have a look at them.

Sebastian