1128354 - Flexbox optimization changed the positioning of "send" icon in Gaia SMS app (with nested flexboxes)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Bug 1127898 highlighted a regression in the gaia SMS app, caused by the optimization in bug 1054010.

I've got a trivial CSS fix for the app over on bug 1127898, which I think we should take.  But I'm filing this bug on investigating & fixing Gecko such that bug 1054010's optimization doesn't cause this regression in the first place.  (Other content is likely to hit it, too.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a reduced testcase from the SMS app.

In nightlies before bug 1054010 landed, the pink area is as tall as the blue area.

In nightlies from after bug 1054010 landed, the pink area (the innermost flex container) is much shorter -- it shrinkwraps its contents' height (so, it's ~1em tall).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a simpler testcase, with only a single (vertical) flex container.

I'm not clear on what the right behavior is here, or related cases, e.g. if we gave the flex item in this testcase a flex-basis of a fixed size like "10px" instead of the default, "auto".  It's unclear to me whether that flex item's final size should be considered "definite" [& usable for percent-height resolution on its children], given that its height technically depends on its children. (It gets its height here via "min-width:auto", which shrinkwraps its children.)

The CSSWG resolved last year (& updated the spec to say) that a flex item's final main-size should be considered definite if that flex item has a definite flex-basis:
 http://dev.w3.org/csswg/css-flexbox-1/#definite-sizes
That seems like a bit of an arbitrary distinction in cases like this where the flex item's min-size is really the primary thing that determines its final size, though.

I emailed the CSSWG to ask about this:
https://lists.w3.org/Archives/Public/www-style/2015Feb/0204.html
https://lists.w3.org/Archives/Public/www-style/2015Feb/0205.html

Comment 3

[Daniel Holbert [:dholbert]]

I'll break down how & why our behavior changed here a bit (partly for my own memory, & partly in case it helps anyone else following along here):

 - Previous to bug 1054010, we'd go through & establish the sizes of our flex items (which may involve reflow, e.g. to resolve 'min-height:auto').  Then, once we know everything's final size, we'd do a final reflow, with the flex-algorithm-provided height & width forced into the item's reflow state as its computed height & width.

 - Nowadays (after bug 1054010): when we're preparing for that final reflow for a flex item, we check if we (1) did a measuring reflow on the flex item and (2) if it ended up at the correct final size. If so, we assume we can skip the final reflow because it would have no effect. However, that *is not quite true* -- it *might* have an effect, even if the size hasn't changed. Specifically: our "measuring" reflow presumably had "height:auto" on the flex item, so any of the item's percent-height children would end up treating their percent heights as "auto".  And if we do a "final" reflow, we'll impose our final height on the flex item, and its percent-height children will see that height as something they *can* resolve percent heights against.

Really, we need to allow/disallow children of a flex item to resolve their percent heights *based on whether the spec says the flex item's height is definite*. But I want to make sure the rules in the spec are consistent & sensible; hence, my emails linked in comment 2.

Comment 4

[Daniel Holbert [:dholbert]]

Fantasai responded to my posts linked in comment 2:
https://lists.w3.org/Archives/Public/www-style/2014Jul/0009.html

The thread she linked to doesn't directly address the scenario here, but I think it's close enough to provide an answer.

Basically: the CSSWG resolved that "min-height:min-content" (or any other content-based value) does not influence percent height calculations on children -- the children's percent heights are resolved w/out considering that min-height on the parent.

Effectively, this means that a content-based "min-height" has no influence on whether its dimension is considered "definite".

So, to answer my question from comment 2:
> I'm not clear on what the right behavior is here, or related cases, e.g. if
> we gave the flex item in this testcase a flex-basis of a fixed size like
> "10px" instead of the default, "auto".  It's unclear to me whether that flex
> item's final size should be considered "definite" [& usable for
> percent-height resolution on its children], given that its height
> technically depends on its children. (It gets its height here via
> "min-width:auto", which shrinkwraps its children.)

It sounds like we should just follow the letter of the spec here, and treat the flexed height as "definite" IFF the flex-basis was definite. So in testcase 2, the flex item's flex-basis is indefinite, which means its resulting flexed size should be treated as indefinite.

And in my hypothesized alternate testcase with "flex-basis:10px", the flex basis is *definite*, which means *its* resulting flexed size should be treated as *definite*.

This is a bit silly, because in both cases the flexed size is primarily determined by the auto "min-height" and not by the "flex-basis".   But I suppose it's consistent & grokable, at least.

