1228455 - [css-grid] Clamp the flex factor to 1 when dividing the base size

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Javier Fernandez]

Attachment: gutters-and-flex-tracks.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Iceweasel/38.2.1
Build ID: 20150828090403

Steps to reproduce:

load the attached html test case.


Actual results:

grid container's height is 50px


Expected results:

grid container's height might be 63px

Comment 1

[Javier Fernandez]

Attachment: gutters-and-flex-tracks.html

Comment 2

[Mats Palmgren (inactive)]

https://drafts.csswg.org/css-grid/#algo-flex-tracks
Is the "available space" there really that of the container of the grid?
We're using the grid container itself, which in this case is unconstrained.

BTW, you can see the correct result in Nightly if you put 'height:60px' on
the grid container in your test.  The result should be:
first row: 10px  <-- because its flex size is less than its base so it's treated
row-gap: 33px        as non-flexible by the algorithm*.
second row: 17px

[*] see bullet 4 here: https://drafts.csswg.org/css-grid/#algo-find-fr-size

Anyway, it all depends on what we should use for the "available space".
If we use the containing block of the grid container then the tracks
would overflow the grid container for something like this:
<div style="width: 10px; height: 60px;">
    <div class="grid" style="height:50px;">
which seems weird to me.

FWIW, currently we're using the computed height of the grid container,
which for the attached test is indefinite and thus we use that part
of the algorithm.

Comment 3

[Sergio Villar]

(In reply to Mats Palmgren (:mats) from comment #2)
> https://drafts.csswg.org/css-grid/#algo-flex-tracks
> Is the "available space" there really that of the container of the grid?
> We're using the grid container itself, which in this case is unconstrained.

This is what CSS sizing says about that:

"The space into which a box is laid out. Unless otherwise specified, this is either a measurement of its containing block (if that is definite) or an infinite size (when it is indefinite). An available size can alternatively be either a min-content constraint or a max-content constraint."

Comment 4

[Mats Palmgren (inactive)]

Yes, and I think that supports my claim that the available height
in this case is indefinite (height is 'auto' on the grid container).
I don't see any support for looking further up the ancestor chain.

Percentage track sizes *are* resolved using the grid container:
https://drafts.csswg.org/css-grid/#track-sizing
"Each track sizing function can be specified as a length,
a percentage of the grid container’s size, ..."

I don't see any spec support for why 'fr' sizes should be different,
or why either should use an ancestor for calculating '%' or 'fr' in
some cases.

Comment 5

[Sergio Villar]

(In reply to Mats Palmgren (:mats) from comment #4)
> Yes, and I think that supports my claim that the available height
> in this case is indefinite (height is 'auto' on the grid container).
> I don't see any support for looking further up the ancestor chain.

If the height is auto then definitely is indefinite, we do the same thing. I was just adding it for reference.

> Percentage track sizes *are* resolved using the grid container:
> https://drafts.csswg.org/css-grid/#track-sizing
> "Each track sizing function can be specified as a length,
> a percentage of the grid container’s size, ..."
> 
> I don't see any spec support for why 'fr' sizes should be different,
> or why either should use an ancestor for calculating '%' or 'fr' in
> some cases.

You need to do that in some cases for example if you have a chain of percentages or "auto"s (the latter only for widths) you need to check your ancestors in order to know whether that size is definite or not.

Comment 6

[Mats Palmgren (inactive)]

(In reply to Sergio Villar from comment #5)
> You need to do that in some cases for example if you have a chain of
> percentages or "auto"s (the latter only for widths) you need to check your
> ancestors in order to know whether that size is definite or not.

Do you have a spec reference that supports that behavior?
It sounds like some old Quirks Mode behavior to me.

Comment 7

[Sergio Villar]

(In reply to Mats Palmgren (:mats) from comment #6)
> (In reply to Sergio Villar from comment #5)
> > You need to do that in some cases for example if you have a chain of
> > percentages or "auto"s (the latter only for widths) you need to check your
> > ancestors in order to know whether that size is definite or not.
> 
> Do you have a spec reference that supports that behavior?
> It sounds like some old Quirks Mode behavior to me.

Well that's our interpretation of the definition of "definite size" from https://drafts.csswg.org/css-sizing/#definite

definite size
A size that can be determined without measuring content; that is, a <length>, a size of the initial containing block, or a <percentage> or other formula that is resolved solely against definite sizes. Additionally, the size of the containing block of an absolutely positioned element is always definite with respect to that element.

Comment 8

[Sergio Villar]

(In reply to Mats Palmgren (:mats) from comment #6)
> (In reply to Sergio Villar from comment #5)
> > You need to do that in some cases for example if you have a chain of
> > percentages or "auto"s (the latter only for widths) you need to check your
> > ancestors in order to know whether that size is definite or not.
> 
> Do you have a spec reference that supports that behavior?
> It sounds like some old Quirks Mode behavior to me.

See also this email from Tab: https://lists.w3.org/Archives/Public/www-style/2014Oct/0315.html

Comment 9

[Mats Palmgren (inactive)]

(In reply to Sergio Villar from comment #8)
> See also this email from Tab:
> https://lists.w3.org/Archives/Public/www-style/2014Oct/0315.html

I agree with what Tab says there, but you're making a mistake to assume that
what he says about "width:auto ... filling its containing block" also applies
in the block-axis.  It does not.

I'm pretty sure we're are doing this correctly per spec, so I've filed a Chrome bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=654904

Comment 10

[Mats Palmgren (inactive)]

OK, so *NOW* Tab has updated the Grid spec (sigh) which makes this bug valid:
https://hg.csswg.org/drafts/rev/42427088d71f

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix

Don't blame me, this stupid behavior is what the spec author asks for.
It will lead to track sizes not having the proportions the author
asked for when using a flex factor < 1 with unconstrained available
space.  Not just in hairy complicated edge cases, but always.

Comment 12

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48e5dda1e9a6

Comment 13

[Daniel Holbert [:dholbert]]

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

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +4671,5 @@
>    }
>  
>    // The used flex fraction is the maximum of:
> +  // ... each flexible track's base size divided by its flex factor if the flex
> +  // factor is greater than one, or its base size otherwise.

This phrasing is a bit easy to misread -- specifically this part:
 ...divided by its flex factor [...], or its base size...

This sounds like it's saying "divided by...its base size" for some cases (but that's not what you want it to be saying).

Perhaps replace with "divided by its flex factor (which is floored at 1)"?

@@ +4680,3 @@
>        fr = std::max(fr, mSizes[track].mBase / flexFactor);
> +    } else {
> +      fr = mSizes[track].mBase;

Don't we still need the "max(fr, ...)" in this new case, too?  (since we're supposed to take the max of this thing across all the tracks)

Given that, to avoid duplicating code, it seems like it might be simpler to scope the 1.0 check to *just* the place where we maybe do the mBase division, and then do the fr update unconditionally (with std::max), like so:

 float possiblyDividedBaseSize = (flexFactor > 1.0f)
   ? mSizes[track].mBase / flexFactor)
   : mSizes[track].mBase;

  fr = std::max(fr, possiblyDividedBaseSize);

Comment 14

[Mats Palmgren (inactive)]

Attachment: fix

Ah, good catch about the removed std::max.

Comment 15

[Daniel Holbert [:dholbert]]

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

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

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9847c40b56c
[css-grid] Update the 'used flex fraction' for indefinite sizes to clamp flex factors less than 1 before dividing the base size (due to CSS Grid spec change). r=dholbert

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9847c40b56c