This is a followup from bug 1434130 comment 67. (In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #67) > ::: servo/components/style/values/generics/grid.rs:202 > (Diff revision 1) > > /// A `minmax` function for a range over an inflexible `<track-breadth>` > > /// and a flexible `<track-breadth>` > > /// > > /// <https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-minmax> > > + #[css(function)] > > Minmax(TrackBreadth<L>, TrackBreadth<L>), > > We should just get rid of the special-case of `Minmax` in `ToCss` and > `derive` it too. Chrome doesn't have that special-case at all.

Can you describe what this bug intends to change please? (don't bother if it's just an implementation detail with no observable effects)

I didn't have close look at the details. This is just filed as a followup for the review comment quoted, so ask emilio what in his mind can be simplified there.

I propose to get rid of this special-case for minmax(auto, <fr>), which would allow us to get rid of all that function altogether, and the manual implementation added in: https://searchfox.org/mozilla-central/rev/68fdb6cf4f40bea1a1f6c07531ebf58fb8ab060b/servo/components/style/values/generics/grid.rs#255 Does that sound reasonable mats? Preserving minmax(..) seems like I would expect as an author.

minmax(auto, <flex>) should be serialized as <flex> per the "simplest possible form" principle. See https://bugzilla.mozilla.org/show_bug.cgi?id=1305244#c2

(In reply to Mats Palmgren (:mats) from comment #4) > minmax(auto, <flex>) should be serialized as <flex> per the "simplest > possible form" principle. > See https://bugzilla.mozilla.org/show_bug.cgi?id=1305244#c2 While I do generally agree, we don't usually do that kind of transformations, specially for stuff that would change how values are interpolated for animations (see the "animatable" section in there). I don't think we should handle that special-case also in animation IMO. Coming up with a precedent that isn't calc(..) (which is preserved only in specified values generally) is hard, though.

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #5) > (In reply to Mats Palmgren (:mats) from comment #4) > > minmax(auto, <flex>) should be serialized as <flex> per the "simplest > > possible form" principle. > > See https://bugzilla.mozilla.org/show_bug.cgi?id=1305244#c2 > > While I do generally agree, we don't usually do that kind of > transformations, specially for stuff that would change how values are > interpolated for animations (see the "animatable" section in there). I don't > think we should handle that special-case also in animation IMO. in there -> in https://drafts.csswg.org/css-grid/#track-sizing

BTW, we no longer do that parse-time transformation from bug 1305244, so this should only affect people that explicitly say minmax(auto, <flex>), not just <flex>.

Mats, given the above (particularly the animation bits, but also comment 7), would that change your mind about this? I can try to convince fantasai otherwise :P

No, not really. A single-value <flex> is really just a shorter notation for minmax(auto, <flex>). Conceptually, they represent the same value. I don't get your point about animation - both <flex> and minmax(auto, <flex>) (which are indistinguishable internally) would be non-animatable components in the "simple list" of values that should be animated, as far as I understand the spec. Can you elaborate please, in case I'm missing something about how the animation should work?

(In reply to Mats Palmgren (:mats) from comment #9) > No, not really. A single-value <flex> is really just a shorter notation > for minmax(auto, <flex>). Conceptually, they represent the same value. > > I don't get your point about animation - both <flex> and minmax(auto, <flex>) > (which are indistinguishable internally) would be non-animatable components > in the "simple list" of values that should be animated, as far as I > understand > the spec. Can you elaborate please, in case I'm missing something about > how the animation should work? Err, I misread it indeed, I understood that we should interpolate components in minmax(..), but we don't, so... Also, in our impl all these are discretely animatable only. Alright, thanks (and sorry for the bother) Mats :)

No problem. Yeah, animation of these values is bug 1348519, which I guess is still kind of blocked on moving animation to stylo. Dunno how that project is going though, I still see we have StyleAnimationValue... Regarding the "components" in the spec, I think they intend that individual min/max values should animate as long as the from-value and to-value are both <length-percentage>. For example; FROM: grid-template-rows: auto minmax(auto, 50px) minmax(10px, 50px) TO: grid-template-rows: auto minmax(auto, 100px) minmax(0px, 200px) should animate as: grid-template-rows: auto minmax(auto, 50px) minmax(10px, 50px) grid-template-rows: auto minmax(auto, 51px) minmax(9px, 51px) grid-template-rows: auto minmax(auto, 52px) minmax(8px, 52px) grid-template-rows: auto minmax(auto, 53px) minmax(7px, 53px) ... grid-template-rows: auto minmax(auto, 100px) minmax(0px, 200px) (disregarding that some values should change faster to arrive at the right target value, but you get my point)

(In reply to Mats Palmgren (:mats) from comment #11) > Regarding the "components" in the spec, I think they intend that individual > min/max values should animate as long as the from-value and to-value are > both <length-percentage>. For example; > FROM: grid-template-rows: auto minmax(auto, 50px) minmax(10px, 50px) > TO: grid-template-rows: auto minmax(auto, 100px) minmax(0px, 200px) > > should animate as: > grid-template-rows: auto minmax(auto, 50px) minmax(10px, 50px) > grid-template-rows: auto minmax(auto, 51px) minmax(9px, 51px) > grid-template-rows: auto minmax(auto, 52px) minmax(8px, 52px) > grid-template-rows: auto minmax(auto, 53px) minmax(7px, 53px) > ... > grid-template-rows: auto minmax(auto, 100px) minmax(0px, 200px) > > (disregarding that some values should change faster to arrive at > the right target value, but you get my point) Right, that was my understanding... But then my point still stands, the Servo code does differentiate between minmax(auto, 10fr) and 10fr (basically, it doesn't distinguish on whether the unit is fr or other length unit). My question is whether: FROM: grid-template-rows: auto minmax(auto, 10fr) minmax(10px, 50px) TO: grid-template-rows: auto 50fr minmax(10px, 50px) Should produce a smooth animation or a discrete one. My reading of the spec is that it should be discrete, and implementing that is straight-forward. But then this special-case for serialization should probably be gone.

It's questionable if that value is animatible at all since the spec says "provided the only differences are the values of the length, percentage, or calc components", so since 10fr is different from 50fr it really shouldn't be animatible, IIUC. ('fr' definitely isn't a "length, percentage, or calc") > the Servo code does differentiate between minmax(auto, 10fr) and 10fr That seems like a good thing, since they really are the same value. IIUC, the values we'd implement animation on are the values in: https://searchfox.org/mozilla-central/rev/12af4303ffd384bc2406b67f54ba68d8264d72c8/layout/style/nsStyleStruct.h#1401-1402 (and presumably require that all the line names etc are identical) This on the other hand: FROM: grid-template-rows: minmax(auto, 10fr) 10px TO: grid-template-rows: 10fr 20px should be animatible since the first value is the same in both (and in mMinTrackSizingFunctions/mMaxTrackSizingFunctions it really is identical) Then again, I don't see any technical reasons for not animating 10fr -> 50fr so if it's easy to implement that and it behaves reasonably then we might propose a spec change for that. Likewise, if animating other things like auto -> min-content (discretely) also is easy to implement and behaves reasonably we might push for that too. But, I seem to recall there was some discussion on www-style about animating non-<length-percentage> values might be undesirable... I could be misremembering though... Maybe because it would be kind of slow since you'd have to compute item intrinsic sizes for the track sizing in each step? Excluding content-based tracks would allow you to optimize that.

> the Servo code does differentiate between minmax(auto, 10fr) and 10fr Oh, I see I misread what you said. The above definitely seems bad since those values are equivalent.