STR: 1. View this testcase: data:text/html,<div style="display:flex"><div style="flex: 0 1 auto;">Inspect 2. Inspect the innermost element (the flex item) in devtools and look at the "Layout" panel. ACTUAL RESULTS: The panel says: > Flexibility: 0 > Item was set to shrink EXPECTED RESULTS: The "Item was set to shrink" message is incorrect and should not be shown. We have plenty of space available, so there's no reason for anything to shrink. (This item *could* shrink if it needed to, but nothing set it to actually shrink.) If we want to say something more accurate, perhaps it'd be something like: "The flex line invited its items to grow, but this item has flex-grow:0 and so could not."

So the issue here is this chunk of code[1] from bug 1495717: === // Then tell users whether the item was set to grow, shrink or none of them. if (flexGrow && !flexGrow0 && lineGrowthState !== "shrinking") { reasons.push(getStr("flexbox.itemSizing.setToGrow")); } if (flexShrink && !flexShrink0 && lineGrowthState !== "growing") { reasons.push(getStr("flexbox.itemSizing.setToShrink")); } if (!grew && !shrank && lineGrowthState === "growing") { reasons.push(getStr("flexbox.itemSizing.notSetToGrow")); } if (!grew && !shrank && lineGrowthState === "shrinking") { reasons.push(getStr("flexbox.itemSizing.notSetToShrink")); } === In this bug's testcase, the second clause there is getting activated, which makes us report a misleading message. We satisfy the conditions for that line -- we have nonzero flex-shrink, and our line GrowthState is not "growing", BUT our line is not "shrinking" either -- it is in fact "unchanged", because really we've got free space but our item is not set to grow. I think part of the problem might be that there's an impedence mismatch around the FlexLine growthState API, between what devtools wants vs. what our API is providing. - It seems like this devtools code *wants* to know which flex factor the line is using (flex-grow vs. flex-shrink) -- i.e. whether there's positive or negative free space, so it can report an accurate reason about why the item might've been able to grow/shrink were it not for its zero'd flex factor. - BUT currently, the growthState API just indicates whether **some items grew**, vs. some items shrank, vs neither. (as documented at [2]) I suppose this means that this API isn't particularly useful in the scenario when items are inflexible in the direction that the line would like to grow -- you can't distinguish between "item wasn't flexible so it didn't grow [shrink]" vs. "line had exactly the right amount of space so nothing grew [shrank]". We could potentially adjust the API to return what devtools seems to want, if that's best. Perhaps the simplest solution would just be to directly return an enum that represents the flex factor that we're using, as defined in the spec[3] (9.7 step 1), and then devtools can just make use of that however it likes? [1] https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/devtools/client/inspector/flexbox/components/FlexItemSizingProperties.js#172-184 [2] https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/dom/chrome-webidl/Flex.webidl#42-43 [3] https://drafts.csswg.org/css-flexbox-1/#resolve-flexible-lengths

