[css-flexbox-1] Intrinsic Main Size algo has errors

[tabatkins]

Reviewing testcases in web-platform-tests/wpt#33242, @fantasai and I found the handling of flex factors <1 is not quite right in the intrinsic main size algo.

Quoting from the issue:

we forgot about the flooring that occurs for min-content contribution when the item is inflexible. [...] This is clearly wrong. It's obviously discontinuous; a flex shrink of .001 would have completely different behavior (essentially what we wrote, modulo the tiny numeric difference). So, that "floor inflexible items by their flex basis" is obviously a spec bug.

The principle of the min-content flex container size algorithm is twofold:

• pick the smallest size that would not violate the min-content contributions of the items once flex layout is run on their contents
• keep behavior continuous across all inputs

In the flex layout algorithm, we had to make some adjustments when the sum of the flex factors is less than one to get the continuity between zero and one, and in those cases the flex container and its items do not fit each other exactly.

It looks like in the intrinsic size algorithm, we're handling individual flex factors less than one specially, where what we should be doing is handling their sum less than one specially. (That is, two items that both have a .5 flex factor should be treated identically to both having 1, but if they're both .25 they'll be somewhat different, ideally halfway between the 1 case and the 0 (fully inflexible) case.)

The cases to consider are the example you show with the following pairs of flex factors:

• 1,1 and 0.5,0.5 should behave the same here as they are the same during flex layout
• 0,0 needs to exactly fit two items that do not shrink at all
• We also looked at 0.75,1; 0.5,1; 0.25,1; 0,1; 0,0.5; 0.25,0.25; 0.75,0.25 to evaluate continuity, focusing on 0,0.5 and 0.25,0.25.

We think the spec needs some edits, to more closely align the behavior of the intrinsic sizing algo and the normal flex layout algo.

So if we set aside the min-content contribution problem (clearly a mistake on our part), then the current behavior is, in general:

1. For each flex item, find what fraction size would cause the item to exactly flex from its flex-basis to its min-content size (or max-content, for the other case).
2. Among those fractions, choose the one that, if used for everyone, gets at least one item to its min-content size and doesn't make anything smaller than its min-content size.
3. Multiply each item's flexibility by the chosen flex fraction, add it to their flex basis, sum all those to find the flex container's size.
4. Run normal flex layout into that container size.

This is a good and straightforward inversion of the flex layout algorithm so far, to go from item sizes -> container size rather than the reverse. It breaks continuity, tho, and so this is restored by, in the current spec, flooring the flex factor in step 1, then applying it normally in step 3; essentially it figures out what flex fraction it would "ideally" like to have, then only gets a fraction of that.

The problem is that this continuity adjustment is not an inversion of the flex layout algo's own continuity adjustment, which does much the same but only if the sum of flex factors is less than 1; as long as there are other flexible items, a single item can transition its flexibility to 0 and behave according to the naive algorithm, which is good. The currently specced continuity adjustment does achieve continuous behavior, but it causes us to produce larger min-content container sizes than it needs to.

Instead, we suggest matching the flex layout algo's continuity adjustment: so long as the sum of flex factors is >=1, then just use the naive distribution algo. If the sum is <1, then use the naive algo initially (steps 1 and 2, above, when determining the flex fractions and choosing one as the winner), then multiply that flex fraction by the sum before doing step 3.

This behavior is identical to the currently specced behavior if all the items individually have flex >= 1, or if there's at most a single flexible item. The behavior is different (and better) in other cases, when at least one item has a flex < 1 but other items are also flexible (with any flex value); the current spec text can produce a larger min-content size than the suggested change, with none of the items reaching their min-content size at all.

As a specific example, using the same markup you've already presented (2 flex items, with flex-basises of 200px and 400px, and both with a min-content size of 100px):

