Repository navigation
[css-flexbox-1] Intrinsic Main Size algo has errors #7189
Description
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
.5flex factor should be treated identically to both having1, but if they're both.25they'll be somewhat different, ideally halfway between the1case and the0(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:
- 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).
- 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.
- 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.
- 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
.5and1:
- 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.
Activity
davidsgrogan commentedon May 10, 2022
spec editors, I'd love it if you could work on this soon!
(you're not waiting for me for anything, are you?)
[css-flexbox-1] Separate out the positive and negative cases to avoid…
fantasai commentedon Jul 1, 2022
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
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)
@davidsgrogan @dholbert Let us know if it looks good, or if you notice any other concerns!
davidsgrogan commentedon Jul 6, 2022
Looks good. Implementing will be the real test, but I'm optimistic!
Is d452a10 the only commit or are there more coming?
tabatkins commentedon Jul 6, 2022
That was it!
css-meeting-bot commentedon Jul 7, 2022
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
[css-flex] Update intrinsic size algorithm for single row containers
[css-flex] Update intrinsic size algorithm for single row containers
[css-flex] Update intrinsic size algorithm for single row containers
[css-flex] Update intrinsic size algorithm for single row containers
[css-flex] Update intrinsic size algorithm for single row containers
[css-flex] Update intrinsic size algorithm for single row containers
tabatkins commentedon Aug 17, 2022
Okay, been six weeks since the last request for impl review. Agenda+ to close it.
/cc @dholbert especially for eyes on this
davidsgrogan commentedon Aug 17, 2022
FYI, the new spec text is implemented in Blink behind a flag.
Bug 1780997 [wpt PR 35214] - [css-flex] Update intrinsic size algorit…
css-meeting-bot commentedon Sep 15, 2022
The CSS Working Group just discussed
Intrinsic Main Size algo has errors, and agreed to the following:RESOLVED: Accept changes, close the issueThe 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 commentedon Sep 15, 2022
@dholbert Mark Commenter Satisfied if you're happy with it, re-open if not!
nicoburns commentedon Apr 11, 2023
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?)
fantasai commentedon Jun 29, 2025
@nicoburns Updated! Sometimes we get out of sync on the manual changelog... but we should fix that!