(In reply to Daniel Holbert [:dholbert] from comment #4) > We could potentially adjust the API to return what devtools seems to want, > if that's best. Perhaps the simplest solution would just be to directly > return an enum that represents the flex factor that we're using, as defined > in the spec[3] (9.7 step 1), and then devtools can just make use of that > however it likes? If we were to do this, I think we'd just want to assign aLineInfo->mGrowthState here: https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/layout/generic/nsFlexContainerFrame.cpp#2642-2644 And in that case, I don't think the "unchanged" enum is particularly useful; we should probably make our reporting match what the spec [and our impl] does, and just treat 0-extra-space as a special case of "growing". Perhaps that means we should morph this enum into a boolean value? As an illustration: testcase 2 is one scenario where the sizes are just right (when the hypothetical, i.e. clamped, flex base sizes add exactly to the container's size). Despite the fact that the free space is 0, we still *try* to do a "flex-grow-flavored" run of flex layout, and we do still try to grow each item from its base size (10px in this example) with an equal share of the free space initially. We end up clamping the one with the larger min-size when that's not sufficient to get it to its minimum, and then restart, which leaves exactly enough for the other one to reach its min-size. So in practice, this is both items growing, with the larger one being clamped "upwards" to satisfy its min-size -- and that still feels useful to report. So, bottom line, I tend to think we should get rid of "unchanged" and just report which flex factor we're using. To do this without causing exceptions in the existing devtools, perhaps we should: - leave growthState as-is, but mark it as deprecated/doomed-to-be-removed-soon - add a new boolean (or enum) API to represent "isUsingFlexGrow" (on a per-line basis), whose value is assigned at that nsFlexContainerFrame.cpp searchfox link above - file a followup bug on making devtools use the new thing rather than growthState (and then remove growthState entirely once we've dropped its usages) Brad/Patrick/Gabe, what do you think?

Here's another fun example -- right now we report the following information for the 2nd flex item, none of which is really accurate: 1) There wasn't enough room available on the flex line. 2) Item was set to shrink. 3) Item was not set to shrink. 4) Item was set to shrink but could not. 5) The item wanted to shrink, but it was clamped. Problems here: - "There wasn't enough room available" - false, there's exactly enough room available for the items' hypothetical (i.e. clamped) sizes. - Clearly #3 contradicts #2 and #4 (RE wanting vs. not-wanting to shrink), - There's some redundancy among #2, #4, and #5. - Plus, in reality, this item really wants to grow -- it does *not* in fact want to shrink, despite what we're saying here! (Its flex line ends up with 0px of left over space in https://drafts.csswg.org/css-flexbox-1/#resolve-flexible-lengths 9.7 step 1, which means we're using flex-grow as the flex factor. This means the algorithm does an early-freeze of the 1st item, which trivially cannot grow at all since its flex base size is already too big. That leaves us with plenty of free space to legitimately distribute to the 2nd item, and it happens to be exactly the right amount to get that 2nd item just up to its min-width -- no clamping required there.)

