|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
Bug 1090031: Apply CSS 'align-content' in flex containers if they *could* wrap (rather than if they *have* wrapped).
58 bytes, text/x-review-board-request
|Details | Review|
Fantasai proposes in  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 . We probably should check the computed style & only return if we have "flex-wrap: nowrap".  http://lists.w3.org/Archives/Public/www-style/2014Oct/0489.html  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=366b01141df5&mark=2457-2467#2457
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
(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 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+
Thanks! Addressed that. I've triggered a try run via mozreview; will land if that looks good.
Pushed by email@example.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
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).
Backed out: https://hg.mozilla.org/integration/autoland/rev/c635c18d9eca4d6791446221b0b43a7f9bec7d08
Try run with updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfc7a4f87574
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.
(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 )
Pushed by firstname.lastname@example.org: 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...
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.