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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

For reference:
https://drafts.csswg.org/css-flexbox/#change-201409-align-content-wrapping
https://bugs.chromium.org/p/chromium/issues/detail?id=599828

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1090031: Apply CSS 'align-content' in flex containers if they *could* wrap (rather than if they *have* wrapped).

The spec says that single-line flex containers should stretch their one flex
line to the flex container's cross size, and should ignore 'align-content'.

Initially, the spec defined 'single-line' to include any flex container that
happens to have only 1 line (even if it's got 'flex-wrap:wrap' or
'wrap-reverse'). But later, the term 'single-line' was intentionally redefined
to *only* include flex containers that have 'flex-wrap: nowrap'.  So, instead
of checking the line-count, we should instead check 'flex-wrap', when deciding
whether to stretch our one line & ignore 'align-content'.

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

Comment 4

[Daniel Holbert [:dholbert]]

(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

[Mats Palmgren (inactive)]

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

Comment 6

[Daniel Holbert [:dholbert]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85036/diff/1-2/

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Daniel Holbert [:dholbert]]

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 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out: https://hg.mozilla.org/integration/autoland/rev/c635c18d9eca4d6791446221b0b43a7f9bec7d08

Comment 11

[Daniel Holbert [:dholbert]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85036/diff/2-3/

Comment 12

[Daniel Holbert [:dholbert]]

Try run with updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfc7a4f87574

Comment 13

[Daniel Holbert [:dholbert]]

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.

Comment 14

[Daniel Holbert [:dholbert]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Mats Palmgren (inactive)]

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

[Carsten Book [:Tomcat]]

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