Closed Bug 1498273 Opened 6 years ago Closed 6 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: dholbert, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

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.
Assignee: nobody → bwerth
Depends on: 1409083
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?
Flags: needinfo?(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.
Flags: needinfo?(dholbert)
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.
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.
(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?
Flags: needinfo?(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.
Flags: needinfo?(dholbert)
Depends on D8769
Depends on D8772
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.)
--> 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.)
Depends on: 1497589, 1499542
Attachment #9017328 - Attachment description: Bug 1498273 Part 3: Add tests of FlexItem clampState. r?dholbert! → Bug 1498273 Part 4: Add tests of FlexItem clampState. r?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().)
Attachment #9019177 - Attachment is obsolete: true
Attachment #9017323 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com: 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
Blocks: 1502002
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: