Closed Bug 1228455 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jfernandez, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: testcase)

Attachments

(2 files, 2 obsolete files)

Attached file gutters-and-flex-tracks.html (obsolete) —
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
Version: 38 Branch → Trunk
Component: Untriaged → Layout
Product: Firefox → Core
Summary: {css-grid] gutters must account for flex tracks sizing → [css-grid] gutters must account for flex tracks sizing
Attachment #8692682 - Attachment is obsolete: true
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
(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."
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.
(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.
(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)
(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)
(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
(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
Closed: 8 years ago
Resolution: --- → INVALID
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 → ---
Summary: [css-grid] gutters must account for flex tracks sizing → [css-grid] Clamp the flex factor to 1 when dividing the base size
Assignee: nobody → mats
Blocks: 1217086
OS: Unspecified → All
Hardware: Unspecified → All
Attached patch fix (obsolete) — Splinter Review
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);
Attached patch fixSplinter Review
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/c9847c40b56c
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.