Review of Flexbox Intrinsic Size Tests

[fantasai]

Some commentary from me and @tabatkins on https://github.com/web-platform-tests/wpt/tree/master/css/css-flexbox/intrinsic-size

Overall Feedback:

The reftests are focused on creating a green box, which isn't always as rigorous a rendering as we could be checking. For example, the size of the flex container is evident, but the size of the flex items are not. We want the test to fail in as many relevant failure conditions as possible! So for these tests, it would be useful to see the bounds of both the flex container and any items in it, to make sure they're all the right sizes.

(Note that we have a lot of reftests that were written before we had automated testing, and had to be manually checked, so there are a lot of tests which go out of their way to be rapidly evaluated by a human. It's still useful to make the tests render understandably, but a) that doesn't mean the tests should compromise their rigor and b) now that we have reftest automation, we don't need to complexify the test to make it simple enough to evaluate in < 3 seconds.)

Also, the asserts in 001-004 are suuuper helpful for reviewing! Thanks for writing them! It would be nice to also have such asserts in the testharness.js tests. :)

row-001:

• Missing closing div

row-002, row-003:

• Looks correct

row-004:

• Test looks correct, but the assert is slightly off: the flex container's min-content size is capped by the item's flex base size when the item can't grow. (If its contents were smaller, then it could still shrink smaller; and there should be a test for that case as well.)

row-005:

• Tests 1, 2, and 3 look correct.
• Test 4 is incorrect - it appears to have missed the "having floored the flex shrink factor at 1" text in 9.9.1 step 1. With that properly taken into account, the expected width should be 400px. (The min-context flex fraction of the first child is -200px; the second's is -400px. We choose -200px as the larger. The first child then doesn't shrink at all from its flex-basis (as the shrink factor is not floored in step 3), while the second shrinks by 200px, ending at 200px wide. 200+200 = 400 total width for the flexbox.)
• Test 5 looks correct (auto min size wins here, making each 100px wide, for 200px flexbox width)
• Test 6 looks correct (honoring 'width' as an "ideal" size, one of them hits 200 and the other grows to 400 to keep the flex fractions correct)
• Test 7 looks correct (one shrinks, one grows, so the positive flex fraction is chosen; first item has 0 growth, second grows to 100 + (2*50)=200, so 400px total flexbox width)

(the contribution/fraction/math comments on test 6 are very helpful; having the same on all 7 would have been great)

CC @davidsgrogan

[davidsgrogan]

Thank you for taking a look. We need to dive deeper into case 4, though.

The min-context flex fraction of the first child is -200px; the second's is -400px.

My interpretation of current spec is that there can never be a negative flex fraction with pixel units. I filed https://github.com/w3c/csswg-drafts/issues/6909 about that. As you can see in one of the last comments, I eventually decided this was intentional.

This is what I've got for case 4:

      <div class="floating-flexbox" data-expected-width="600">
        <div style="flex: 1 0 200px; width:50px;">
          <div></div>
        </div>
        <div style="flex: 1 1 400px; width:50px;">
          <div></div>
        </div>
      </div>

1. min-content contributions of each item = 200px, 100px respectively (min-content of each item is 100px but item 1 floors by its flex base size because it is non-shrinkable)
2. min-content contribution - flex base size = 0px, -300px respectively
3. flex fraction for item 1 = 0px
4. scaled flex shrink factor having floored the flex shrink factor for item 2 = 400px
5. flex fraction for item 2 = -3/4 (unitless value, see https://github.com/w3c/csswg-drafts/issues/6909)
6. largest fraction = 0px
7. flex base size + flex grow factor chosen max-content flex fraction for item 1 = 200px + 10px = 200px
8. flex base size + flex grow factor chosen max-content flex fraction for item 2 = 400px + 10px = 400px
9. sum of last two steps = 600px.

Our interpretations diverge by step 3. Could you describe how you calculated the flex fractions to be -200px and -400px?

[gsnedders]

How do these tests relate to #5791?

[tabatkins]

You're right per spec - we forgot about the flooring that occurs for min-content contribution when the item is inflexible. Taking that into account, we do indeed get the result you're asserting in your test.

That said, 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. We haven't gotten an exact proposal down yet; we'll work on that later today.

[davidsgrogan]

Ah, the "continuous" requirement is obvious in retrospect but I totally didn't think of it. Looking forward to seeing what you come up with.

(I'm glad you reviewed the tests before we shipped this -- thank you again!)

[tabatkins]

Okay, 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.

Actual proposed spec text incoming.

[tabatkins]

Just as a note: the min-content contribution discontinuity error was added in https://github.com/w3c/csswg-drafts/commit/2c446befdf0f686217905bdd7c92409f6bd3921b, from the conclusions in https://lists.w3.org/Archives/Public/www-style/2015Aug/0212.html