(In reply to Daniel Holbert [:dholbert] from comment #6) > To do this without causing exceptions in the existing devtools, perhaps we > should: > - leave growthState as-is, but mark it as > deprecated/doomed-to-be-removed-soon > - add a new boolean (or enum) API to represent "isUsingFlexGrow" (on a > per-line basis), whose value is assigned at that nsFlexContainerFrame.cpp > searchfox link above > - file a followup bug on making devtools use the new thing rather than > growthState (and then remove growthState entirely once we've dropped its > usages) > > Brad/Patrick/Gabe, what do you think? I think the name of the enum is fine, and 2 of the 3 values are fine, but we should just update the meaning to match what you suggest in comment 6. So we keep the growthState enum, and it only ever gets "growing" or "shrinking". Nothing needs to be deprecated -- just re-interpreted.

Here's one more variant. Right now devtools reports the following somewhat inaccurate information for the 1st item, which has style="flex: 1 300px;max-width:10px" : 1) There was extra room available on the flex line. 2) Item was set to grow. 3) Item was not set to grow. 4) Item could not grow, siblings have likely used the extra space. Problems with these statements: #1 is not quite true (there's 0 space available per https://drafts.csswg.org/css-flexbox-1/#resolve-flexible-lengths 9.7 step 1) #3 directly contradicts #2 and #4. #4 isn't really accurate RE *siblings* preventing us from growing here (really, it was our max-width that was the limiting factor on our size here; that, combined with our extremely-greedy flex-basis). I think bug 1498273 will help with improving the messaging for #4 here, but #1-#3 are all related to this line growthState stuff & could hopefully be improved/collapsed into a single more-correct statement via a reworking of that state & its resulting messaging.

(In reply to Brad Werth [:bradwerth] from comment #8) > [...] we keep the growthState enum, and it only ever gets "growing" or "shrinking". > Nothing needs to be deprecated -- just re-interpreted. Cool, that sounds reasonable to me, as long as we document that "growing" == the items' hypothetical sizes do not exceed the size of the flex line (though they might exactly reach it)

Conveniently, the "unchanged" value doesn't seem to be used in FlexItemSizingProperties.js at all :) So it'll be easy to deprecate. I'll spin off a Core::Layout:Flexbox helper bug to remove that value (& simplify the growing/shrinking assignment), and then let's use this bug to fix the messaging & logic in devtools in some/all of these testcases. (It's possible the messaging will be somewhat improved "for free" just by virtue of the helper bug being fixed & "growing"/"shrinking" being reported in cases where we're currently reporting "unchanged").

Depends on D9451

I've moved my patches over to the helper Bug 1501109.

Notes: * The fix for Bug 1501109 (which hit autoland earlier today) has improved things here a bit. Now we report the correct information for testcase 1 ("There was extra room available on the flex line; Item was not set to grow.") * ...but, for testcase 2, 3, and 4, we still report contradictory information for at least one flex item (i.e. we say that an item is both "set to shrink" + "not set to shrink" in testcases 2 & 3, and "set to grow" + "not set to grow" in testcase 4) * If I also add the current patch for bug 1501207, that improves things a bit more -- that removes the contradictory report for testcase 4. But testcases 2 and 3 still report contradictory information ("set to shrink" + "not set to shrink").

I've been working on this today, and cleaned up a lot of things. I believe that all incorrect cases reported here are fixed now. I'm wondering about one thing though, related to clampState: With testcase 3 (attachment 9019183 [details]) the first flex item is as follow: flex: 1 300px;max-width:20px; the getAsFlexContainer API returns a base size of 300px (correct), a delta of 280px (seems correct too), but a clampState of "unclamped". I was expecting it to be "clamped_to_min" in this case. Brad: do you have thoughts on that?

dholbert and I discussed about this, it actually does not make sense to be saying that there was or was not enough room on the line for all items, because we can't know this for sure. So this part was essentially giving false information to users. It's kind of tricky, since it varies on each run of the flex algorithm. In the scenario where an item gets clamped (or multiple items get clamped in successive runs), then the answer to those questions are iffy (& different for each item, potentially).

In cases where an item did not grow or shrink, devtools doesn't really have access to any useful information about how the item might have wanted to flex. So in this change, I'm just making sure we don't attempt to display this info at all, because it's basically incorrect otherwise. Depends on D10235

(In reply to Patrick Brosset <:pbro> from comment #16) > I'm wondering about one thing though, related to clampState: > > With testcase 3 (attachment 9019183 [details]) the first flex item is as > follow: > flex: 1 300px;max-width:20px; > the getAsFlexContainer API returns a base size of 300px (correct), a delta > of 280px (seems correct too), but a clampState of "unclamped". I was > expecting it to be "clamped_to_min" in this case. (edit: I think you meant clamped_to_max.) I think the reporting you described ("unclamped") is actually correct here. The key thing here is that *both* items get clamped, and as it happens, the 2nd item gets clamped first. (Note that each run of flex layout can *only* clamp max-width violations OR min-width violations -- never both at the same time. So we can only clamp one or the other on the first run of the flex loop.) Then in the second run of the flex loop, there's only the first flex item, and we give it all of the (negative) free space, which happens to be *exactly* enough to reach its max-width. (because this is a contrived example, with 1st-item-max-width:20px + 2nd-item-min-width:80px = exact-flex-container-width:100px)

(In reply to Daniel Holbert [:dholbert] from comment #19) > I think the reporting you described ("unclamped") is actually correct here. > The key thing here is that *both* items get clamped Sorry, this was wrong :D I meant to say: "both items have a min or max constraint that their base sizes violate". But in fact, only the second item gets clamped, because things work out for the 1st item such that it (barely) satisfies its constraint via normal flexing without any clamping needed.

(In reply to Daniel Holbert [:dholbert] from comment #19) > (edit: I think you meant clamped_to_max.) Yes I did :) Thanks. Thanks for the explanation Daniel. As always, very helpful to understand the inner workings of the flex algorithm. I'll leave my changes as is then. I feel like the tool is much more robust with these now. I'm really looking forward to people trying it out and hopefully learning a thing or 2 about flexbox!

Pushed by pbrosset@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/733862cddcb7 part 1 - Remove 'there was/nt enough room' part as it's sometimes incorrect; r=mtigley

Pushed by pbrosset@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c5dc48aa9dc part 2 - Simplify flexibility info and bail out when useless; r=mtigley

https://hg.mozilla.org/mozilla-central/rev/733862cddcb7 https://hg.mozilla.org/mozilla-central/rev/7c5dc48aa9dc