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

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

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

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

Pushed by mpalmgren@mozilla.com: 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.

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

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

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

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

(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