[css-flexbox][css-grid] Intrinsic sizing with ∑flex < 1 is broken

[fantasai]

The Intrinsic Main Sizes algorithm in Flexbox fails to handle cases where the sum of the flex factors is less than one. It creates a container that fits too snugly, so when the flex algorithm is run to set the sizes of the items, they are too small: essentially the multipliers are multiplied twice.

We should fix this, and for Grid, too.

[fantasai]

I think the correct fix to Flexbox is to add

The flex container’s max-content size is the largest sum of the afore-calculated sizes of all items within a single line. If the sum of the flex factors on that line is less than one, however, additionally divide this sum by the sum of the flex factors.

[tabatkins]

Here's a well-formatted testcase showing off the behavior in Flexbox and Grid: https://jsfiddle.net/Lbvz57zj/4/

Chrome's behavior:

• Grid, sum < 1: double-applies the fractions. Each items' max-content size is 200px, so when the floated grid container is sizing itself, they report 40% (80px) and 50% (100px), so the grid ends up 180px wide. Then the columns are 40% and 50% of 180px, resulting in values smaller than originally requested.
• Grid, sum >= 1: correct behavior. The items report 50% (100px) and 100% (200px), so the grid ends up 300px wide. The columns then take 1/3 and 2/3 of that size (proper re-scaled shares), resulting in the exact sizes they requested earlier.
• Flexbox, either case: The items just report their max-content size (200px each), so the flexbox is 400px wide. The items then size correctly within that size.

Firefox's behavior:

• Everything: acts the same as Chrome/Flexbox; the container is 400px wide, the items size correctly within that size.

---

Ideal behavior for both Grid and Flexbox:

• Sum < 1: Items report 40% (80px) and 50% (100px). The sum is < 1, so this sum gets rescaled to 180px / .9 = 200px, and the container ends up 200px wide. The items then claim 40% and 50% of that size, resulting in them being 80px and 100px.
• Sum > 1: Items report 50% (100px) and 100% (200px). The sum is > 1, so the container sizes itself to that exactly and ends up 300px wide. The items then claim 1/3 and 2/3 of that space, resulting in them being 100px and 200px.

[fantasai]

Actually, that fix is only correct if the flex basis is zero. We need to do something a little more complicated here.

[fantasai]

OK, new fix:

1. For each flex item, subtract its outer flex base size from its max-content contribution size. If that result is not zero, divide it by (if the result was positive) its flex grow factor floored at 1, or (if the result was negative) by its scaled flex shrink factor, having floored the flex shrink factor at 1. This is the item’s max-content flex fraction.
2. Place all flex items into lines of infinite length.
3. Within each line, find the largest max-content flex fraction among all the flex items. If the sum of the flex grow factors (flex shrink factors, if the largest max-content flex fraction was negative) on that line is less than one, divide each flex grow factor by that sum to rescale their total to one.
4. Add each item’s flex base size to the product of its rescaled flex grow factor (or rescaled scaled flex shrink factor, if the chosen max-content flex fraction was negative) and the chosen max-content flex fraction, then clamp that result by the max main size property floored by the min main size property.
5. The flex container’s max-content size is the largest sum of the afore-calculated sizes of all items within a single line.

(Yes, “rescaled scaled” sounds a bit silly.)

[tabatkins]

Okay, so example problem showing off non-zero base sizes:

Two flex items, both have a max-content size of 200px, both have flex: .4. First item has flex-basis: 100px, second has flex-basis: 300px.

Ideal outcome: First item wants to grow, second wants to shrink, so per standard flex rules, we're growing. It wants to get 40% of the way toward its ideal (max-content) width, so should land at 140px. Second item wants to grow the same amount, so it should land at 340px.

("Idealness" is defined as "gives reasonable continuous behavior as the flex values change from 0 to >=1" and vice-versa. A max-content flexbox with all inflexible items should definitely just sum the flex basises of its items, and not pay attention to their max-content size at all.)

Algo walkthrough:

