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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
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.
Blocks: 1478396
Assignee: nobody → bwerth
(Assignee)

Comment 1

6 months ago
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.
(Assignee)

Comment 2

6 months ago
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?
Flags: needinfo?(pbrosset)
(Assignee)

Comment 3

6 months ago
(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...)
(Assignee)

Comment 4

6 months ago
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.
Assignee: bwerth → dholbert
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
(Assignee)

Updated

6 months ago
Depends on: 1409083
(Assignee)

Comment 5

6 months ago
(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 )
(Assignee)

Comment 6

6 months ago
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.
(Assignee)

Comment 7

6 months ago
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.
(Assignee)

Comment 9

6 months ago
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.
(Assignee)

Comment 10

6 months ago
(I'm rebasing this on top of bug 1499542, which is making some other related fixups.)
Depends on: 1499542
(Assignee)

Comment 11

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

This is now ready for review.
Attachment #9016485 - Attachment description: Bug 1498281 part 1: Don't pre-clamp flex base size in flexbox devtools API. → Bug 1498281: Make flexbox devtools API report actual flex base size (not its min/max-clamped version).
(Assignee)

Comment 12

6 months ago
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
(Assignee)

Updated

6 months ago
Attachment #9016486 - Attachment is obsolete: true
(Assignee)

Comment 13

6 months ago
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).
Blocks: 1499875

Comment 14

6 months ago
Pushed by dholbert@mozilla.com:
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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48155e26a841
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.