1264067 - [css-grid] 'fr' min-sizing is now invalid

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

> How are you thinking of invalidating?  Forbid 'fr' units from being used in
> the 'min' position, or ban then from minmax() expressions altogether, or
> something else entirely?

Forbid them from the min position.  We already adjusted the grammar
for this yesterday. ^_^

https://lists.w3.org/Archives/Public/www-style/2016Apr/0209.html

Comment 1

[Mats Palmgren (inactive)]

https://lists.w3.org/Archives/Public/www-style/2016Apr/0351.html

  - RESOLVED: Change fr to invalid as the min in minmax() and add a
              note we'd like to add it back at some point in the
              future.

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix

I'm intentionally not removing the eFlexMinSizing flag for now, because
I don't really trust the CSSWG decisions anymore.  They might change their
mind and revert their decisions at any time (or rather, when they discover
that what they've specced is incompatible with what Chrome does).
Especially in this case, when they explicitly say so (see above).

So this just maps it to 'auto' in layout, and reject explicit minmax(Nfr,...)
in parsing, which is what the spec says "du jour".

Comment 3

[Mats Palmgren (inactive)]

Attachment: test adjustments

Comment 4

[Daniel Holbert [:dholbert]]

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

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

r=me

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8744753 [details] [diff] [review]
test adjustments

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

Two thoughts on the test patch:

::: layout/reftests/css-grid/grid-flex-min-sizing-001.html
@@ +23,5 @@
>  }
>  
> +/*
> +  NOTE Due to a spec change, 'fr' min-sizing inside minmax() is now
> +  invalid, so they were replaced with 'auto' instead.

Nit: This comment is a bit vague -- unless you're looking directly at this commit, it's unclear *where* it's saying there was a replacement.  (This can easily be misread as saying "fr" was replaced with "auto" *in the spec*, or maybe that there's some sort of dynamic replacement that happens in the parser on the fly, or something.)

So, consider clarifying with a tweak like:
  "so they were replaced" --> "so they've been replaced in this test"

(You have one other instance of this comment, in grid-flex-min-sizing-002.html -- if you do tweak this, make sure to tweak both instances.)

::: layout/style/test/property_database.js
@@ +6045,5 @@
>        "repeat(auto-fit,1fr)",
>        "repeat(auto-fit,minmax(auto,auto))",
>        "repeat(auto-fit,minmax(min-content,1fr))",
>        "repeat(auto-fit,minmax(1fr,auto))",
> +      "repeat(auto-fill,minmax(1fr,1em)) [a] minmax(0, max-content)",

This value is pretty complex -- is it possible to add a simpler invalid value for this change, to test this more directly?

E.g. would just "minmax(1fr,1em)" on its own be a previously-valid-but-now-invalid value?  If so (or if there's some other significantly-simpler value that was previously valid but is now invalid), that'd be nice to have in this list, as a more targeted/obvious test for this change.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8dbd493faa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/26d6bfd76058

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/a8dbd493faa3
https://hg.mozilla.org/mozilla-central/rev/26d6bfd76058