1498281 - The flex base size that we report to devtools API has sometimes already been clamped to min/max-size

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

Right now we opportunistically do some "early" min/max-size clamping when generating flex items.

This means that devtools ends up reporting the wrong (already-clamped) value for the flex item's "flex base size".

We should capture the flex base size a little bit earlier, for more accurate reporting in devtools.  Ideally we should've resolved keywords and `calc` expressions, but not performed min/max clamping yet.

Comment 1

[Daniel Holbert [:dholbert]]

I think this should work as a testcase:
 data:text/html,<div style="display:flex"><div style="flex:0;min-width:50px">Inspect me

(We don't actually display the "resolved" flex base size in devtools yet, but I think we will soon after pbro lands a patch that reworks the inspector a bit. With that patch, I believe we start displaying "flex base size: 50px" for this data-URI testcase -- but we should instead report "flex base size: 0px".)

And for this one:
  data:text/html,<div style="display:flex"><div style="min-width:0px;max-width:20px">Inspect me
... we probably report "flex base size: 20px", but really we should report it as being whatever the text's max-content size is.

Comment 2

[Daniel Holbert [:dholbert]]

Actually, it looks like we do this "early clamping" on a different variable, and we do preserve the untainted flex-base-size (I think).  So it might be that we're just capturing the wrong variable to report via the devtools API.

In particular, I suspect this line:
>        aLineInfo->mItems[itemIndex].mMainBaseSize = item->GetMainSize();
https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/layout/generic/nsFlexContainerFrame.cpp#2674

...should say "item->GetFlexBaseSize()", instead of "item->GetMainSize()".

Patrick: if you get a chance, would you mind testing that ^^ tweak (in a non-artifact build) with your patches applied, and see if the reported flex-basis value seems more accurate in the testcases above & the ones that you've got?

Comment 3

[Daniel Holbert [:dholbert]]

(Er actually, I suppose I can apply your patch and test this hypothetical tweak, too. I think I'd just need https://phabricator.services.mozilla.com/D8220 , which seems to apply cleanly, in order to see this member var reported...)

Comment 4

[Daniel Holbert [:dholbert]]

Yup, that does it.

In my first data URI in comment 1, this tweak makes us report a base size of 0px (and it attributes that to the "flex-basis:0%" which comes from the "flex:0" shorthand).

In my second data URI in comment 1, this tweak makes us report a base size of 89.px (and it attributes that to "The item's content size when unconstrained").

And the diagram helpfully shows those base sizes visually, vs. the actual final size (and the min or max that affected us).

--> I'll steal the assignee field, since I've already got a patch locally.  Hope you don't mind, Brad (& thanks for being up for taking this!)  I'll tag you to review if that's all right.

Comment 5

[Daniel Holbert [:dholbert]]

(note to self, I think the tests here should simply be additions to https://dxr.mozilla.org/mozilla-central/source/dom/flex/test/chrome/test_flex_items.html )

Comment 6

[Daniel Holbert [:dholbert]]

FWIW, this code-change makes us "fail" one existing testcase in that test, but it's OK, because really the testcase was making an incorrect assumption.

The failure report looks like this:
> dom/flex/test/chrome/test_flex_items.html
>  FAIL Item index 3 has expected mainDeltaSize. - got 37, expected +0

This is for a flex item with "min-width: 120px" and with some textual content -- the phrase "offset (first)".  This is kinda similar to my first data URI in comment 0 -- the true flex base size (in this case, the max-content width of the text) is smaller than the min-width (on my system at least -- the text size depends on fonts, of course).  So we end up "clamping up" to the min-width, which is 120px.

In current nightly, our devtools API (incorrectly) reports the already-clamped flex base size in this case, so it looks like we're going from 120px to 120px with a delta of 0.  But with my patch, we'll now report a flex base size from the text's max-content width (which is smaller than 120px, on my system), and the final size after clamping is still 120px, so we have a nonzero delta, which is correct.

So: I'll fix up this bit of the test so that the content is reliably wider than the min-width, so that the delta is actually 0. I'll probably do this by replacing the text with a fixed-size element of some sort, since text sizing is unreliable across platforms.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1498281: Make flexbox devtools API report actual flex base size (not its min/max-clamped version).

This patch includes a fix for a faulty assumption in test_flex_items.html, to
avoid causing a spurious test failure.  Previously, that test was assuming that
a particular flex item didn't grow away from its flex base size at all -- but
on my system, it does in fact have to grow in order to satisfy its specified
min-width.  I've fixed this in the test by giving the flex item larger content,
so that it won't have to grow to accommodate its min-width.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1498281 part 2: Add tests for flexbox devtools API with min/max-clamped flex items.

Depends on D8476

Comment 9

[Daniel Holbert [:dholbert]]

This isn't quite ready for review -- it introduces a problem with how we determine the FlexLineGrowthState right now (which is why "part 1" is currently softening some fatal assertions -- to allow the mochitest to complete).

Right now, if we see that an item's final size is smaller than our recorded-for-devtools flex base size, then we take that as a signal that the whole line is expected to be shrinking.  Now that we've got more "raw" flex base sizes, this is mistakenly making us think that a max-width clamped item is "shrinking" and hence its line must be shrinking, and we end up failing some assertions as a result, when that doesn't mesh with reality.

Really, we need to be smarter about determining the FlexLineGrowthState, and we need to accommodate cases where we've got a delta that's going in the opposite direction purely because of an up-front min/max violation.

Comment 10

[Daniel Holbert [:dholbert]]

(I'm rebasing this on top of bug 1499542, which is making some other related fixups.)

Comment 11

[Daniel Holbert [:dholbert]]

In particular, bug 1499542 part 2 avoids the FlexLineGrowthState assertions that I mentioned in comment 9 here.

This is now ready for review.

Comment 12

[Daniel Holbert [:dholbert]]

Try run with this bug's patch (and the patches from other bugs that it depends on):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d75e31129201ea4876644e0d586cbc726a9c5953

Comment 13

[Daniel Holbert [:dholbert]]

Note on testing:
- There are a few test changes in the "main" patch here (to update the test's previously-wrong expectations to reflect the more accurate reality).
- I initially had a second patch here with some new tests, but I ended up punting those "serious" tests to followup bug 1499875 so that I can assert more things about them (e.g. the exact amount that we tried to grow before we were clamped).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48155e26a841
Make flexbox devtools API report actual flex base size (not its min/max-clamped version). r=bradwerth

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/48155e26a841