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

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Javier Fernandez, Assigned: mats)

Tracking

({testcase})

Trunk
mozilla52
testcase
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8692682 [details]
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
(Reporter)

Updated

3 years ago
Version: 38 Branch → Trunk
(Reporter)

Updated

3 years ago
Component: Untriaged → Layout
Product: Firefox → Core
(Reporter)

Updated

3 years ago
Summary: {css-grid] gutters must account for flex tracks sizing → [css-grid] gutters must account for flex tracks sizing
(Reporter)

Comment 1

3 years ago
Created attachment 8693769 [details]
gutters-and-flex-tracks.html
Attachment #8692682 - Attachment is obsolete: true
(Assignee)

Comment 2

3 years ago
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.
Keywords: testcase

Comment 3

3 years ago
(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."
(Assignee)

Comment 4

3 years ago
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

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
(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.
Flags: needinfo?(svillar)

Comment 7

3 years ago
(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.
Flags: needinfo?(svillar)

Comment 8

3 years ago
(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
(Assignee)

Comment 9

2 years ago
(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
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
(Assignee)

Comment 10

2 years ago
OK, so *NOW* Tab has updated the Grid spec (sigh) which makes this bug valid:
https://hg.csswg.org/drafts/rev/42427088d71f
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
(Assignee)

Updated

2 years ago
Summary: [css-grid] gutters must account for flex tracks sizing → [css-grid] Clamp the flex factor to 1 when dividing the base size
(Assignee)

Updated

2 years ago
Assignee: nobody → mats
Blocks: 1217086
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 11

2 years ago
Created attachment 8805173 [details] [diff] [review]
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.
Attachment #8805173 - Flags: review?(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);
(Assignee)

Comment 14

2 years ago
Created attachment 8805198 [details] [diff] [review]
fix

Ah, good catch about the removed std::max.
Attachment #8805173 - Attachment is obsolete: true
Attachment #8805173 - Flags: review?(dholbert)
Attachment #8805198 - Flags: review?(dholbert)
Comment on attachment 8805198 [details] [diff] [review]
fix

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

r=me
Attachment #8805198 - Flags: review?(dholbert) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9847c40b56c
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.