flexbox align-content handling & single-line-sizing should depend on "flex-wrap", not number of lines

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Fantasai proposes in [1] that we honor "align-content" for flex containers that have "flex-wrap:wrap" even if they only have a single-line. (So, e.g. we center that single line, if there's "align-content:center".)

She also proposes that we only force a single line to take the container's cross-size if it's actually "flex-wrap: nowrap" -- again, not for single-line wrappable flex containers.

I think both changes probably make sense.

I think both changes would be addressed by changing the condition for the early-return from the CrossAxisPositionTracker constructor, at [2]. We probably should check the computed style & only return if we have "flex-wrap: nowrap".

[1] http://lists.w3.org/Archives/Public/www-style/2014Oct/0489.html
[2] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=366b01141df5&mark=2457-2467#2457
(Assignee)

Comment 1

3 years ago
I believe this spec change took effect. The relevant spec text now reads:
 # If the flex container is single-line and has a definite cross size,
 # the cross size of the flex line is the flex container’s inner cross size. 
https://drafts.csswg.org/css-flexbox-1/#algo-cross-line

And "single-line" there is linked to this definition:
 # A single-line flex container (i.e. one with flex-wrap: nowrap)
https://drafts.csswg.org/css-flexbox-1/#single-line
Blocks: 1055888
(Assignee)

Updated

2 years ago
Assignee: nobody → dholbert
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
(The test changes here are updating the reference case.

Currently, the reference case is set to expect that all of the scenarios with 1 flex line have that line aligned at the start (and technically stretched, though that's not visible since the item inside the line isn't stretched).  With this change, I'm updating the reference cases to actually expect that the "align-content:center" 1-flex-line scenario will *actually* be centered, etc.

(I copypasted the margin-top/margin-left offsets from the equivalent "justify-content" testcases, which look & behave pretty much the same.)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8800015 [details]
Bug 1090031: Apply CSS 'align-content' in flex containers if they *could* wrap (rather than if they *have* wrapped).

https://reviewboard.mozilla.org/r/85036/#review83784

::: layout/generic/nsFlexContainerFrame.cpp:2752
(Diff revision 1)
> +  const bool isSingleLine =
> +    NS_STYLE_FLEX_WRAP_NOWRAP == aReflowInput.mStylePosition->mFlexWrap;

nit: move this to the line before its first use
Attachment #8800015 - Flags: review?(mats) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Thanks! Addressed that. I've triggered a try run via mozreview; will land if that looks good.

Comment 8

2 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37e0c017b268
Apply CSS 'align-content' in flex containers if they *could* wrap (rather than if they *have* wrapped). r=mats
(Assignee)

Comment 9

2 years ago
Hit some test failures on Autoland TreeHerder which didn't appear on Try -- that's because the reference case *just* got a new chunk, in https://hg.mozilla.org/integration/autoland/rev/ffca0f4dcd4e (for Bug 1221565), and I need to update that new chunk as well (per comment 4 here).
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
My Try run from comment 12 completed without running any tests, for some reason.  The syntax I used ("try: -b do -p linux64 -u reftest -t none") seems good & matches what I get from TryChooser, so I have no idea what happened there...

Anyway, I'm going to re-land because I'm pretty confident this is fixed & Try is not being helpful.
Flags: needinfo?(dholbert)
(Assignee)

Comment 14

2 years ago
(oh, looks like linux reftests are just hidden at the moment. Here's a Try link with them un-hidden, which looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfc7a4f87574&exclusion_profile=false
)

Comment 15

2 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c14eb1c34961
Apply CSS 'align-content' in flex containers if they *could* wrap (rather than if they *have* wrapped). r=mats
Yeah, the default exclusion profile on Try that hides Linux reftests confused me too
before I could figure it out.  We should probably file a bug about that...

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c14eb1c34961
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.