1255393 - [css-grid] Change 'auto' track sizing to match the (updated) spec

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

The CSS Grid spec now says:
"For auto minimums: If the track has an 'auto' min track sizing function and the grid container is being sized under a min/max-content constraint, set the track’s base size to the maximum of its items’ min/max-content contributions, respectively."


This was previously implemented in bug 1176775.
Spec changes:
https://hg.csswg.org/drafts/rev/16735
https://hg.csswg.org/drafts/rev/16908

Comment 1

[Mats Palmgren (inactive)]

See also:
https://lists.w3.org/Archives/Public/www-style/2016Apr/0189.html

Comment 2

[Mats Palmgren (inactive)]

The spec now says:
"If the track has an 'auto' min track sizing function and the grid container
is being sized under a min/max-content constraint, set the track’s base size
to the maximum of its items’ min/max-content contributions, respectively.

Otherwise, set its base size to the maximum of its items’ min-size contributions:
the value specified by its respective 'min-width' or 'min-height' properties
(whichever matches the relevant axis) if the specified size is 'auto', or else
the item’s min-content contribution.

Note: For items with a specified minimum size of 'auto' (the initial value),
this is usually equivalent to a min-content minimum—but can differ in some cases,
see §6.5 Implied Minimum Size of Grid Items."

https://drafts.csswg.org/css-grid/#algo-content

The second paragraph isn't clear to me.
Does the "if the specified size is 'auto'" mean "if the specified 'width' or
'height' (whichever matches the relevant axis) is 'auto'" -or- does it refer
to the just mentioned value of the min-width/height property?
(IOW, in the latter case the whole sentence would mean "if min-width/height:auto,
then use "§6.5 Implied Minimum Size of Grid Items", otherwise use the min-content
contribution"

Comment 3

[Daniel Holbert [:dholbert]]

I agree this spec-text is confusing -- the meaning isn't clear to me, either. I'll ask Tab and/or fantasai about it tomorrow.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #2)
> Does the "if the specified size is 'auto'" mean "if the specified 'width' or
> 'height' (whichever matches the relevant axis) is 'auto'"

Tab says "yes" -- this is the correct interpretation. He also agrees this text is confusing & needs rewording.

Comment 5

[Mats Palmgren (inactive)]

Hmm, there are also these (related) changes:
https://hg.csswg.org/drafts/diff/575fb847e29d/css-grid/Overview.bs

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Introduce a SizingConstraint enum type

We need this later b/c the 'auto' min sizing for span=1 is now a tri-state:
https://drafts.csswg.org/css-grid/#algo-content
"For auto minimums ... If ... under a min/max-content constraint, ...
Otherwise, ..."

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Update MinSize() to reflect the latest spec text for min-size contribution

https://drafts.csswg.org/css-grid/#min-size-contributions
This is the "or else the item’s min-content contribution."

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Update 'auto' min track sizing for span=1 to the latest Grid spec

https://drafts.csswg.org/css-grid/#algo-content
This is the "For auto minimums ... under a min/max-content constraint" part
for span=1.  (span>1 changes comes in a later patch)

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Cache min/max-content and min-size contributions better

Since the Min/MaxContentContribution values are used in even more places,
I figured we should cache them a little better.  Also, MinSize will also
be used in a later patch (for step 2.5) so let's cache that too.

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Remove dead code related to obsolete flex track min-sizing

<flex> min-szing was removed from the spec some time ago, but I kept
the code around just in case it would be added back.  Seems unlikely
at this point, so let's remove this dead code.  (It simplifies later
patches.)

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 6 - [css-grid] Update 'auto' min track sizing for span>1 to the latest Grid spec

This adds the 'auto' min/max-content constraint part for span>1
under step 2.2 and 2.3.

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 7 - [css-grid] Update intrinsic max track sizing for span>1 to the latest Grid spec

This makes step 2.5 use min-size instead of min-content, implementing
the last hunk of this spec change:
https://lists.w3.org/Archives/Public/www-style/2016Apr/0189.html

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 8 - [css-grid] Add/tweak reftests

Note: I'm disabling grid-auto-min-sizing-definite-001.html for now
because the addition of the min-constraint parts above introduces
a small regression in how percentages are handled.
I will address that, and re-enable this test, in bug 1303643.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f8857f7110d

Comment 14

[Mats Palmgren (inactive)]

I pasted the wrong URL in comment 12, this is the correct one:
https://hg.csswg.org/drafts/diff/575fb847e29d/css-grid/Overview.bs