• flexes of .5 and 1:
  • current spec floors the first flex to 1 while calculating fraction sizes, giving desired fraction sizes of -100px and -150px (basically; the actual text calculates a different number but with those ratios). We choose the -100px and apply it with the unfloored flexes, shrinking the first item to 150px and the second to 200px, for a container size of 350px. (Then running flex layout with a container size of 350px gives those same item sizes back; note that neither item has hit its min-content size.)
  • proposed spec treats them naively, giving desired fraction sizes of -200px and -150px. We choose the -150px and apply it with the scaled flexes, shrinking the first item to 125px and the second to 100px, for a container size of 225px. (Again, running flex layout with a container size of 225px gives those item sizes back, but this time one of the items does hit its min-content size.)
• flexes of .25, .25:
  • current spec floors both flexes to 1. giving the same fraction sizes as before. We again choose the -100px and apply it using unfloored flexes, shrinking the first to 175px and the second to 350px, for a container size of 525px. Running flex layout does not reproduce those item sizes, but that's expected: the sum of flexes is <1, so they don't shrink to the extent they'd "want" to and they overflow the container a little bit.
  • proposed spec treats them naively, giving desired fraction sizes of -400px and -600px, and we select the -400px. Since the sum is only .5, we multiply the fraction by that, getting -200px. Distributing it then shrinks the first item to 150px and the second to 300px, for a container size of 450px. Neither item hits its min-content size, but one of them gets "halfway" there (shrinking from 200px to 150px, rather than all the way to 100px), which is appropriate/expected since the sum of the flex factors is .5. Again, flex layout will give the items slightly larger sizes than this so they overflow, but that's expected.

---

Just as a note: the min-content contribution discontinuity error was added in 2c446be, from the conclusions in https://lists.w3.org/Archives/Public/www-style/2015Aug/0212.html. At the time, this wasn't an error, as the intrinsic sizing algo didn't have the flooring behavior for flex factors < 1.

[tabatkins]

Proposed changes:

diff --git a/css-flexbox-1/Overview.bs b/css-flexbox-1/Overview.bs
index 358740409..c5d628f65 100644
--- a/css-flexbox-1/Overview.bs
+++ b/css-flexbox-1/Overview.bs
@@ -2903,38 +2903,52 @@ Flex Container Intrinsic Main Sizes</h4>
 			For each <a>flex item</a>,
 			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;
+			divide by its <a>flex grow factor</a>;
 			if negative,
-			divide 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>.
+			divide by its <a>scaled flex shrink factor</a>.
+			This is the item's <var>desired flex fraction</var>.
 
 		<li>
 			Place all <a>flex items</a> into lines of infinite length.
-
-		<li>
 			Within each line,
-			find the largest <var>max-content flex fraction</var>
+			find the greatest (most positive)
+			<var>desired flex fraction</var>
 			among all the <a>flex items</a>.
+			This is the line’s <var>chosen flex fraction</var>.
+
+		<li>
+			If the sum of the line’s <a>flex grow factors</a>
+			(<a>flex shrink factors</a>,
+			if the <var>chosen flex fraction</var> is negative)
+			is less than 1,
+			multiply the <var>chosen flex fraction</var> by that sum.
+
+		<li>
 			Add each item’s <a>flex base size</a>
 			to the product of its <a>flex grow factor</a>
-			(or <a>scaled flex shrink factor</a>, if the chosen <var>max-content flex fraction</var> was negative)
-			and the chosen <var>max-content flex fraction</var>,
+			(<a>scaled flex shrink factor</a>, if shrinking)
+			and the <var>chosen flex fraction</var>,
 			then clamp that result by the <a>max main size</a>
 			floored by the <a>min main size</a>.
 
 		<li>
