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

VERIFIED FIXED in Firefox 61

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
9 days ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Depends on 1 bug, Blocks 2 bugs, Regressed 1 bug)

Trunk
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
webcompat +

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox61 verified)

Details

(Whiteboard: [webcompat:p1])

User Story

Business driver: Achieve tier-1 Google Search experience for Gecko on Android

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8879411 - Attachment description: testcase 1 → testcase 1 ("flex:none" thing is itself a wrappable flexbox)
(Assignee)

Comment 2

2 years ago
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.
Priority: -- → P3

Updated

2 years ago
Flags: webcompat?
Whiteboard: [webcompat]
Flags: webcompat? → webcompat+
Whiteboard: [webcompat] → [webcompat:p1]
(Assignee)

Updated

a year ago
See Also: → 1441390
(Assignee)

Comment 3

a year ago
(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".
Depends on: 1105111
User Story: (updated)
(Assignee)

Comment 4

a year ago
I've got a WIP patch locally -- assigning.
Assignee: nobody → dholbert
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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
Attachment #8967837 - Flags: review?(mats) → review+

Comment 10

a year ago
mozreview-review
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
Attachment #8967838 - Flags: review?(mats) → review+

Comment 11

a year ago
mozreview-review
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
Attachment #8967840 - Flags: review?(mats) → review+

Comment 12

a year ago
mozreview-review
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?
Attachment #8967839 - Flags: review?(mats) → review+
(Assignee)

Comment 13

a year ago
mozreview-review-reply
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).
(Assignee)

Updated

a year ago
Blocks: 1454069
(Assignee)

Comment 14

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 15

a year ago
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

a year ago
Pushed by dholbert@mozilla.com:
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 18

a year ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

a year ago
Duplicate of this bug: 1282821
(Assignee)

Updated

a year ago
Blocks: 1377253
Depends on: 1455976
I noticed some breakage after the part4 patch. Would you take a look if this is expected? Testcase: https://gist.github.com/kasper93/c27bb264d409128d3fd1a5c683fecf0d
Flags: needinfo?(dholbert)
(Assignee)

Updated

11 months ago
Depends on: 1461446
(Assignee)

Comment 21

11 months ago
Indeed, that looks like a regression. I filed bug 1461446 and transferred my needinfo over there.
Flags: needinfo?(dholbert)
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!
Flags: needinfo?(dholbert)
(Assignee)

Comment 23

11 months ago
I think it's accurate, assuming I'm reading it correctly -- see my comment on the pull request.
Flags: needinfo?(dholbert)
(Assignee)

Updated

10 months ago
Depends on: 1469649

Updated

10 months ago
Depends on: 1474277
You need to log in before you can comment on or make changes to this bug.