1374540 - [css-flexbox] Wrappable (but "flex:none") content inside flex container is forced to wrap in Firefox, but not in other browsers

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 ("flex:none" thing is itself a wrappable flexbox)

STR:
 1. Load attached testcase.

ACTUAL RESULTS:
Text wraps.

EXPECTED RESULTS:
Text should not wrap.

The text is part of a "flex:none" element, inside of a skinny flex container.

The flexbox spec says that:
 * we should give the flex:none thing its content width for a flex basis (since flex:none implies flex-basis:auto, which triggers content sizing if "width" is at its default "auto" value)
 * we should not shrink the flex:none thing below that size (since flex:none implies flex-shrink:0)

I think the problem is that we do our flex-basis content measurement *using the flex container's size as the available size*, so we already assume the text is going to be wrapped at that early point.  But we're supposed to pretend there's infinite available space when we do that measuring reflow, IIRC.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 ("flex:none" thing is just a block with two words)

Comment 2

[Daniel Holbert [:dholbert]]

This bug is implicated in a Google Flight Search interoperability bug:
 https://github.com/webcompat/web-bugs/issues/7351

I think this may have been reported before, but I can't find any older dupes at the moment.

This behavior was from a spec change at some point --  I recall older flexbox spec versions requiring our current behavior, though I know it changed at some point.  e.g. this snapshot's overview of the layout algorithm...
  https://www.w3.org/TR/2011/WD-css3-flexbox-20111129/#layout-algorithm
...says that, to find the hypothetical size, "Pretend that the flexbox item is the only child of the flexbox".  That's basically what we're doing here, but we're not supposed to anymore.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> The flexbox spec says that:
>  * we should give the flex:none thing its content width for a flex basis
> (since flex:none implies flex-basis:auto, which triggers content sizing if
> "width" is at its default "auto" value)

Just to ground this ^^^ a bit more concretely in the spec: the relevant chunk is 9.2 part 3:
> 3. Determine the *flex base size* and *hypothetical main size* of each item:
>   [...]
>   E. ... size the item into the available space, using its used flex basis in place of its main size, treating a value of 'content' as 'max-content'
https://drafts.csswg.org/css-flexbox-1/#algo-main-item