-			The <a>flex container</a>’s <a>max-content size</a> is the
-			largest sum of the afore-calculated sizes of all items within a single line.
+			The <a>flex container</a>’s <a>max-content size</a> is
+			the largest sum (among all the lines)
+			of the afore-calculated sizes of all items within a single line.
 	</ol>
 
 	The <strong><a>min-content</a> <a>main size</a></strong> of a <em><a>single-line</a></em> flex container
 	is calculated identically to the <a>max-content</a> <a>main size</a>,
 	except that the <a>flex items</a>’ [[#intrinsic-item-contributions|min-content contributions]] are used
 	instead of their [[#intrinsic-item-contributions|max-content contributions]].
+
 	However, for a <em><a>multi-line</a></em> container,
-	it is simply the largest [[#intrinsic-item-contributions|min-content contribution]]
+	the [=min-content=] [=main size=] is simply the largest [[#intrinsic-item-contributions|min-content contribution]]
 	of all the non-[=collapsed=] <a>flex items</a> in the <a>flex container</a>.
+	For this purpose,
+	each item's contribution
+	is capped by the item’s [=flex base size=] if the item is not growable,
+	and floored by the item’s [=flex base size=] if the item is not shrinkable.
 
 	<details class=note>
 		<summary>Implications of this algorithm when the sum of flex is less than 1</summary>
@@ -3003,17 +3017,13 @@ Flex Item Intrinsic Size Contributions</h4>
 	is the larger of its <em>outer</em> <a>min-content size</a>
 	and outer <a>preferred size</a> (its 'width'/'height' as appropriate)
 	if that is not ''width/auto'',
-	clamped by its <a>flex base size</a> as a maximum (if it is not growable)
-	and/or as a minimum (if it is not shrinkable),
-	and then further clamped by its <a lt="min main size">min</a>/<a>max main size</a>.
+	clamped by its <a lt="min main size">min</a>/<a>max main size</a>.
 
 	The <strong>main-size <a>max-content contribution</a> of a <a>flex item</a></strong>
 	is the larger of its <em>outer</em> <a>max-content size</a>
 	and outer <a>preferred size</a> (its 'width'/'height' as appropriate)
 	if that is not ''width/auto'',
-	clamped by its <a>flex base size</a> as a maximum (if it is not growable)
-	and/or as a minimum (if it is not shrinkable),
-	and then further clamped by its <a lt="min main size">min</a>/<a>max main size</a>.
+	clamped by its <a lt="min main size">min</a>/<a>max main size</a>.

[tabatkins]

Explanation of proposed changes:

1. Removed the "capped by flex base size if not growable, floored by flex base size if not shrinkable" from the definition of min-content contribution, and narrowly restored it just to the multi-line calculation (where no actual flex calculations are done at all, so a bald "flexible or not" test is adequate).
2. Removed the per-item flooring of flex factors, and replaced it with a per-line scaling of the flex fraction, same as the flex layout algorithm.
3. Made some editorial edits for better clarity.

/cc @dholbert @davidsgrogan for review

[davidsgrogan]

Thanks for working on this.

Two things:

1. I think min/max main sizes need to be reapplied after multi-line containers cap/floor the contribution by flex base size for non-growable/shrinkable items.

2. This change removing the floors introduces divide-by-zero operations, which are undefined, when grow/shrink factors are 0.

@@ -2903,38 +2903,52 @@ Flex Container Intrinsic Main Sizes</h4>
 			For each <a>flex item</a>,
 			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;
+			divide by its <a>flex grow factor</a>;
 			if negative,
-			divide 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>.
+			divide by its <a>scaled flex shrink factor</a>.
+			This is the item's <var>desired flex fraction</var>.

[fantasai]

OK, fixed (in cda2822, b545fcd, and e3d1a41)! Agenda+ for WG review.

[davidsgrogan]

I think the new text gives this flexbox an infinite min and max intrinsic size. Is that intended?

<div style="display: flex;">
  <!-- contribution = 500 -->
  <!-- fraction = inf -->
  <div style="flex: 0 1 200px; width: 500px;"></div>
  <!-- contribution = 200 -->
  <!-- fraction = 50 -->
  <!-- flex base size + largest fraction * flex grow factor = inf -->
  <div style="flex: 2 0 100px; width: 200px;"></div>
</div>

[davidsgrogan]

There's another technical issue around prescribing flex factor * chosen flex fraction.
flex factor can be 0 and chosen flex fraction can be inf.
0 * inf is undefined.

This happens when calculating min-content size in http://wpt.live/css/css-flexbox/intrinsic-size/row-001.html. Well, it's actually 0 * -inf in that test, but same problem.

I think you intend for the result of that multiplication to be 0 here.

[tabatkins]

Argh, all our test cases were looking at the shrinking case, where the "goes to infinity" behavior is fine because it's negative infinity and gets discarded, since the other value is more positive. We didn't even look at the growth case where the infinite behavior is the one that gets chosen.

Looking into this...

[tabatkins]

Okay, so there's no way to rescue continuity here with the existing behavior we've put in the spec. Given two flex items, each with a flex-basis: 0; and 100px of min-content contribution:

• flex-grows of .1 and .9 give desired fractions of 1000px and ~111px, so we'll choose 1000px and get final sizes of 100px and 900px
• flex-grows of .01 and .99 give desired fractions of 1e4px and ~101px, so we choose 1e4 and get final sizes of 100px and 9900px
• etc, resulting in a limit behavior at (0, 1) of the first item being 100px and the second item being infinity px, even tho the clearly desired behavior there is the first item being frozen (staying at its flex-basis of 0px) and the second getting to grow to 100px.

The only way to rescue continuity is indeed to care about individual values <1; multiplying the (flex base size - contribution size) by the fraction first. (Or equivalently, floor the factor at 1 when calculating the desired flex fraction, which is what the original spec text did.)

Since this would return us to the original spec text, I suspect that's why we wrote it that way originally; we must have been looking at growing cases rather than shrinking cases.

So there are two options:

1. Return to the original spec text, which correctly handles growth but doesn't shrink as much as it could.
2. Use the individual-flooring behavior for growth, and the sum-flooring behavior for shrink.

The issue, ultimately, is that the same goal - make at least one element reach its contribution size, and every other element be its contribution size or larger, given a single consistent fraction size - is produced asymmetrically for grow and shrink. In grow, you have to select the largest fraction to satisfy the goal, so the elements with a smaller desired fraction overshoot their contribution size; in shrink you have to select the smallest fraction, so the elements with a larger desired fraction stop before they get smaller than their contribution size. Both cases get an infinite blow-up for zeros and near-zeros, but that gets discarded in the shrink case but honored in the grow case. It might very well make sense to have these two cases require slightly different algos, then.

[davidsgrogan]

It might very well make sense to have these two cases require slightly different algos, then.

I'm good with implementing this if you're good with speccing it.

[davidsgrogan]

spec editors, I'd love it if you could work on this soon!

(you're not waiting for me for anything, are you?)

[fantasai]

So @tabatkins and I worked out some edits for this issue. We tweaked the growth algorithm a bit to make the continuity curve less extreme (by the time we do layout, we've triple-applied the flex factor without the fix) by dividing the chosen flex fraction by the sum of the growth factors when that sum is < 1.

Principles

1. Don't explode any sizes, whether growing or shrinking, as inputs approach zero.
2. When flex factors are all >=1, return the minimum size necessary for every item to be >= max-content size.
3. Items with a zero flex shouldn't affect the sizes at all.
4. Keep it continuous over variance of flex factors and item sizes.
5. Keep sizing variance as linear as possible with respect to linear changes to any input variable (size, flex factor).
6. When the sum of flex factors is >=1, return the minimum size necessary for every item to be >= max-content size.

Principle 6, which is a superset of 2, was what we sacrified in order to make the rest work.

Sample Cases

(based on your Apr 19 testcase but with varying flex-grow values)

Flex	Calculation	Final Sizes
0|0	no growth	200|100
0|.1	0|10 → chosen 10, reversed to 100 → size is (200+0)+(100+.1*100) = 310px	200|101
.1|.1	30|10 → chosen 30, reversed to 150 → size is (200+.1*150) + (100+.1*150) = 330px	203|103
.4|.4	120|40 → chosen 120, reversed to 150 → size is (200+.4*150) + (100+.4*150) = 420	248|148
.5|.5	150|50 → chosen 150 → size is (200+.5*150) + (100+.5*150) = 450	275|175
.75|.75	225|75 → chosen 225 → size is (200+.75*225) + (100+.75*225) = 368.75+268.75 = 637.5	368.75|268.75
1|1	300|100 → chosen 300 → size is (200+300) + (100+300) = 900	500|400
0|2	0|50 → chosen 50 → size is (200+0) + (100+2*50) = 400	200|200
.1|2	30|50 → chosen 50 → size is (200+.1*50) + (100+2*50) = 405	205|200
.2|2	60|50 → chosen 60 → size is (200+.2*60) + (100+2*60) = 432	212|220
2|2	150|50 → chosen 150 → size is (200+2*150) + (100+2*150) = 900	500|400

@davidsgrogan @dholbert Let us know if it looks good, or if you notice any other concerns!

[davidsgrogan]

Looks good. Implementing will be the real test, but I'm optimistic!

Is d452a10 the only commit or are there more coming?

[tabatkins]

That was it!

[css-meeting-bot]

The CSS Working Group just discussed [css-flexbox-1] Intrinsic Main Size algo has errors.

The full IRC log of that discussion

<dael> Topic: [css-flexbox-1] Intrinsic Main Size algo has errors
<fantasai> See https://github.com//issues/7189#issuecomment-1172771501
<dael> github: https://github.com//issues/7189
<dael> TabAtkins: This is a call for review from others. We found when reviewing WPT that the handling of intrinsic sizes of flexboex when sum < 1 wasn't correct. Have gone back and forth with dgrogan a few times. Pretty sure it's right now.
<dael> TabAtkins: Found to avoid explosions to infinity need slightly different algo to get correct intrinsic size. David thinks they're fine. Anyone else, we would appriciate review. It's is subtle changes and if there are mistakes or violates other requirements would appreciate review
<dael> fantasai: We should get resolution, but can have more time for review
<dael> Rossen_: I was reading through commit linked in issue and it's quite involved. Would prefer to give time to reason through it
<dael> Rossen_: I think even though some of it is notes they're complex and good to give time
<dael> TabAtkins: Yeah. This was complicated enough I had to do 2 nested details. Definitely a little complex
<dael> Rossen_: So in that case action to implementors of flexbox to please review the commit
<Rossen_> commit to be reviewed https://github.com/w3c/csswg-drafts/commit/d452a10a921391dc6b6a96e7be97218692c1de53
<Rossen_> q?
<dael> jensimmons: Do the WPT tests need updating or are they now accurate?
<dael> TabAtkins: Not 100% sure but I believe tests will need updating. I think David will take care of that
<dael> jensimmons: If he's not, recommend opening an issue on interop repo
<dael> jensimmons: Just so they're aware
<dael> jensimmons: I can file
<dael> fantasai: I think intrinsic algo is a focus on there since it's been changing based on impl feedback
<dael> Rossen_: Would be great to add the issue, thanks jensimmons

[tabatkins]

Okay, been six weeks since the last request for impl review. Agenda+ to close it.

/cc @dholbert especially for eyes on this

[davidsgrogan]

FYI, the new spec text is implemented in Blink behind a flag.

[css-meeting-bot]

The CSS Working Group just discussed Intrinsic Main Size algo has errors, and agreed to the following:

• RESOLVED: Accept changes, close the issue

The full IRC log of that discussion

<heycam> Topic: Intrinsic Main Size algo has errors
<heycam> github: https://github.com//issues/7189
<heycam> TabAtkins: this one should be easier
<heycam> ... we noticed an error, discovered what it was, talked it over with David, fixed it to his satisfaction
<fantasai> https://github.com/w3c/csswg-drafts/commit/d452a10a921391dc6b6a96e7be97218692c1de53
<heycam> ... we asked for a review few weeks ago, if no comments, we can go ahead and accept it
<heycam> fantasai: this is already folded into the spec
<heycam> dholbert: I haven't had a chance to look
<heycam> ... I could take a quick look offline. but I suspect it's fine.
<heycam> Ian: we are still implementing this. don't know what the compat fallout will be
<heycam> ... but we need to try to work out what that looks like
<heycam> ... this is just a bit hairy because we've all had the same intrinsic sizing alg for a while
<heycam> RESOLVED: Accept changes, close the issue

[fantasai]

@dholbert Mark Commenter Satisfied if you're happy with it, re-open if not!

[nicoburns]

There seems to be no changelog entry for this change here (https://drafts.csswg.org/css-flexbox/#changes-20181119). Is that intentional? (am I looking in the wrong place?)