Closed Bug 1433339 Opened 3 years ago Closed 3 years ago

flex-direction: column-reverse not reversed when combined with justify-content: space-between


(Core :: Layout, defect, P3)




Tracking Status
firefox60 --- fixed


(Reporter: drew, Assigned: dholbert)


(Whiteboard: [parity-Chrome][parity-Edge])


(2 files)

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.


Actual results:

The child appears at the beginning of the parent.

Expected results:

Child should have appeared at end of parent.
Attached image rowVsColumn.png
Comment on attachment 8945666 [details]

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.
Component: Untriaged → Layout
Product: Firefox → Core
Whiteboard: [parity-Chrome][parity-Edge]
Ever confirmed: true
Version: 58 Branch → Trunk
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 ( ) 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 ("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)
(Thanks for the bug report, BTW!)
Priority: -- → P3
(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
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 on attachment 8951797 [details]
Bug 1433339: Apply single-item fallback behavior for 'space-between' more directly, in flexbox code.

::: 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.

Attachment #8951797 - Flags: review?(mats) → review+
Good point - fixed. Thanks!
Pushed by
Apply single-item fallback behavior for 'space-between' more directly, in flexbox code. r=mats
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.