Comment 15

[Mats Palmgren (inactive)]

(I filed a separate bug 1305244 on removing <flex> min-sizing from the CSS parser.)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8794311 [details] [diff] [review]
part 1 - [css-grid] Introduce a SizingConstraint enum type

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

r=me

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8794312 [details] [diff] [review]
part 2 - [css-grid] Update MinSize() to reflect the latest spec text for min-size contribution

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

r=me, one nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +3673,5 @@
>    const nsStylePosition* stylePos = child->StylePosition();
> +  const nsStyleCoord& sizeStyle =
> +    axis == eAxisHorizontal ? stylePos->mWidth : stylePos->mHeight;
> +  if (sizeStyle.GetUnit() != eStyleUnit_Auto) {
> +    return MinContentContribution(aGridItem, aState, aRC, aCBWM, aAxis);

This would be a bit inscrutable if I didn't have the particular relevant chunk of spec text immediately handy.  (Your link in the commit message is great, but we should explain it in the code, too.)

Could you add some brief documentation just above this function to establish which flavor of "min size" this MinSize function is aiming to compute?  That makes this new chunk of code a bit easier to understand & anchor in something authoritative.

e.g.:
// Computes the min-size contribution for a grid item, as defined at
// https://drafts.csswg.org/css-grid/#min-size-contributions

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8794313 [details] [diff] [review]
part 3 - [css-grid] Update 'auto' min track sizing for span=1 to the latest Grid spec

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

r=me

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8794314 [details] [diff] [review]
part 4 - [css-grid] Cache min/max-content and min-size contributions better

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

r=me, with one reservation/nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +3815,5 @@
>    } else if ((sz.mState & TrackSize::eMinContentMinSizing) ||
>               (aConstraint == SizingConstraint::eMinContent &&
>                (sz.mState & TrackSize::eFlexMinSizing))) {
> +    auto s = MinContentContribution(aGridItem, aState, rc, wm, mAxis, &cache);
> +    sz.mBase = std::max(sz.mBase, s);

This patch is changing a few "nscoord" local variables to be declared with "auto", like this one here.

I'd (pretty-strongly) prefer that we keep these as "nscoord s", since I don't think the benefits of "auto" outweigh the readability costs of these particular conversions.

* Costs of using "auto" here:
 - The type of "s" is not at all clear from context here. (The variable's name, "s", doesn't give much of a hint about its type; and the type doesn't appear in any of the contextual code, either.  And while it's kind of clear it's a size, there are many size-flavored types that it could conceivably be.  So, when reading this code, someone would have to go look up the definition of MinContentContribution and/or sz.mBase if they want a hint at what type the "s" variable actually has here.)
* Benefits of using "auto" here:
 - It's shorter, but not by much
 - There are accidental-conversion problems that "auto" helps address in some cases, but I don't think those are much of a risk here.)

So, on balance, I'd rather we keep "nscoord".  (Note that a bunch of other similar chunks in this patch are keeping "nscoord" right now, too -- e.g. "nscoord s = MaxContentContribution(...)" right below this one. If we did switch to "auto", we'd probably want to make the usage consistent within a given chunk of code.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8794317 [details] [diff] [review]
part 5 - [css-grid] Remove dead code related to obsolete flex track min-sizing

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +1364,5 @@
>    {
>      MOZ_ASSERT(aAvailableSpace > 0 && aGrowableTracks.Length() > 0);
>      uint32_t numGrowable = aGrowableTracks.Length();
>      if (aSelector) {
> +      MOZ_ASSERT(aSelector == (aSelector & TrackSize::eIntrinsicMinSizing));

Two things:
 (1) Can we simplify the asserted condition to just "aSelector == TrackSize::eIntrinsicMinSizing"?  (The version in the patch is basically asserting that aSelector is either 0 or TrackSize::eIntrinsicMinSizing, I think.  But really, this is inside of an "if (aSelector)" check, so I think that means it can really only be TrackSize::eIntrinsicMinSizing here.)

 (2) Could you add a message to this assertion? It's not immediately clear to me why we expect/require it to be true.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8794320 [details] [diff] [review]
part 6 - [css-grid] Update 'auto' min track sizing for span>1 to the latest Grid spec

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

r=me

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8794322 [details] [diff] [review]
part 7 - [css-grid] Update intrinsic max track sizing for span>1 to the latest Grid spec

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

r=me

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8794327 [details] [diff] [review]
part 8 - [css-grid] Add/tweak reftests

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

::: layout/reftests/css-grid/reftest.list
@@ +55,5 @@
>  == grid-auto-min-sizing-intrinsic-001.html grid-auto-min-sizing-intrinsic-001-ref.html
>  == grid-auto-min-sizing-intrinsic-002.html grid-auto-min-sizing-intrinsic-002-ref.html
>  == grid-auto-min-sizing-intrinsic-003.html grid-auto-min-sizing-intrinsic-003-ref.html
>  == grid-auto-min-sizing-intrinsic-004.html grid-auto-min-sizing-intrinsic-004-ref.html
> +skip-if(Android) == grid-auto-min-sizing-percent-001.html grid-auto-min-sizing-percent-001-ref.html

Why is this test becoming "skip-if(Android)"? (Does it fail in some catastrophic way? or perhaps it's becoming a random-failure because of this bug's changes?)

Ideally we should have a bug filed on whatever failure is happening there, and we should mention the bug number alongside the skip annotation.  Or, if possible, it'd of course be better to avoid breaking the test on Android in the first place. :)

Comment 24

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> // Computes the min-size contribution for a grid item, as defined at
> // https://drafts.csswg.org/css-grid/#min-size-contributions

Correct.  I added that comment, thanks.

(In reply to Daniel Holbert [:dholbert] from comment #19)
> "nscoord s = MaxContentContribution(...)" right below this one. If we did
> switch to "auto", we'd probably want to make the usage consistent within a
> given chunk of code.

OK, I changed that one too to make it consistent.


(In reply to Daniel Holbert [:dholbert] from comment #20)
> >      if (aSelector) {
> > +      MOZ_ASSERT(aSelector == (aSelector & TrackSize::eIntrinsicMinSizing));
> 
> Two things:
>  (1) Can we simplify the asserted condition to just "aSelector ==
> TrackSize::eIntrinsicMinSizing"?

No, that wouldn't be the same check.  eIntrinsicMinSizing is a mask
for auto/min-content/max-content min-sizing.  So the above is checking
that we have at least one of those bits and no other bits.

>  (2) Could you add a message to this assertion? It's not immediately clear
> to me why we expect/require it to be true.

It's just an internal consistency check to make sure this code
stays somewhat in sync with the callers.  I changed it to:

MOZ_ASSERT(aSelector == (aSelector & TrackSize::eIntrinsicMinSizing) &&
           (aSelector & TrackSize::eMaxContentMinSizing),
           "Should only get here for track sizing steps 2.1 to 2.3");

(I made it stricter by also asserting that eMaxContentMinSizing is
always included.)


(In reply to Daniel Holbert [:dholbert] from comment #23)
> Why is this test becoming "skip-if(Android)"?

You can see the failures here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c6ea383c2a

It looks like some kind of rounding error to me.  Since it pass on
all Tier-1 platforms, I don't think it's worth spending time on.

> Ideally we should have a bug filed on whatever failure is happening there,
> and we should mention the bug number alongside the skip annotation.

Sure, bug 1305716.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e76de17c05
part 1 - [css-grid] Introduce a SizingConstraint enum type.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dcdf5d3d578
part 2 - [css-grid] Update MinSize() to reflect the latest spec text for min-size contribution.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/622d8de79f0c
part 3 - [css-grid] Update 'auto' min track sizing for span=1 to the latest Grid spec.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/580d291f1771
part 4 - [css-grid] Cache min/max-content and min-size contributions better.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/35d48b9feded
part 5 - [css-grid] Remove dead code related to obsolete flex track min-sizing.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/9821b801c990
part 6 - [css-grid] Update 'auto' min track sizing for span>1 to the latest Grid spec.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b923d48ea7e
part 7 - [css-grid] Update intrinsic max track sizing for span>1 to the latest Grid spec.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4785d16bb24d
part 8 - [css-grid] Add/tweak reftests.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17e76de17c05
https://hg.mozilla.org/mozilla-central/rev/7dcdf5d3d578
https://hg.mozilla.org/mozilla-central/rev/622d8de79f0c
https://hg.mozilla.org/mozilla-central/rev/580d291f1771
https://hg.mozilla.org/mozilla-central/rev/35d48b9feded
https://hg.mozilla.org/mozilla-central/rev/9821b801c990
https://hg.mozilla.org/mozilla-central/rev/2b923d48ea7e
https://hg.mozilla.org/mozilla-central/rev/4785d16bb24d