903880 - Vertical flex containers get the incorrect height, if they have a stretched child whose height changes when it's given a width larger than its preferred width (e.g. due to percent-width on a descendant)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: test.html

The simple page I'm attaching should just display two tabs with a shadow directly underneath them. Every browser supporting Flexbox in it's `display: flexbox` or `display: flex` incarnation (potentially prefixed) displays it this way except for Firefox which creates a significant gap between the tabs and the shadows.

This means the example page works fine in IE10, Chrome stable & Canary, WebKit nightly, Opera both Presto & Blink-based. It displays incorrectly in all 4 Firefox channels

I wasn't able to reduce the test case much more, removing any of the tags seems to stop the issue from happening as well as replacing links with spans.

Comment 1

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: Chrome screenshot - correct rendering

Comment 2

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: Firefox screenshot - incorrect rendering

Comment 3

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: Firefox screenshot - incorrect rendering

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a simplified testcase. Firefox shows some red below "A B C"; Chrome and Opera (w/ Presto) do not.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (now w/ closing tags fixed)

Comment 6

[Daniel Holbert [:dholbert]]

I suspect we're doing what the spec implicitly calls for, but Chrome & Opera are doing something more intelligent, and we probably should match them. (and get the spec fixed)

Basically, it has to do with the question of whether "align-self: stretch" (the default value) should have any effect on the main-axis sizing of flex items, specifically for "vertItem" in my attached "testcase 2". The spec suggests it should not have any effect -- its algorithm establishes the main-axis size before it resolves align-self:stretch. Gecko follows that ordering and establishes the main-axis size before stretching in the cross axis. (In this case, that means we end up making <div class="vertItem"> as tall as several lines of text; then we stretch it horizontally, and its content no longer occupies its full height anymore.)  In contrast, Chrome and Opera(Presto) make vertItem tall if it's got align-self:flex-start, and short if it's got align-self:stretch -- this means they *are* letting "align-self:stretch" affect the main-size.

I think their behavior probably makes sense.  The sanest way to get that behavior, for vertical flex items with "width:auto; height:auto" in a definite-width flex container, is to figure out the stretched computed width before we resolve the auto-height.  I've got a WIP fix that basically does that & appears to work -- I'll post it for review in the next day or so.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (comparing to "align-self:flex-end")

Here's a testcase that demonstrates what I'm talking about.

Chrome & Opera both render the first container (and its sole flex item) as being the height of a single line of text, whereas they render the second container (and its sole flex item) as being the height of 3 lines of text.

I think that technically violates the spec.  The only difference between the two examples in this testcase is the presence of "align-self: flex-end" on the second container's flex item (vs. the implied "align-self:stretch" on the first container's flex item).  And we're supposed to establish the main size (the height, in this case) *before* we take "align-self" into consideration.

