Closed
Bug 1498281
Opened 6 years ago
Closed 6 years ago
The flex base size that we report to devtools API has sometimes already been clamped to min/max-size
Categories
(Core :: Layout: Flexbox, enhancement, P3)
Core
Layout: Flexbox
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
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.
Updated•6 years ago
|
Assignee: nobody → bwerth
Assignee | ||
Comment 1•6 years 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 years 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 years 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 years 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 | ||
Comment 5•6 years 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 years 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 years 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 8•6 years ago
|
||
Depends on D8476
Assignee | ||
Comment 9•6 years 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 years ago
|
||
(I'm rebasing this on top of bug 1499542, which is making some other related fixups.)
Depends on: 1499542
Assignee | ||
Comment 11•6 years ago
|
||
In particular, bug 1499542 part 2 avoids the FlexLineGrowthState assertions that I mentioned in comment 9 here.
This is now ready for review.
Updated•6 years ago
|
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 years 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 years ago
|
Attachment #9016486 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years 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 years 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 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•