983427 - In flex containers with a bottom-to-top axis, process children in top-to-bottom order by flipping logic, to aid in pagination

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Currently, we lay out flex items in their document order (modulo tweaks from the "order" property).  This means that when for flex containers with a bottom-to-top axis, the last children we reflow will be the ones at the top.

As a result, we'd naturally end up paginating the following column-reverse flex container...

     +-----+
     |+---+|
     ||E F||
     ||G H||
     |+---+|
     |+---+|
     ||A B||
     ||C D||
     |+---+|
     +-----+
...to look like this:

      PAGE1       PAGE2
     +-----+     |     |
     |+---+|     ||G H||
     ||E F||     |+---+|
     |     |     +-----+
     |+---+|
     ||A B||
     ||C D||
     |+---+|

...with content being split from the top of one page to the top of the next page. (or even to the bottom of the next page, if there were more items).

This is counterintuitive, from a "toilet paper" printing model.

I think we should instead paginate to:

      PAGE1       PAGE2
     +-----+     |     |
     |+---+|     ||C D||
     ||E F||     |+---+|
     ||G H||     +-----+
     |+---+|
     |+---+|
     ||A B||

so that the content at the bottom of the first page is the content that gets split.

We can do this in a relatively straightforward way by simply processing the children in the reverse order (as if we had a reversed child list), and also treating our axes as if they were flipped. So a bottom-to-top flex container becomes top-to-bottom, with the children reversed.

For this to work correctly, we need a few other logic-flips, too (e.g. "flex-start" becomes "flex-end").

I've got a local patch series that lets us process our children in reverse like this. Filing this bug to cover that work.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: flip the axes (guarded by a debugging flag, currently turned off)

This first patch adds code to FlexboxAxisTracker to flip our axes if either of them is bottom-to-top.

