Two changes: 1. <flex> min-sizing is an error when inside minmax() 2. single value <flex> should compute to minmax(auto, <flex>) (Currently, we accept <flex> min-sizing but treat it as 'auto' in layout.) The relevant grammar: https://drafts.csswg.org/css-grid/#track-sizing <track-size> = <track-breadth> | minmax( <inflexible-breadth> , <track-breadth> ) | fit-content( [ <length> | <percentage> ] ) <track-breadth> = <length> | <percentage> | <flex> | min-content | max-content | auto <inflexible-breadth> = <length> | <percentage> | min-content | max-content | auto

Actually, I see that we already did this in bug 1264067, at least point 1. Hmm, it's not clear to me what the spec says about 2. Filed spec issue: https://github.com/w3c/csswg-drafts/issues/529

fantasai clarified that an explicit minmax(auto, <flex>) should serialize to <flex> too, so it doesn't really matter how we represent it internally. I'll make <flex> compute to minmax(auto, <flex>) though, since it simpler not having to deal with <flex> min-sizing at all.

David, feel free to steal the reviews here if you have time. I suspect dholbert will be busy with other Grid reviews once he's back and I'd like to get this landed before we branch.

Comment on attachment 8798585 [details] [diff] [review] part 1 - [css-grid] Serialize minmax(auto, <flex>) track sizes as <flex> Review of attachment 8798585 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSValue.cpp @@ +1283,5 @@ > "Functions must have an enumerated name."); > + // The first argument is always of nsCSSKeyword type. > + const nsCSSKeyword functionId = functionName.GetKeywordValue(); > + > + // minmax(auto, <flex>) is serialized as <flex> Maybe: s/computes to/is equivalent to (and is our internal representation of)/ Right now, the comment sounds like it's saying that the subsequent special-case code is primarily intended to do some custom simplification on author-provided "minmax(auto, <flex>)" values. But I don't think that's right. Rather, this code is aiming to recognize our expanded *internal representation* of an author-provided <flex> value, and do some special-case work to serialize back to that nice concise <flex>. Right? (It just so happens that the author *might* actually have specified a longform minmax(auto, <flex>) -- if so, we'll be serializing that as <flex>, too. But that's not the case we're primarily aiming to catch here, so the comment shouldn't really focus on that [or sound like it's focusing on that].) @@ +1286,5 @@ > + > + // minmax(auto, <flex>) is serialized as <flex> > + if (functionId == eCSSKeyword_minmax && > + array->Count() == 3 && > + array->Item(1).GetUnit() == eCSSUnit_Auto && We technically should check aProperty here, too, right? (to be sure it's one of the specific grid properties that this matters for). If some other CSS property in the future happens to accept minmax(auto, <flex>), it's conceivable that it'd have different semantics / equivalences. Alternately: if that feels like overkill, perhaps just assert that aProperty is one of the expected properties, inside of the "if" body here, to make that assumption explicit & make this more future-proof? ::: layout/style/nsComputedDOMStyle.cpp @@ +2550,5 @@ > nullptr, nsCSSProps::kGridTrackBreadthKTable); > return val.forget(); > } > > + // minmax(auto, <flex>) computes to <flex> Same here RE the comment. (computes to --> "is equivalent to & is our internal representation of")

Comment on attachment 8798586 [details] [diff] [review] part 2 - [css-grid] Represent single-value <flex> track sizes as minmax(auto, <flex>) internally Review of attachment 8798586 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSParser.cpp @@ +8765,5 @@ > + } > + if (result == CSSParseResult::Ok) { > + if (aValue.GetUnit() == eCSSUnit_FlexFraction) { > + // Single value <flex> computes to minmax(auto, <flex>). > + nsCSSValue minmax; (observation: I guess my notes about "internal representation of" aren't technically true until this change here. Still: IIUC this is the use-case you're intending to address with that serialization logic, so it makes sense for the comment to emphasize that.)

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b727c67b91a3 part 1 - [css-grid] Serialize minmax(auto, <flex>) track sizes as <flex>. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4500ce11a7 part 2 - [css-grid] Represent single-value <flex> track sizes as minmax(auto, <flex>) internally. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4628bf9912 part 3 - [css-grid] Stop handling <flex> track min-sizing in layout since they can't occur anymore. r=dholbert

(In reply to Daniel Holbert [:dholbert] from comment #8) > > + // minmax(auto, <flex>) is serialized as <flex> > (It just so happens that the author *might* actually have specified a > longform minmax(auto, <flex>) -- if so, we'll be serializing that as <flex>, > too. But that's not the case we're primarily aiming to catch here, so the > comment shouldn't really focus on that [or sound like it's focusing on > that].) I don't think this code is *primarily* amining at one other the other. It's equally serializing both to <flex>. But sure, I changed it to: + // minmax(auto, <flex>) is equivalent to (and is our internal representation + // of) <flex>, and both are serialized as <flex> > Alternately: if that feels like overkill, perhaps just assert that aProperty > is one of the expected properties, inside of the "if" body here, to make > that assumption explicit & make this more future-proof? Both seem like overkill, but sure, I added an assertion.

https://hg.mozilla.org/mozilla-central/rev/b727c67b91a3 https://hg.mozilla.org/mozilla-central/rev/1f4500ce11a7 https://hg.mozilla.org/mozilla-central/rev/ac4628bf9912