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

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Drew Goodwin]

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.

Comment 1

[Drew Goodwin]

Attachment: rowVsColumn.png

Comment 2

[Drew Goodwin]

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.

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

(Thanks for the bug report, BTW!)

Comment 5

[Daniel Holbert [:dholbert]]

(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.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1433339: Apply single-item fallback behavior for 'space-between' more directly, in flexbox code.

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

Comment 7

[Daniel Holbert [: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).

Comment 8

[Mats Palmgren (:mats)]

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

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8951797 [details]
Bug 1433339: Apply single-item fallback behavior for 'space-between' more directly, in flexbox code.

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

Comment 10

[Daniel Holbert [:dholbert]]

Good point - fixed. Thanks!

Comment 11

[Pulsebot]

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

[Stefan Hindli [:stefan_hindli]]

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