1. First item's max-content flex fraction is (200px - 100px) / max(.4, 1), or 100px. (.4 from its flex-grow.) Second item's max-content flex fraction is (200px - 300px) / max(1, 1), or -100px. (1 from its flex-shrink.)
2. Both items are on the same line.
3. Largest mcff is 100px, so we use that. Sum of flex grow factors (since it's positive) is .8, so we rescale each item's flex grow factors to sum to 1 - they both become .5.
4. First item becomes 100px + (100px * .5) = 150px. Second item becomes 300px + (100px * .5) = 350px. No additional clamping for this example.
5. Flex container is thus 150px + 350px = 500px wide.

Then we actually run flex layout inside that flex container. Container is 500px wide, sum of base sizes is 400px, free space is 100px. Items thus grow. Both have a flex-grow factor of .4, so they each get 40px of space added to their base size, resulting in 140px and 340px. Success!

[tabatkins]

Just ran the above example for differing flex-grow factors as well; specifically, factors of .4 and .5. End result is that step 4 yields a size of 144.444px for the first item and 355.555px for the second, which still sums to 500px. Then layout runs, yielding a size of 140px and 350px, which is again exactly the ideal size we calculate beforehand.

[fantasai]

Wording proposal for Grid:

The max-content size of a grid container is the sum of the grid container’s track sizes (including gutters) in the appropriate axis, when the grid is sized under a max-content constraint. However, if the sum of the flex factors of the flexible tracks is less than one, then instead calculate the max-content size of the grid container with those tracks using rescaled flex factors--each flex factor divided by the sum of the flex factors--in place of their actual flex factors.

[tabatkins]

(And I've run this Grid text thru the same examples - as far as I can tell, Chrome's current behavior is exactly correct per spec, and this edit will make the sum<1 case match our ideal behavior.)

[css-meeting-bot]

The Working Group just discussed Intrinsic sizing with ∑flex < 1 is broken, and agreed to the following resolutions:

• RESOLVED: take this edit as soon as dholbert gives approval

The full IRC log of that discussion

<dael> Topic: Intrinsic sizing with ∑flex < 1 is broken
<dael> Github: https://github.com//issues/1735
<dael> fantasai: There was an error in the algo for flexing for intrinisic size. We look at flexing algo in compo with min/max sizes so we give continer enough space so when flexing algo takes place min/max are respected. The error was when flex values add up to less than one.
<dael> fantasai: TabAtkins and I proposed fixes, we think are correct. We're happy to edit them in. If anyone wants to review, great. We're pretty confident.
<dael> dbaron: Are impl signed up to make thesE? WEll, does change match existing impl?
<dael> fantasai: Current impl don't account for the flexing in calc of intrinisc. For grid we have impl that do that. THis change is fixing a behavior that doesn't seem right. I think chrome is the one that does that...I don't remember other impl.
<bkardell_> present +bkardell_
<dael> dbaron: My worry is if somebody tries to impl do websites depend on old
<dael> fantasai: There's 2 steps of impl. 1 is if you have the algo tweak it. That would be to grid where I don't think we have any dependency. I think for sure we should fix that. For flexbox the algo overall isn't impl so changing the algo won't affect impl.
<dael> fantasai: dholbert has volunteered to impl the algo, but he hasn't gotten to it.
<dael> TabAtkins: They impl a simplier version of the algo and they are consistant in that, but it's wrong and dholbert is offering to fix that.
<dael> fantasai: And that will make it more consistant with grid
<dael> dbaron: And if dholbert succeeds will other impl follow?
<dael> TabAtkins: Yes, we will
<dael> astearns: Has dholbert reviewed this proposed change?
<dael> TabAtkins: We pinged him, he hasn't yet been able to do a full review.
<dael> fantasai: It's not difficult to do, it's a tweek to some math. Largely it's a q of did we get the math right.
<TabAtkins> (Re: compat, we implement an *almost* correct version of this for Grid already, the tweak to fix it only changes grids with a sum of fr < 1.)
<dael> astearns: I'd be okay with getting it edited in, but I'm a little concerned no one has checked the math. I'd be happier if someone looked and said yay or nay
<dael> fantasai: We can resolve and say it takes effect once verified.
<dael> astearns: BUt that doesn't get us closer to pub
<dael> fantasai: It does b/c if we replies today or tomorrow we can get it into pipeline
<TabAtkins> (Our Flexbox impl is currently the "simple and dumb" version, which matches what Firefox currently does for both Grid and Flexbox; if dholbert fixes in Gecko and it sticks, we're happy to change ours to match.)
<dael> astearns: Does it make sense to publish even if this doesn't make it?
<dael> fantasai: Yeah, there's a ton of changes. THis is a CR so changes pub takes 6 weeks. Might as well start.
<dael> Chris: If it takes 6 weeks is dependant on how good DoC and changes are and availability.
<dael> fantasai: DoC is compiled.
<fantasai> s/availability/availability of Ralph and plh/
<dael> astearns: Any other comments on resolving or resolve pending dholbert?
<dael> fantasai: I'd prefer to get resolutions in place b/c that will get things rolling
<dael> Chris: I agree.
<dael> astearns: You propose to resolve we take this edit as soon as dholbert gives a thumbs up. Obj?
<dael> RESOLVED: take this edit as soon as dholbert gives approval

[fantasai]

Edits from #1735 (comment) and #1735 (comment) checked in as 5c66e72 and added to DoC. Waiting now for @dholbert’s review. :)

(Side conversation with @astearns about step 1 wording fixed over in #1803)

[dholbert]

My first nit: This section starts out:

The max-content main size of a flex container is the smallest size the flex container can take while maintaining the max-content contributions of its flex items

That doesn't seem to be accurate anymore. (And maybe it was never strictly accurate even with the old spec text, in cases of inflexible items with flex-basis values smaller than their max-content contribution)

In @tabatkins' example above (#1735 (comment) ), we end up at a max-content size of 500px, but really "the smallest size the flex container can take while maintaining the max-content contributions of its flex items" would be considerably larger than 500px. It'd be whatever size the container would need to be so that the first item end up at 200px.

I think the issue is that "maintaining the max-content contributions of its flex items" is not strictly the goal here. Rather, it's something more like "maintaining the max-content contributions of its flex items, as much as their flex factors will allow, in light of our desire for continuity with 0".

So that first sentence probably needs to get either more specific or more vague, so that it's not stating something that's strictly false?

[tabatkins]

Yes, that's a more accurate statement. Now to get it shortened to something pithy. ^_^

[dholbert]

My second nit: the math/units don't work out when we're using scaled flex shrink factor. Example below to illustrate this.

Scenario: Just one flex item (for simplicity) with max-content size of 200px, and flex: 1 1 400px.

Ideal outcome (I think?): flex container should end up exactly 200px wide (forcing item to shrink to its max-content size).

Walkthrough of what the current draft's algorithm would do:

1. compute mcff: first, do 200px-400px = -200px, which is negative so we divide by scaled flex shrink factor, so we get -200px / 400px = -0.5 (which happens to be a unitless value, FWIW, since the px cancel out in the division)
2. The flex item goes on its own line.
3. Largest mcff is the only one, which is -0.5. It's negative, so we're using flex-shrink. Sum of flex-shrink factors is exactly 1, so there's no factor-rescaling needed.
4. "Add each item's flex base size to the product of its... rescaled flex shrink factor ... and the chosen mcff". So, we do 400px + (1 * -0.5) (with the latter term having no units, notably)... So we get 399.5px (if I disregard the unit mismatch and just do the math)
5. The flex container ends up being that size, 399.5px, which makes no sense.

I think some piece of this algorithm wants to use the flex shrink factor instead of the *scaled* flex shrink factor, or vice versa, or needs to un-scale a computation somehow...

[dholbert]

My third nit (on wording that might get changed in addressing second nit): the algorithm uses the term "rescaled scaled flex shrink factor*" without clearly explaining what that is.

These parts are clear IMO:

• "scaled flex shrink factor" (clearly defined elsewhere as flex-shrink * flex base size)
• "rescaled flex shrink factor" (defined implicitly in step 3, as flex shrink factor rescaled so that their sum is at least 1)

... but "rescaled scaled flex shrink factor" is never defined. I assume it probably would be "rescaled flex shrink factor" times the flex base size (i.e. the scaled flex shrink factor, calculated using the rescaled flex shrink factor instead of the real one)? But this isn't super clear.

[fantasai]

Proposed changes to address @dholbert‘s last two comments:

diff --git a/css-flexbox/Overview.bs b/css-flexbox/Overview.bs
index afcb32f..8a8edfc 100644
--- a/css-flexbox/Overview.bs
+++ b/css-flexbox/Overview.bs
@@ -2797,13 +2797,15 @@ Flex Container Intrinsic Main Sizes</h4>
 
                <li>
                        For each <a>flex item</a>,
-                       subtract its outer <a>flex base size</a> from its [[#intrinsic-item-contributions|max-content contribution]] size.
+                       find its <var>max-content flex fraction<var>:
+                       Subtract its outer <a>flex base size</a> from its [[#intrinsic-item-contributions|max-content contribution]] size.
                        If that result is positive,
                        divide by its <a>flex grow factor</a> floored at 1;
                        if negative,
-                       by its <a>scaled flex shrink factor</a>
-                       having floored the <a>flex shrink factor</a> at 1.
-                       This is the item's <var>max-content flex fraction</var>.
+                       by its <a>flex shrink factor</a> floored at 1
+                       multipled by its <var>flex base ratio</var>--
+                       the ratio of its <a>flex base size</a>
+                       to the sum of the <a>flex base sizes</a> of all shrinkable items on the line.
 
                <li>
                        Within each line,
@@ -2819,7 +2821,8 @@ Flex Container Intrinsic Main Sizes</h4>
                <li>
                        Add each item’s <a>flex base size</a>
                        to the product of its <em>rescaled</em> <a>flex grow factor</a>
-                       (or <em>rescaled</em> <a>scaled flex shrink factor</a>,
+                       (or its <em>rescaled</em> <a>flex shrink factor</a>
+                       multiplied by its <var>flex base ratio</a>,
                        if the chosen <var>max-content flex fraction</var> was negative)
                        and the chosen <var>max-content flex fraction</var>,
                        then clamp that result by the <a>max main size property</a>

This change switches us back to using flex shrink factors in place of scaled shrink factors, and introduces the concept of flex base ratio as a multiplier instead.

[fantasai]

OK, so I checked that in. @dholbert @tabatkins Could you review so we can tell @svgeesus to republish asap? :)
1ddd303

[dholbert]

One more issue I'm noticing: suppose we consider Tab's example above #1735 (comment) and we make one tweak -- we add margin-left: 50px to the first flex item. Ideally, this should make the algorithm produce a flex container that is 50px larger (so, 550px). But it doesn't -- it remains at 500px. This is because right now, the margin only comes into play in step 2, and it exactly cancels out so that it doesn't have any effect. ("For each flex item, find its max-content flex fraction: Subtract its outer flex base size from its max-content contribution size" -- my margin tweak makes each of these operands 50px larger, so the margin has no net effect. The mcff still ends up 100px, and the rest of the algorithm proceeds just as Tab outlined and produces 500px.)

Possible solution: in step 4...

Add each item’s flex base size to the product of [...]

...maybe that mention of "flex base size" should really be "outer flex base size"?

[dholbert]

Possible problem (not sure): the current algorithm is discontinuous when a flex factor reaches 0.

Consider a simple scenario with just a single flex item, with flex-basis: 100px and max-content size of 200px (like the first item in Tab's example above). Right now, this intrinsic sizing algorithm will give the flex container an intrinsic size of...

• 100px, if the item's flex-grow is exactly zero.[1]
• 200px, if the item's flex-grow is anything nonzero.

This distinction makes some sense to me -- if the flex item isn't flexible, then clearly it's not going to be able to grow at all beyond its 100px flex-basis (if we assume there's no min-width interfering with things). So: it's reasonable to say that the flex container doesn't need to be any larger than 100px in this case -- so that makes some sense as an intrinsic size.

And on the other hand, for all nonzero flexibilities, it seem reasonable to always size the container to the (sole) item's full max-content size (as the "target size") and then let the flex layout algorithm size the item to some intermediate growth point towards that target, depending on whether the flexibility is fractional or not. (This is basically what would happen with the algorithm as-written now.)

Still: despite this apparent reasonableness, it's unfortunate that this creates a discontinuity. Maybe we should try to set up the algorithm so that its behavior for inflexible items is equal to the limit of its behavior for flexible items with fractional flexibilities? (I think that was part of the motivation here, right?) Or would that cause other problems?

[1] Just in case it's not clear why the algorithm produces 100px in the "flex-grow is exactly zero" case: the easiest way to see this is to just jump straight to step 5, which computes flexBaseSize + rescaledFlexGrow * [something]. The second term of that addition is clearly 0, because flex-grow is 0 (and we don't rescale it away from 0), so we're left with just the flex base size, which in this case is 100px.

[fantasai]

Still: despite this apparent reasonableness, it's unfortunate that this creates a discontinuity.

Indeed, having a discontinuity is not ideal. :(

Maybe we should try to set up the algorithm so that its behavior for inflexible items is equal to the limit of its behavior for flexible items with fractional flexibilities? (I think that was part of the motivation here, right?) Or would that cause other problems?

The problem here is that it would be really confusing if inflexible items create space in the container that they don't consume.

I guess our options are to revert the fix here, or to accept the discontinuity in flex container sizing? Or is there another option?

[tabatkins]

Okay, the discontinuity is definitely bad; I can't accept an algorithm that has it. This sucks. :(

So yeah, if we avoid a discontinuity, there's only two options:

1. Revert the fix, so that we're back to the "sum<1 double-applies the fraction in a non-intuitive way" behavior. This sucks, but it's perfectly continuous.
2. Pass around some extra state somehow, and tweak the layout algorithm such that the flexbox is sized to exactly the fraction that the items want to grow to, and they then fill the flexbox exactly. This also totally sucks, because it breaks the feature that you can always read out the post-layout size and then set it explicitly to get the same behavior.

I think the drawback of #2 is probably much worse than the drawback of #1; slightly non-intuitive sizing in a minority case vs breaking a pretty universal and useful layout guarantee has a pretty clear winner, I think.

So, we should revert, and then double-check that the original text does indeed do what we (now) want, and has no other unintended consequences.

[tabatkins]

I've reverted to the previous text; on review, it does seem to do what we now want to do. Confirmation of that would be appreciated!

I carried over the modification to the opening paragraph, and added an explanatory note to the end of the section.

[css-meeting-bot]

The Working Group just discussed Flex sum < 1, and agreed to the following resolutions:

• RESOLVED: Revert the previous resolution regarding flex sum <1 (as has already been done) and solicit dholbert's review.

The full IRC log of that discussion

<TabAtkins> Topic: Flex sum < 1
<TabAtkins> GitHub: https://github.com//issues/1735#issuecomment-332671797
<TabAtkins> astearns: My understanding was that we had a resolution, but we had to revert it because there was a discontinutiy.
<TabAtkins> fantasai: Yes, we need to be continuous for a given flex item going from 0=>1, *and* for the flex container when its children do the same.
<TabAtkins> fantasai: Previous resolution fixed the flex item, but it created a discontinuity for the flex container.
<TabAtkins> fantasai: So our proposal ends up giving non-linear behavior for flex items when the sum is <1, but it's continuous.
<TabAtkins> astearns: Has dholbert looked at the revert?
<TabAtkins> TabAtkins: He hasn't commented yet, so if he's seen it he hasn't approved it.
<TabAtkins> astearns: So should we wait until we get feedback?
<TabAtkins> TabAtkins: I'd prefer to get someone invested to give positive feedback.
<TabAtkins> rego: I think we should accept the proposal.
<TabAtkins> astearns: Have you reviewed the reverted text.
<TabAtkins> rego: No, haven't checked the new text specifically, but I want continuity.
<TabAtkins> astearns: fantasai, what do you want to do?
<TabAtkins> fantasai: Resolve to revert to the previous text unless dholbert has opposite ideas.
<TabAtkins> astearns: Any objections to reverting?
<TabAtkins> RESOLVED: Revert the previous resolution regarding flex sum <1 (as has already been done) and solicit dholbert's review.

[dholbert]

I'll run through the current (reverted) text with some scenarios tomorrow, and I'll post any issues that I find.

[dholbert]

The current text on https://drafts.csswg.org/css-flexbox-1/#intrinsic-main-sizes looks good to me!

A few nits on the "Implications of this algorithm when the sum of flex is less than 1" informational section that comes right after the algorithm:

1. It mentions "the flexbox" several times -- that should be clarified to "the flex container" for consistency with the rest of the spec (which never uses (or defines) the term "flexbox", except as the general feature name)
2. "When all items are flexible with flexes > 1" -- that should say >= (greater-than-or-equals). Flexes greater than 1 aren't qualitatively different from flexes equal to one.
3. That section has two mentions of "max content contribution", which (a) need a hyphen, and (b) should probably be linkified to its definition.

[mrego]

The relevant text for Grid was reverted and the result of the final text looks good to me.

[tabatkins]

All right, nits fixed. Thanks, y'all!