This example triggers this spec text -- our flex item is indeed supposed to have a *used* flex-basis of 'content' here (which we'll support as of bug 1105111).  (It has computed 'flex-basis:auto', but that's defined as having a used value of 'content' in this case where the main-size property is 'auto' also: https://drafts.csswg.org/css-flexbox-1/#valdef-flex-basis-auto )

So we should be recognizing that we've got a value of 'content' and treating that as 'max-content', which means we shouldn't care about the available space.  (But instead, we're currently using the fit-content size and being deferential to the available space.)

(Side note: we do need to *support* using the fit-content size (and wrapping to the available space) in the specific scenario where the author asks for it -- e.g. via "flex-basis:auto; width: -moz-fit-content", "flex-basis: -moz-fit-content".

Comment 4

[Daniel Holbert [:dholbert]]

I've got a WIP patch locally -- assigning.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1374540 part 1: Add reftests to verify that used 'flex-basis:content' resolves to the max-content size on flex items in row-oriented flex container.

Note: These tests fail in current mozilla-central (and hence are marked as
failing), but they start passing as of a patch later on in this series.

Review commit: https://reviewboard.mozilla.org/r/236540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236540/

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1374540 part 2: Add reftest variants with column-oriented flex container (to verify that used 'flex-basis:content' is treated as max-content).

This patch's reftests already pass, regardless of whether we have this bug's
fix, because the max-content size in the block axis is the same as the "auto" size
(which is what we were already using in this scenario). I'm just adding these reftests
 for symmetry & completeness.

Review commit: https://reviewboard.mozilla.org/r/236542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236542/

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1374540 part 3: Refactor nsFrame::ComputeSize methods to handle the two "used flex-basis of content" scenarios with a consistent codepath.

This patch doesn't change behavior.

It simply makes us share code/data for two different cases that both ended up
producing mainAxisCoord->GetUnit() == eStyleUnit_Auto.  Now, they'll *both* use
the same static nsStyleCoord to represent this "auto" value.

Originally, in one of these cases ("flex-basis:auto;[main-size-property]:auto),
we left the mainAxisCoord untouched. Now we'll point it at this dummy 'auto'
value. Either way we end up with mainAxisCoord->GetUnit() == eStyleUnit_Auto,
so the behavior doesn't change.

The next patch in this series will make further changes to one of these spots,
as noted in the "XXXdholbert" code-comment included here.

Review commit: https://reviewboard.mozilla.org/r/236544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236544/

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1374540 part 4: Change nsFrame::ComputeSize to treat used flex-basis:content as 'max-content'.

This brings us into alignment with the spec and makes us pass some web-platform
tests, along with the reftests that I've included for this bug.

Review commit: https://reviewboard.mozilla.org/r/236546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236546/

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8967837 [details]
Bug 1374540 part 1: Add reftests to verify that used 'flex-basis:content' resolves to the max-content size on flex items in row-oriented flex container.

https://reviewboard.mozilla.org/r/236540/#review242322

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8967838 [details]
Bug 1374540 part 2: Add reftest variants with column-oriented flex container (to verify that used 'flex-basis:content' is treated as max-content).

https://reviewboard.mozilla.org/r/236542/#review242324

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8967840 [details]
Bug 1374540 part 4: Change nsFrame::ComputeSize to treat used flex-basis:content as 'max-content'.

https://reviewboard.mozilla.org/r/236546/#review242326

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8967839 [details]
Bug 1374540 part 3: Refactor nsFrame::ComputeSize methods to handle the two "used flex-basis of content" scenarios with a consistent codepath.

https://reviewboard.mozilla.org/r/236544/#review242320

::: layout/generic/nsFlexContainerFrame.cpp:4437
(Diff revision 1)
> +  // We have a used flex-basis of 'content' if flex-basis explicitly has that
> +  // value, OR if flex-basis is 'auto' (deferring to the main-size property)
> +  // and the main-size property is also 'auto'.
> +  // See https://drafts.csswg.org/css-flexbox-1/#valdef-flex-basis-auto

How about the case where flex-basis is 'auto' and the main-size property is a <percentage> and the percentage basis is indefinite?
Should that make the main-size "behave as auto" and thus return true here instead of false?
Do we have a reftest that covers that case?

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8967839 [details]
Bug 1374540 part 3: Refactor nsFrame::ComputeSize methods to handle the two "used flex-basis of content" scenarios with a consistent codepath.

https://reviewboard.mozilla.org/r/236544/#review242320

> How about the case where flex-basis is 'auto' and the main-size property is a <percentage> and the percentage basis is indefinite?
> Should that make the main-size "behave as auto" and thus return true here instead of false?
> Do we have a reftest that covers that case?

Ooh, good question.

Two-part answer:
 (1) flex-basis itself accepts percentages, so I think it's natural that we just clone [main-size]:<percentage> into flex-basis, and not pre-resolve the main-size property before doing that cloning.  So: no special checks should be needed on the main-size property (RE percentages) before deciding how to clone it into flex-basis.

 (2) It looks like we do need some special checks on the "used flex-basis" (wherever it comes from) RE percentages and indefinite sizes, though!  I just noticed this nugget in the spec:
# ...percentage values of flex-basis are resolved against
# the flex item’s containing block (i.e. its flex
# container); and if that containing block’s size is
# indefinite, the used value for 'flex-basis' is 'content'.
https://drafts.csswg.org/css-flexbox-1/#propdef-flex-basis

So I think technically, IsUsedFlexBasisContent probably needs to take the CB Size (for the main axis), and it should return true if the flex-basis (or deferred-to main-size-property) has a percent AND the CB Size is indefinite.

From a quick test, I don't think this is what Chrome does, actually -- they seem to resolve the flex base size to 0 in this case with percentages & indefinite sizes (and we do the same). So there might be some interop issues for this part.  For that reason, I'm not going to try to address this in this bug here -- instead, I'll spin off a followup bug for further investigation & so that we can reason independently about these distinct changes (one of which helps with interop, & one of which might hurt slightly).

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8967839 [details]
Bug 1374540 part 3: Refactor nsFrame::ComputeSize methods to handle the two "used flex-basis of content" scenarios with a consistent codepath.

https://reviewboard.mozilla.org/r/236544/#review242320

> Ooh, good question.
> 
> Two-part answer:
>  (1) flex-basis itself accepts percentages, so I think it's natural that we just clone [main-size]:<percentage> into flex-basis, and not pre-resolve the main-size property before doing that cloning.  So: no special checks should be needed on the main-size property (RE percentages) before deciding how to clone it into flex-basis.
> 
>  (2) It looks like we do need some special checks on the "used flex-basis" (wherever it comes from) RE percentages and indefinite sizes, though!  I just noticed this nugget in the spec:
> # ...percentage values of flex-basis are resolved against
> # the flex item’s containing block (i.e. its flex
> # container); and if that containing block’s size is
> # indefinite, the used value for 'flex-basis' is 'content'.
> https://drafts.csswg.org/css-flexbox-1/#propdef-flex-basis
> 
> So I think technically, IsUsedFlexBasisContent probably needs to take the CB Size (for the main axis), and it should return true if the flex-basis (or deferred-to main-size-property) has a percent AND the CB Size is indefinite.
> 
> From a quick test, I don't think this is what Chrome does, actually -- they seem to resolve the flex base size to 0 in this case with percentages & indefinite sizes (and we do the same). So there might be some interop issues for this part.  For that reason, I'm not going to try to address this in this bug here -- instead, I'll spin off a followup bug for further investigation & so that we can reason independently about these distinct changes (one of which helps with interop, & one of which might hurt slightly).

(Er -- sorry, disregard my report about the Chrome behavior -- I was using a [tiny] definite size on the container without realizing it in my quick-and-dirty spot test there.)

I think we basically get this correct, because even if we don't trigger the "nsFlexContainerFrame::IsUsedFlexBasisContent" check, our percent value + indefinite CB size will still make us return true from IsAutoBSize(...) a little ways down, so we end up producing the same behavior anyway.  (At least, this is what happens in the block axis -- and IIRC we only allow ourselves to have indefinite CB sizes in the block axis anyway, for this ComputeSize code.)

Anyway, I spun off helper bug 1454069 to be sure and to add reftests for this.

Comment 15

[Daniel Holbert [:dholbert]]

Here are a few Try run links to demonstrate that this is landable (after which I made only non-functional edits to comments & commit messages & such).

Reftests green, including the new ones (and some WPT orange because I hadn't removed all the necessary .ini files for unexpectedly-passing tests yet):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be27c4d715bc0619307a9e6bb5f26c67a018f5a&selectedJob=173325425

WPT now green, after updating patches to remove those .ini files for newly-passing tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60a71e6e3c60d2465eb4adecd28f5038e2eec1f1

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/944a3acec62e
part 1: Add reftests to verify that used 'flex-basis:content' resolves to the max-content size on flex items in row-oriented flex container. r=mats
https://hg.mozilla.org/integration/autoland/rev/b1c6a20217fb
part 2: Add reftest variants with column-oriented flex container (to verify that used 'flex-basis:content' is treated as max-content). r=mats
https://hg.mozilla.org/integration/autoland/rev/30e0cdcd531a
part 3: Refactor nsFrame::ComputeSize methods to handle the two "used flex-basis of content" scenarios with a consistent codepath. r=mats
https://hg.mozilla.org/integration/autoland/rev/a6a21ea56a9c
part 4: Change nsFrame::ComputeSize to treat used flex-basis:content as 'max-content'. r=mats

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/944a3acec62e
https://hg.mozilla.org/mozilla-central/rev/b1c6a20217fb
https://hg.mozilla.org/mozilla-central/rev/30e0cdcd531a
https://hg.mozilla.org/mozilla-central/rev/a6a21ea56a9c

Comment 18

[Levente Sacal]

Marked as verified in Nightly 61.0a1 (2018-04-15) as per testcase1 and testcase2. The text is not wrapped and it appears in one line.

Comment 20

[Kacper Michajłow [:kasper93]]

I noticed some breakage after the part4 patch. Would you take a look if this is expected? Testcase: https://gist.github.com/kasper93/c27bb264d409128d3fd1a5c683fecf0d

Comment 21

[Daniel Holbert [:dholbert]]

Indeed, that looks like a regression. I filed bug 1461446 and transferred my needinfo over there.

Comment 22

[Will Bamberg [:wbamberg]]

For the MDN docs, I've tried to note this in the compat data here: https://github.com/mdn/browser-compat-data/pull/2151

Would you mind taking a look to see if this is an accurate summary? Thanks!

Comment 23

[Daniel Holbert [:dholbert]]

I think it's accurate, assuming I'm reading it correctly -- see my comment on the pull request.