1498273 - Extend flexbox devtools API to return an indication of whether flex items were clamped

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

I think we could add a tri-state "clampStatus" enum to the FlexItem devtools API (returned per flex item)

Its values would be e.g. UNCLAMPED, CLAMPED_TO_MIN_SIZE, CLAMPED_TO_MAX_SIZE
and then DevTools could only display the min-size (or max-size) and the "lock" icon for the flex item if the enum is set to one of the "CLAMPED" variants.

Comment 1

[Brad Werth [:bradwerth]]

As I work on this, I wonder if the outcome will be any different than the results of a conditional that could be done on the existing data. Such as:

mainSize == mainMinSize ? CLAMPED_TO_MIN
mainSize == mainMaxSize ? CLAMPED_TO_MAX
else UNCLAMPED

Semantically, this seems to me to provide all the value. The only confusing case I can think of is if the mainBaseSize is the same as the min or max size and we might report that as CLAMPED when the value didn't change at all. But is that an important case?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Brad Werth [:bradwerth] from comment #1)
> Semantically, this seems to me to provide all the value. The only confusing
> case I can think of is if the mainBaseSize is the same as the min or max
> size and we might report that as CLAMPED when the value didn't change at
> all. But is that an important case?

That's exactly the case where this sort of check would be problematic, yeah.  (strictly speaking, where flex-base-size == main-min-size, and where we have flex-grow:0).  That is a common scenario, which makes it important -- there are quite a few scenarios where the max-content and min-content size would be the same, and that will produce a flex-base-size and main-min-size that are the same as well, by default.

Here's a simple example where mainSize == mainMinSize, where nothing is shrinking and no clamping is happening:
  data:text/html,<div style="display:flex;background:yellow"><div style="background:pink">aaa</div>


We want to be precise about reporting "clamped? y/n" for this scenario, because we want to be able to use the "was-clamped" status to report information like:
 - "Item tried to grow, and was clamped at its max-width"
 - "Item tried to shrink, and was clamped at its min-width"

If we naively do that right now, we report that the item tried to shrink and was clamped -- and that makes no sense.

Comment 3

[Brad Werth [:bradwerth]]

Thank you. Got it. I agree we don't want to report clamping in common cases where no clamping has happened. I'll proceed with the plan.

Comment 4

[Daniel Holbert [:dholbert]]

Thanks!

BTW, I *think* the items that we want to report as clamped are precisely the ones that we call "SetHadMinViolation" / "SetHadMaxViolation" on... At least, those are the ones that most directly match the "tried to grow and was clamped" scenario that we're going to be trying to describe with the clamping UI for this feature.

Comment 5