Gecko is doing that, which is why it renders both examples as being the same height (even though that's taller than it ends up needing to be in the first example, since the "align-self:stretch" gives us some bonus horizontal space to work with. But importantly, we only find out about that bonus space *after* we've already locked in the flex item's height).

Comment 8

[Daniel Holbert [:dholbert]]

So my proposed way to handle this was inspired by this chunk of spec text (which is the one place where "stretch" is mentioned before main-size resolution, I think):

> For the purposes of these definitions, a definite size is [...].
> If a single-line flex container has a definite cross size, any flex items with
> ‘align-self: stretch’ must also be treated as having a definite cross size
> (since its size is determined from the size of the flex container

So that spec chunk lays out some circumstances under which we treat flex items as if they have a "definite" cross size (for the purpose of the flex layout algorithm, which has some things conditioned off of whether sizes are definite or not).

To fix this bug, I think we need to *calculate* that definite cross size ahead of time, and then *actually use it* as the computed width, when we resolve the flex base size (i.e. when we resolve the auto-height).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 1: Add a setter for flex item's base size

Posting my patch-stack (of 5 smallish patches) for this, and then I'll post a brief comment with the big picture & how they fit together.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 2: Split out code for "reflow to resolve auto height, to establish flex base size" into its own function

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: part 3: move ResolveStretchedCrossSize() method to live on FlexItem

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: part 4: [the fix] Resolve stretched cross-sizes (widths) on vertical FlexItems earlier, and use that to help resolve their main size

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 5: reftests

This adds two reftests to our w3c flexbox reftest directory (since that's where most of our flexbox reftests live, and as long as the csswg agrees that this behavior is desirable, we might as well enforce it in their test suite).

Comment 14

[Daniel Holbert [:dholbert]]

OVERVIEW OF PATCH STACK:
========================
The goal of the patch-stack is to do what I laid out in comment 8: figure out the stretched cross-size (width) *early*, and use that as our computed width when calculating the flex item's auto-height, when we're establishing its flex base size.

HOWEVER, we can't do that directly, because right now we establish the flex base size *before* we construct a FlexItem, and we can't resolve the stretched cross-size before that, because that code requires us to already have a FlexItem (as a convenient package for all the inputs of stretched-size resolution).

SO: Parts 1 through 3 are refactoring patches to make it possible for us to interject and resolve the stretched cross-size (width) at the right time, and then part 4 actually does that interjecting.

In particular...
* Patch 1 (attachment 791578 [details] [diff] [review]) adds a setter for the base size.
* Patch 2 (attachment 791579 [details] [diff] [review]) then splits out the "resolve auto height" code to happen *after* we've constructed our flex item, instead of before. (and then it updates the flex item with the setter from Patch 1.) This one is essentially just moving a bunch of existing code into its own function.
* Patch 3 (attachment 791581 [details] [diff] [review]) refactors the existing ResolveStretchedSize() method so that it only requires a FlexItem. (Currently, it's a method on one of the position-tracker objects, but it doesn't need to live there, so I'm moving it to FlexItem so that we can call it earlier.)
* Patch 4 (attachment 791585 [details] [diff] [review]) fixes this bug by inserting a call to ResolveStretchedSize() after we've created the FlexItem, just before we resolve its auto-height, and it uses the resulting stretched width as the flex item's computed width when we calculate that auto-height. (This is what I described in comment 8, and it fixes this bug.)
* Patch 5 (attachment 791588 [details] [diff] [review]) adds two reftests, similar to the two examples in "testcase 3" on this bug.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Daniel, are spec changes also needed here?

Comment 16

[Daniel Holbert [:dholbert]]

I believe so. I emailed the CSSWG to ask about spec changes last night:
 http://lists.w3.org/Archives/Public/www-style/2013Aug/0360.html

Comment 17

[Michał Gołębiowski-Owczarek [:mgol]]

What's the status of this bug? Is the spec change going into effect?

Comment 18

[Daniel Holbert [:dholbert]]

Waiting on the spec-editors to confirm that the spec doesn't address this situation & respond to my proposed spec-change.

Tab replied a few weeks ago saying that he & fantasai think the current spec is correct & matches the Opera/Chrome behavior (but he said it was late and they might be missing something):
 http://lists.w3.org/Archives/Public/www-style/2013Sep/0222.html

...and I replied to summarize why I disagree:
 http://lists.w3.org/Archives/Public/www-style/2013Sep/0224.html

...and I've been waiting to hear back. I just posted a followup message to ask again:
 http://lists.w3.org/Archives/Public/www-style/2013Sep/0852.html

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 791579 [details] [diff] [review]
part 2: Split out code for "reflow to resolve auto height, to establish flex base size" into its own function

>-  // Returns nsresult because we might have to reflow aChildFrame (to get its
>-  // vertical intrinsic size in a vertical flexbox), and if that reflow fails
>-  // (returns a failure nsresult), we want to bail out.
>-  nsresult AppendFlexItemForChild(nsPresContext* aPresContext,
>-                                  nsIFrame* aChildFrame,
>-                                  const nsHTMLReflowState& aParentReflowState,
>-                                  const FlexboxAxisTracker& aAxisTracker,
>-                                  nsTArray<FlexItem>& aFlexItems);
>+  nsresult ResolveFlexItemMaxContentSizing(nsPresContext* aPresContext,
>+                                           FlexItem& aFlexItem,
>+                                           const nsHTMLReflowState& aParentReflowState,
>+                                           const FlexboxAxisTracker& aAxisTracker);

The comment seems like it could stay, since it applies just as much
to the new function as the old.


Should the commit message say something about the removal of the checks
that flex-grow and flex-shrink are both 0?  I didn't follow why those
checks were removed -- do you need to do this reflow when there's no
flexibility?

r=dbaron

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 791581 [details] [diff] [review]
part 3: move ResolveStretchedCrossSize() method to live on FlexItem

The change to the Reflow method seems to imply that a spec change is needed to match.  I didn't see such a change in your proposal, but I was looking quickly.  Could you make sure an appropriate spec change is on the table?

r=dbaron

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 791585 [details] [diff] [review]
part 4: [the fix] Resolve stretched cross-sizes (widths) on vertical FlexItems earlier, and use that to help resolve their main size

Again, please make sure we end up sync'd with the spec one way or another.

r=dbaron

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 791588 [details] [diff] [review]
part 5: reftests

The "Testcase to check" comment paragraphs in both tests (but not in the references) could probably go in a <meta name="assert"> in both tests (but again, not the references) without the initial "Testcase to check".  Then it'll get picked up as standard CSS test suite metadata.

r=dbaron

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: Split out code for "reflow to resolve auto height, to establish flex base size" into its own function [r=dbaron]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #19)
> >-  // Returns nsresult because we might have to reflow aChildFrame (to get its
[...]
> The comment seems like it could stay, since it applies just as much
> to the new function as the old.

Good point. I reverted the comment-removal (with the mention of "aChildFrame" fixed to match the new function's param-name)

> Should the commit message say something about the removal of the checks
> that flex-grow and flex-shrink are both 0?  I didn't follow why those
> checks were removed -- do you need to do this reflow when there's no
> flexibility?

I've now added those back, so this review-comment no longer applies.

(That whole clause with the flex values being 0 is just an optimization, so tweaking it doesn't affect behavior.  The "resolve flexible sizes" code will freeze those items, too, but it might take a few cycles to do it in pathological cases -- so that clause is there to speed things up by freezing known-freezable things up-front and save us a few cycles through the flex items.

The previous patch removed the early-freeze for 0-flexibility things because it was making us fail a sanity-check assertion (added in part 1) about tweaking the main size after we're frozen.  That assertion was failing because this patch adds a place where we *do* tweak the main size (to resolve an auto height) after that early freeze. So to prevent the assertion failure, I disabled the early freeze.

But that's kind of silly; we might as well keep the optimization, and just allow ourselves to skip the assertion if we're resolving an auto-height. So, that's what this updated patch-version does (via a NS_INTRINSICSIZE check).)

(I made one other tweak in this version: now, I append the returned FlexItem directly to our nsTArray, where the previous patch-version would store the FlexItem in a local var and then append that var to the array. This tweak reduces needless copying. I don't think this requires additional review, so carrying forward r+.)

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: part 3 v2: move ResolveStretchedCrossSize() method to live on FlexItem [r=dbaron]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #20)
> Comment on attachment 791581 [details] [diff] [review]
> part 3: move ResolveStretchedCrossSize() method to live on FlexItem
> 
> The change to the Reflow method seems to imply that a spec change is needed
> to match.

This patch doesn't actually change reflow -- it just moves some code up one level. (Previously, PositionItemInCrossAxis() called ResolveStretchedCrossSize(), but now we just have Reflow call it right before it calls PositionItemInCrossAxis.)

So, no spec-change modification is needed for this part.

(I'm making one minor change here, though -- hence the repost. The previous version of this patch removed a MOZ_ASSERT, but I'm taking out that change and doing it instead in part 4, since that's where the assertion actually becomes broken (due to early stretched-cross-size resolution)).

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: part 4 v2: [the fix] Resolve stretched cross-sizes (widths) on vertical FlexItems earlier, and use that to help resolve their main size [r=dbaron]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #21)
> Again, please make sure we end up sync'd with the spec one way or another.

Haven't heard definitely back yet about the spec edit -- last update was
  http://lists.w3.org/Archives/Public/www-style/2013Sep/0853.html
Note also that it is at least on the issues list (#23), here:
  http://dev.w3.org/csswg/css-flexbox/issues-cr-2012

I'll send another followup post to the list, but in the meantime, I think I'm going to proceed with landing this, for interop & doing-what-authors-expect reasons.  (Note that a dupe was filed in the last few days -- bug 931757 -- which indicates that this is a problem that authors are indeed running up against in the real world. So, probably better to land the fix and let the spec change happen when it happens.

In the (hopefully) unlikely event that the spec change is rejected, we can easily switch back to the old behavior without worrying about bitrot by just reverting part 4, since all the other patches here are code-refactoring with no behavior-change.

Anyway -- here's an updated version of part 4, unbitrotted & with the MOZ_ASSERT-removal that I moved from part 3, per end of comment 25.

Comment 27

[Daniel Holbert [:dholbert]]

oops, I attached copies of parts 2 and 3 from the wrong copy of my patch queue, so they didn't have the (trivial) shift in MOZ_ASSERT-removal described at the ends of comment 25 / comment 26. Reposting w/ correct patch versions.

Comment 28

[Daniel Holbert [:dholbert]]

Attachment: part 3 v3: move ResolveStretchedCrossSize() method to live on FlexItem [r=dbaron]

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: part 4 v3: [the fix] Resolve stretched cross-sizes (widths) on vertical FlexItems earlier, and use that to help resolve their main size [r=dbaron]

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: part 5 v2: reftests [r=dbaron]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #22)
> The "Testcase to check" comment paragraphs in both tests (but not in the
> references) could probably go in a <meta name="assert"> in both tests (but
> again, not the references) without the initial "Testcase to check".  Then
> it'll get picked up as standard CSS test suite metadata.

Thanks - I made that change (with an all-new concise high-level description of the thing being tested) in the first test.

I ended up just removing the second test (flexbox-align-self-stretch-vert-2.html) because it doesn't use align-self:stretch. (It was sort of intended as an informative "notref" for the first test (demonstrating that 'stretch' is special), but it doesn't really belong, so I'm just taking it out.)

Comment 31

[Daniel Holbert [:dholbert]]

https://tbpl.mozilla.org/?tree=Try&rev=85feb5b1ffdc

Comment 32

[Daniel Holbert [:dholbert]]

Final try run:
 https://tbpl.mozilla.org/?tree=Try&rev=abb7329ebc02

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da39672c243
https://hg.mozilla.org/integration/mozilla-inbound/rev/fba3076a7217
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1150c824f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/12161d958d0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3d2de2bebe

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1da39672c243
https://hg.mozilla.org/mozilla-central/rev/fba3076a7217
https://hg.mozilla.org/mozilla-central/rev/3b1150c824f9
https://hg.mozilla.org/mozilla-central/rev/12161d958d0c
https://hg.mozilla.org/mozilla-central/rev/7c3d2de2bebe