Closed
Bug 1433339
Opened 3 years ago
Closed 3 years ago
flex-direction: column-reverse not reversed when combined with justify-content: space-between
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: drew, Assigned: dholbert)
Details
(Whiteboard: [parity-Chrome][parity-Edge])
Attachments
(2 files)
9.94 KB,
image/png
|
Details | |
Bug 1433339: Apply single-item fallback behavior for 'space-between' more directly, in flexbox code.
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36 Steps to reproduce: View a page with a parent div with styles: flex-direction: column-reverse; justify-content: space-between; and a single child div. Example: https://jsfiddle.net/tacomanator/57f63f5f/5/ Actual results: The child appears at the beginning of the parent. Expected results: Child should have appeared at end of parent.
Reporter | ||
Comment 1•3 years ago
|
||
Reporter | ||
Comment 2•3 years ago
|
||
Comment on attachment 8945666 [details]
rowVsColumn.png
Screenshot of example. The yellow block is the child. For row-reverse (the red parent), the child appears at the end. For column-reverse (the turquoise parent) the child appears at the beginning. If justify-content: space-between is removed from the parent, the child will appear at the end of both parents.
![]() |
||
Updated•3 years ago
|
Component: Untriaged → Layout
Product: Firefox → Core
![]() |
||
Updated•3 years ago
|
Whiteboard: [parity-Chrome][parity-Edge]
![]() |
||
Updated•3 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 58 Branch → Trunk
Assignee | ||
Comment 3•3 years ago
|
||
I think our column behavior here is actually *correct*, and our row-reverse behavior is wrong. (And similarly, Chrome & Edge's behavior in both cases is wrong, if they are actually pushing the orange box to the end.) There are two specs that purport to define what happens here -- the flexbox spec & the align spec. Unfortunately, they disagree. :) But the align spec is newer & disagrees towards a behavior that it introduced (the "start" keyword). Flexbox says to fall back to 'flex-start' (i.e. the cross-axis / main-axis start side, which is the right or bottom edge in this "reverse" scenario): > space-between > If...there is only a single flex item on the line, this value is identical to flex-start. ...whereas Align says to fall back to 'start' (i.e. the inline-axis / block-axis start side, which is the left or top edge unless the "writing-mode" property has been set to something nondefault): > The default fallback alignment for this value is start. I filed a spec issue ( https://github.com/w3c/csswg-drafts/issues/2316 ) to get this cleared up in the specs, but I'm pretty sure the alignment spec is supposed to win here, per the last sentence of the green box in https://drafts.csswg.org/css-flexbox-1/#alignment ("the Box Alignment module, once completed, will supercede the definitions here.") --> Setting needinfo=me to investigate our probably-broken row-reverse behavior & to file bugs on other browsers here.
Flags: needinfo?(dholbert)
Assignee | ||
Comment 5•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #3) > I think our column behavior here is actually *correct*, and our row-reverse > behavior is wrong. Our column behavior here is correct "by accident", though. We're not intentionally going for the newer CSS-Align 'fall back to start' behavior -- we're getting it accidentally. (And bug 1207698 is where I should really add code to make us *intentionally* fall back to 'start'.) For now, for the purposes of this bug, I'm going to make us consistent per the original flexbox spec & do what we're intending to do, and match other browsers -- and then in bug 1207698, we can add the subtlety from the alignment spec. SO -- anyway, here's what was going on to cause this bug for us: * We've got this special handling in flexbox to detect cases where a flexbox is bottom-to-top (e.g. flex-direction:column-reverse) -- when this happens, we do some special stuff under the hood to invert the axes & reorder the children so that we're actually dealing with them in a top-to-bottom order, so that we can (eventually) fragment the child list across printed pages in a sane way. * For this to work, we flip some of the alignment keywords (so that when an author requests "justify-content:flex-start" in a bottom-to-top flexbox, we treat it as "flex-end" in our sneakily-reordered top-to-bottom under-the-hood representation, so that we end up aligning stuff at the bottom like the author requested). * This bug is a case where we're failing to do that. Specifically, we're using nsFlexContainerFrame::CalculatePackingSpace to do the fallback alignment here, and that function is not aware of this special axes-reversed-under-the-hood mode. I've got a fix coming up to make us handle the single-alignment-subject fallback cases for "space-{around,between,evenly}" up-front, so that nsFlexContainerFrame::CalculatePackingSpace won't have to bother with this. (We already handle it up-front for the less-than-zero-packing-space special case, and I'm going to adjust that special case to also trigger the fallback mode for the single-alignment-subject special case.)
Assignee: nobody → dholbert
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•3 years ago
|
||
See comment 5 for an overview of the code changes. Without this bug's fix, Firefox fails two of the tests that I'm adding here, with just the single-item "space-between" piece of the testcase rendering at the top/left edge rather than the bottom/right edge: flexbox-justify-content-vertrev-001.xhtml (this is analogous to the reporter's example) flexbox-align-content-horizrev-001-ref.xhtml (this wasn't in the reporter's example, but it's similar -- just for when the cross axis is reversed and we have "align-content:space-between") RE the tests -- I'm copying some of our existing w3c-submitted "justify-content"/"align-content" tests, but these tests (the existing ones & new ones) aren't entirely right/comprehensive right now. The tests will eventually need updating to clarify/correct/add keyword usage, per bug 1207698. Plus, the new tests' expectations aren't really correct in light of updates in CSS-Align, as discussed in comment 5. SO: I woudln't feel right adding this bug's new tests into the w3c-submitted test directory, in their current state with incomplete/obsolete expectations. Therefore, I'm putting this bug's tests into layout/reftests/flexbox, and I'll plan to move them over as part of bug 1207698 (at which point they'll be fixed up).
Flags: needinfo?(dholbert)
Comment 8•3 years ago
|
||
mozreview-review |
Comment on attachment 8951797 [details] Bug 1433339: Apply single-item fallback behavior for 'space-between' more directly, in flexbox code. https://reviewboard.mozilla.org/r/221050/#review227044 ::: layout/generic/nsFlexContainerFrame.cpp:3037 (Diff revision 1) > // If packing space is negative, 'space-between' and 'stretch' behave like > // 'flex-start', and 'space-around' and 'space-evenly' behave like 'center'. > // In those cases, it's simplest to just pretend we have a different > - // 'align-content' value and share code. > - if (mPackingSpaceRemaining < 0) { > - if (mAlignContent == NS_STYLE_ALIGN_SPACE_BETWEEN || > + // 'align-content' value and share code. (If we only have one line, all of > + // the the "space-*" keywords fall back as well, but "stretch" doesn't > + // because even a single line can still stretch.) minor nit: we're currently using single-quote around property values in this comment (and the rest of the file it seems), so "space-*" and "stretch" should probably use that too for consistency ::: layout/generic/nsFlexContainerFrame.cpp:4291 (Diff revision 1) > - MOZ_ASSERT(aNumThingsToPack >= 1, > - "Should not be called with less than 1 thing to pack"); > + // Note: In the aNumThingsToPack==1 case, the fallback behavior for > + // "space-between" depends on precise information about the axes that we > + // don't have here. So, for that case, we just depend on the caller to > + // explicitly convert "space-{between,around,evenly}" keywords to the > + // appropriate fallback alignment and skip this function. ditto
Attachment #8951797 -
Flags: review?(mats) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•3 years ago
|
||
Good point - fixed. Thanks!
Comment 11•3 years ago
|
||
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c2b3a9f871a3 Apply single-item fallback behavior for 'space-between' more directly, in flexbox code. r=mats
Comment 12•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2b3a9f871a3
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•