[Brad Werth [:bradwerth]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> BTW, I *think* the items that we want to report as clamped are precisely the
> ones that we call "SetHadMinViolation" / "SetHadMaxViolation" on... At
> least, those are the ones that most directly match the "tried to grow and
> was clamped" scenario that we're going to be trying to describe with the
> clamping UI for this feature.

There seem to be other areas where clamping occurs, such as in UpdateMainMinSize() and SetFlexBaseSizeAndMainSize(). Are those important clamping "events" to detect and report?

Comment 6

[Daniel Holbert [:dholbert]]

The min/max clamping isn't "official" until the item has been frozen (via Freeze() / IsFrozen()). Before that time, these preemptively-kinda-clamped elements are just used to help us determine how much free space we expect to have.

I did find one other place that does matter, though:

- FreezeItemsEarly(), where we need to be a bit subtle:
** easier part: we can infer (and should record) that we're clamping to min/max in each of the GetFlexBaseSize()-compared-to-GetMainSize() cases (where we set shouldFreeze = true)

** subtler part: in the other up-front "shouldFreeze" case (where the item's flex-factor is 0 in the direction that we're flexing), we should record a violation in one direction or the other iff GetFlexBaseSize() != GetMainSize().

so e.g. we have:

      bool shouldFreeze = (0.0f == item->GetFlexFactor(aIsUsingFlexGrow));
...and right after that, I think we should add something like:
      if (shouldFreeze && GetFlexBaseSize() != GetMainSize()) {
        clampStatus = GetFlexBaseSize() < GetMainSize() ?
          CLAMPED_TO_MIN_SIZE : CLAMPED_TO_MAX_SIZE;
      }

and then a bit lower down where we have...
          if (item->GetFlexBaseSize() > item->GetMainSize()) {
            shouldFreeze = true;
          }
...we should insert something like this into that clause:
             clampStatus = CLAMPED_TO_MAX_SIZE;

And then at the very bottom, "commit" the clamp status to our devtools FlexItem data-structure or whatever.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1498273 Part 1: Updated Flex.webidl to add a per-item clamp state attribute. r?dholbert!

Comment 8

[Brad Werth [:bradwerth]]

Attachment: Bug 1498273 Part 2: Define and set ComputedFlexItemInfo::mClampState. r?dholbert!

Depends on D8769

Comment 9

[Brad Werth [:bradwerth]]

Attachment: Bug 1498273 Part 4: Add tests of FlexItem clampState. r?dholbert!

Depends on D8772

Comment 10

[Daniel Holbert [:dholbert]]

I realized we actually want to skip the substantial bit of FreezeItemsEarly when devtools are active, so I filed bug 1499542 on that.

(That'll mean we can get rid of this bug's FreezeItemsEarly changes.  In an ideal world, that probably means we should fix bug 1499542 before fixing this bug, to avoid unnecessary churn in FreezeItemsEarly -- though we could also take this bug first and then revert its FreezeItemsEarly changes as part of bug 1499542.)

Comment 11

[Daniel Holbert [:dholbert]]

--> Updating dependency field to reflect that this now implicitly depends on:
 bug 1497589 (which is renaming the devtools classes to avoid naming collisions so we can use FlexBinding.h's enum without naming collisions, hooray)
 bug 1499542 (which is delaying some clamping as noted in comment 10, and obsoleting some of Part 2 here.)

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Bug 1498273 Part 3: Move Flex API info structure construction earlier in layout to capture early clamping outcomes. r?dholbert!

Depends on D8772

Comment 13

[Daniel Holbert [:dholbert]]

Per IRC discussion: it now seems like it'd be cleanest to do something like:

* First part:
Coopt FlexItem::mHad{Min,Max}Violation (currently unused after freezing) to track whether we were clamped to {min,max}-size, in frozen items.

* Second part:
Create the enum-typed ComputedFlexItemInfo / FlexItemValues / etc. fields for tracking clamped state, and just directly set the ComputedFlexItemInfo field based on new (mutually exclusive) FlexItem::WasClampedToMinSize() accessors.  We'd probably want to do this immediately after the call to ResolveFlexibleLengths().

(Note: Initially, I was thinking we could do this in the loop at the bottom of ResolveFlexibleLengths(), but that doesn't quite work because we might take the early-return and skip that loop entirely.  So, probably best to do it after ResolveFlexibleLengths().)

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1498273 Part 2: Change mHasMinViolation and mHasMaxViolation to also track clamp state after item freeze.

Depends on D8769

Comment 15

[Brad Werth [:bradwerth]]

Attachment: Bug 1498273 Part 3: Define and set ComputedFlexItemInfo::mClampState.

Depends on D9727

Comment 16

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43c04f01bf7fca027cc0a2c6696cfb19dc56682e

Comment 17

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05acc494a292b1da4bf1c6f80086d2e7235ecbb1

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24b46ea119a0
Part 1: Updated Flex.webidl to add a per-item clamp state attribute. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/28d81e5c33d8
Part 2: Change mHasMinViolation and mHasMaxViolation to also track clamp state after item freeze. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a40344d98665
Part 3: Define and set ComputedFlexItemInfo::mClampState. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1c2698dc96cb
Part 4: Add tests of FlexItem clampState. r=dholbert

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/24b46ea119a0
https://hg.mozilla.org/mozilla-central/rev/28d81e5c33d8
https://hg.mozilla.org/mozilla-central/rev/a40344d98665
https://hg.mozilla.org/mozilla-central/rev/1c2698dc96cb