Closed Bug 1499542 Opened 1 year ago Closed 1 year ago

Skip FreezeItemsEarly() step of flex layout if we're collecting data for flex devtools

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(4 files)

Attached file testcase 1
We have a (small) optimization in flex layout to do an up-front freeze of some flex items that obviously can't flex away from their flex base size. That happens here:
https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#2503-2551

...and it's actually "required" by the spec as Step 2 "Size inflexible items" here:
  https://drafts.csswg.org/css-flexbox/#resolve-flexible-lengths

However, for flex items with nonzero flex-grow/flex-shrink, this early-freezing means we never find out how much these items "wanted" to flex, which means we can't accurately draw the flex item diagram in devtools.

I think we should skip this early-freeze (at least, the part for items with a nonzero flex factor) if we're using devtools.  In practice, this won't impact the results of flex layout (i.e. the divergence from the letter-of-the-spec isn't detectable), because the affected items will trivially cause min/max violations in the first loop of flex layout and we'll simply be forced to run our flex layout loop an extra time.  (In other words, the "Early freeze" step just gets us out of doing an extra run of the main flex layout loop, and I think we want to opt in to doing that loop so that we can capture more info for devtools.)

I'm attaching a testcase that (I think) exercises all three scenarios, for eventual testing in devtools to be sure the results make sense.
Summary: Skip FreezeItemsEarly() step of flex layout if we're compiling data for flex devtools → Skip FreezeItemsEarly() step of flex layout if we're collecting data for flex devtools
Ultimately, I think we want devtools to show the following for the attached testcase (attachment 9017700 [details]):

 Item 1 (purple): Inflexible -- flex-size == base-size == 50px. (This is already basically good.)

 Item 2 (orange):
 - Base size is 40px.
 - Wants to grow to 100px (to fill container).
 - ...but it gets clamped at its max-width which is 30px (which happens to be smaller than its base size)

 Item 3 (blue):
 - Base size is 110px.
 - Wants to shrink to 100px (to stop overflowing container).
 - ...but it gets clamped at its min-width which is 150px (which happens to be larger than its base size)


For this to work, we need to report a deltaSize of "+60" for the orange item, and a deltaSize "-10" for the blue item (even though those aren't the deltas that those items end up actually getting).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
We'd like to be able to record the "desired" growth/shrinking for these items
during the first loop of the flex layout algorithm, but we can't do that if we
freeze them before we start flexing.
Depends on: 1499569
Frozen flex items have already been clamped to their min/max sizes, so their
size-change isn't a "how much we want to flex" measurement, and it's not useful
for determining whether the rest of the line is shrinking.

Depends on D8922
Attachment #9017706 - Attachment description: Bug 1499542: Don't do early-freeze of flexible-but-doomed-to-be-clamped flex items, if devtools are active. → Bug 1499542 part 1: Don't do early-freeze of flexible-but-doomed-to-be-clamped flex items, if devtools are active.
Blocks: 1498273
Blocks: 1498281
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52bd865d757c
part 1: Don't do early-freeze of flexible-but-doomed-to-be-clamped flex items, if devtools are active.  r=bradwerth
https://hg.mozilla.org/integration/autoland/rev/3ea646bde4de
part 2: Skip frozen flex items when recording grow-vs-shrink state and deltas. r=bradwerth
https://hg.mozilla.org/integration/autoland/rev/2ce0ac92bee5
part 3: Give test_flex_items.html an item that's trivially clamped to small max-size. r=bradwerth
Depends on: 1500608
You need to log in before you can comment on or make changes to this bug.