Comment 5

[Daniel Holbert [:dholbert]]

(Posted https://lists.w3.org/Archives/Public/www-style/2015Feb/0489.html after some more thought; I still think we might want to change the spec.)

Comment 6

[Daniel Holbert [:dholbert]]

[Tracking Requested - why for this release]:
Regression/inconsistency from a change in the Firefox 38 timeframe. I'm aiming to fix this on Nightly soon & we'll want to backport that to Aurora38 so we don't ship with this.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking as a regression and tracking so we ensure this gets uplifted when ready.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Daniel are you still working on this and intend to get it into 38 while it's still in aurora? Is there still discussion happening on what the spec should be?

Comment 9

[Daniel Holbert [:dholbert]]

Yeah, sorry. I'm still waiting to hear back on www-style about this, but in the meantime we should preserve backwards-compatibility here (and continue matching the current spec & fantasai's recommendation per comment 4 here).

I've got a patch which I'll post shortly.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix v1

So the idea here is, if any of our descendants have a relative height, then doing this second reflow *with an imposed (instead of "auto") height* _will_ affect that's descendant's height.  Even if our size hasn't changed, the fact that it's now definite means our kids will behave differently w.r.t. percent-resolution.  (So, our 2nd-reflow-avoiding optimization is invalid in that case, and we bypass it.)

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8580341 [details] [diff] [review]
fix v1

This makes sense to me.  r=mats

A couple of nits:

>+          // wasn't in our previous "measuring" reflow], if & they have a

"if & they", typo?

>+          if (!item->Frame()->HasAnyStateBits(
>+                 NS_FRAME_CONTAINS_RELATIVE_HEIGHT)) {

I'd just let this be > 80 columns to avoid the formatting ugliness.

Comment 12

[Daniel Holbert [:dholbert]]

Thanks for the review!

I fixed the typo, and I changed the state-bit check to just use "GetStateBits() &", so that it's more wrappable to a newline without violating coding-style. (and because that's how we usually check this bit anyway) (I normally default to using HasAnyStateBits(), since a function-call is more foolproof than  boolean arithmetic, but in this case it's a simple-enough check, so I just reverted to GetStateBits().)

Try run:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=257e782dee4e
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d111d64d9f0f

needinfo=me to request backport approval tomorrow.

Comment 13

[Daniel Holbert [:dholbert]]

This was flagged as causing a small (2-3%) regression in "Customization Animation" tests, as noted here:
  https://groups.google.com/forum/#!topic/mozilla.dev.tree-alerts/MkctEZYELQU

Really, what must've happened is that bug 1054010 *greatly improved* our perf on these tests (likely at the cost of some correctness), and then this bug's patch reined in the optimization a bit to get back the lost correctness.

So, I don't think this perf regression is something we should back out over [particularly given that the reinded-in-optimization hasn't made it very far in the release channels yet -- it's just on Aurora]. But I did file bug 1145820 to see if we can make the Customization UI more friendly to this optimization.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8580341 [details] [diff] [review]
fix v1

Approval Request Comment
[Feature/regressing bug #]: Bug 1054010 (an optimization)

[User impact if declined]: Broken layout on sites / webapps that have percent-height elements inside of flex items, in some cases.

[Describe test coverage new/current, TreeHerder]: We have pretty solid flexbox tests. (They didn't cover this particular case, but they do now; the patch includes a regression test.)

[Risks and why]: As noted in previous comment, this will likely have a small perf cost in some cases (with respect to current Aurora), due to restricting bug 1054010's optimization a bit. However, these cases should still be at least as fast as they are in Beta/Release, though (which don't have bug 1054010's optimization at all).

[String/UUID change made/needed]: None.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> [User impact if declined]: Broken layout on sites / webapps that have
> percent-height elements inside of flex items, in some cases.

One example of this: bug 1145868 (broken customize-mode UI, caused by bug 1054010's optimization, after I fixed a broken piece of it in Bug 1142686)

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d111d64d9f0f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0936a835649d

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Created attachment 8561675 [details]
> testcase 2
> 
> Here's a simpler testcase, with only a single (vertical) flex container.
> 
> I'm not clear on what the right behavior is here, or related cases, e.g. if
> we gave the flex item in this testcase a flex-basis of a fixed size like
> "10px" instead of the default, "auto".  It's unclear to me whether that flex
> item's final size should be considered "definite" [& usable for
> percent-height resolution on its children]

For the record -- the spec has now changed here a bit. See bug 1161771.