This code is guarded by a debugging switch, "sPreventBottomToTopChildOrdering", which is currently off. The final patch here will turn it on (and perhaps make it #ifdef DEBUG), after my intermediate patches make us flip our logic appropriately in a few places.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: Link up lists of FlexLines/FlexItems in reverse

If we're reversing our axes, this patch makes us link our FlexLines of FlexItems in reverse, too. We do the line-groupings the same as we otherwise would, but we just reverse the order of the lists.

So e.g. in a flex container with 3 children "1,2,3" and "flex-flow: row wrap-reverse", which renders like so:
  |3  |
  |1 2|

...in current m-c, we'll represent that as a LinkedList of 2 FlexLines like this:
  First FlexLine:  { item1, item2 }
  Second FlexLine: { item3 }
  Main axis: left-to-right
  Cross Axis: bottom-to-top

But now with this patch, we'll represent it as:
  First FlexLine: { item3} 
  Second FlexLine: { item2, item1 }
...and with the following (reversed!) axes, due to patch 1:
  Main Axis: right-to-left
  Cross-axis: top-to-bottom

This representation produces the same rendering, which makes the behavior-change transparent, which is good.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 3: swap "flex-start" and "flex-end" alignment behaviors

If we're internally reversing the axes, then to faithfully render what the author is expecting, we need to swap the "flex-start" and "flex-end" alignment behaviors, for all flexbox alignment properties.

Building off of the example from the previous comment, to make this concrete: if the author adds "justify-content: flex-end", then they'll expect this rendering (snapping the "3" to the right side):
 |  3|
 |1 2|

(And if we're internally reversing our axes (converting LTR bottom-to-top to RTL top-to-bottom), the right side will become the "flex-start" instead of "flex-end" main-axis edge.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 4: handle baseline alignment

This is the last bit of special-casing that's necessary -- to handle baseline-aligned flex items (which are (as a group) aligned with the container's cross-start edge, after they've been baseline-aligned with each other).

I might change this part a bit to make it clearer/cleaner, so I'm not requesting review on this one quite yet.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 5: toggle the switch to activate this code

And this last patch actually activates this code-path, by toggling the "sPreventBottomToTopChildOrdering" switch that was added in part 1 (now that we've got the special-cases handled).

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8399751 [details] [diff] [review]
part 3: swap "flex-start" and "flex-end" alignment behaviors

>   EnterAlignPackingSpace(const FlexLine& aLine,
>+  // If we're reversing the cross axis, swap the
>+  // align-self: flex-start/flex-end behaviors
>+  // If our cross axis is (internally) reversed, swap the align-self
>+  // "flex-start" and "flex-end" behaviors:

Looks like the first two lines should be removed?

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Part 4a: Generify GetBaselineOffsetFromOuterCrossStart to support cross-end as well as cross-start

HIGH-LEVEL OVERVIEW OF PATCH 4:
 - Currently, we store the baseline alignment position of a line as an offset from the line's cross-start edge. (This makes sense, because baseline-aligned items end up being collectively snapped up against the FlexLine's cross-start edge.)
 - When we're internally reversing axes, we'll instead need to snap them against the FlexLine's *cross-end* edge.
 - So, we need to do our calculations in terms of that edge instead.

This first part, Part 4a, generifies the existing "FlexItem::GetBaselineOffsetFromOuterCrossStart()" function to allow us to measure the baseline's distance from *either* the cross-start or cross-end edge.

(The next part will take advantage of this.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 4b: Perform item baseline alignment with respect to FlexLine's cross-end edge, if axes are reversed

This patch:
 - Changes the FlexLine's computed "baseline alignment position" to be with respect to the line's cross-end edge (instead of cross-start edge), when we're reversing the axes
 - Adjusts the code for FlexItem cross-axis positioning -- in SingleLineCrossAxisPositionTracker::EnterAlignPackingSpace -- to accommodate this change. (This is also the spot where we make use of the generified function from Part 4a.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 4c: update baseline-computation of container from first (or last!) flex line

This last patch fixes up how we determine the flex container's own baseline, given its FlexLines.

The spec says that if the first line has baseline-aligned items (i.e. returns anything other than nscoord_MIN from GetBaselineOffset()), then we should use that line's baseline-alignment-position as the container's baseline. So currently, the code does some work on the first FlexLine for this. But if we're reversing our axes, we need to instead check the *last* FlexLine.

In the service of that, this patch refactors the "physical ascent from logical ascent" computation into a helper-function (see comment above ComputePhysicalAscentFromLogicalAscent in patch), to slightly simplify the code & clarify what it's doing.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch 4c v2: compute baseline of container from *last* flex line, if axes are reversed

(minor whitespace fix; removed a blank line & condensed two args to be on the same line)

Comment 11

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=eda8c974ba13

(Wrong bug # in some of the commit messages; I'll fix that before landing for real)

Comment 12

[Daniel Holbert [:dholbert]]

(& thanks for the reviews!)

Comment 13

[Daniel Holbert [:dholbert]]

The mochitest failures in the Try run are from some testcases with huge & small flex values combined (e.g. a container with some items having flex:3000000 and others with flex:1).

In that situation, we don't distribute the extra space *quite* perfectly, and we have mochitests with hardcoded expectations for the amount of bias that we end up with. And if we process the children in the opposite order, we end up with slightly different bias.

Those tests probably just need some softening of their assumptions.

Comment 14

[Daniel Holbert [:dholbert]]

Try run with bug 992397 fixed (using an epsilon for the comparisons that were failing in the earlier try run): https://tbpl.mozilla.org/?tree=Try&rev=3367620e6781

Comment 15

[Daniel Holbert [:dholbert]]

(Fixed typo from that Try run, as discussed in bug 992397 comment 3 and later.)

While review is pending on bug 992397, I've landed the bulk of this bug's patches -- all except the final "enable" patch (which can land when bug 992397 is fixed):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/2933825b6ddc
  https://hg.mozilla.org/integration/mozilla-inbound/rev/e973011769ef
  https://hg.mozilla.org/integration/mozilla-inbound/rev/83239ec80721
  https://hg.mozilla.org/integration/mozilla-inbound/rev/eca40ea81243
  https://hg.mozilla.org/integration/mozilla-inbound/rev/23e5d4a21960
  https://hg.mozilla.org/integration/mozilla-inbound/rev/fae9b3994877

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2933825b6ddc
https://hg.mozilla.org/mozilla-central/rev/e973011769ef
https://hg.mozilla.org/mozilla-central/rev/83239ec80721
https://hg.mozilla.org/mozilla-central/rev/eca40ea81243
https://hg.mozilla.org/mozilla-central/rev/23e5d4a21960
https://hg.mozilla.org/mozilla-central/rev/fae9b3994877

Comment 17

[Daniel Holbert [:dholbert]]

Landed part 5, the final "enable" patch:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/16bca471e1dc

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16bca471e1dc