939896 - Refactor flexbox layout code to deal with a "FlexLine" helper class

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, our flexbox layout code deals with a nsTArray of FlexItems.

I'd like to abstract that into a FlexLine object, which represents a single line of flex items.  This object will primarily be responsible for managing that nsTArray, and it will manage a few other pieces of line-specific data.

(The goal of this is to prepare for multi-line flexbox. We'll of course ultimately end up with multiple FlexLine objects. But for the purposes of this bug, there will be only one.)

Comment 1

[Daniel Holbert [:dholbert]]

This bug's patch-stack are layered on top of the patches in bug 811024. (Not that it has to be that way; that's just the order in which I wrote them, and they share a lot of contextual code.)
   --> Adding dependency.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1: Create FlexLine class (and s/items/line.mItems/)

This patch creates the FlexLine class, just as a wrapper for the |items| array of FlexItems.  Now, GenerateFlexItems populates the line's nsTArray, instead of directly populating a passed-in array.  (And in bug 939901, it'll end up populating an _array of FlexLines_.)

For simplicity (so that everything compiles and works), this patch just replaces all references to |items| with |line.mItems|. Subsequent patches here will have us pass around the actual FlexLine and refactor some functionality to live directly on the FlexLine.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2: Make ComputeLineCrossSize take a FlexLine (instead of an array of FlexItems)

This fixes up one helper-function ComputeLineCrossSize (which clearly operates on a line) to take a FlexLine now.

(no functional change here; just search-and-replace, basically)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 3: Track line cross-size & baseline offset in FlexLine, instead of SingleLineCrossAxisPositionTracker

This patch moves two pieces of metadata about a line (its cross size and its offset for baseline-aligned items) to FlexLine.

(Previously these were tracked in SingleLineCrossAxisPositionTracker, which is a temporary object used while we're positioning items within a line. Now that we have a FlexLine object (which lives longer than the SingleLineCrossAxisPositionTracker), it makes more sense to store this data on the FlexLine.)

(I'm not requesting review on this part yet, because I might tweak the baseline-related code here a bit while preparing patches for bug 939901.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 4: move PositionItemInCrossAxis into FlexLine (and have it position *all* the line's items)

This patch makes a FlexLine responsible for positioning its flex items in the cross axis.

(Note that the function is also renamed from PositionItem to PositionItems (with an "s") -- i.e. now the function positions *all* the items. (This makes ::Reflow a bit shorter/simpler, and lets us abstract away the mItems array a bit better.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 5: move PositionItemInMainAxis into FlexLine (and have it position *all* the line's items)

Similar to the previous patch, but now for the *main* axis. (i.e. the horizontal axis, in a normal horizontal flex container)  Now the line manages the main-axis positioning of its items, and it does the iteration internally.

As a (necessary) side modification here, this simplifies the MainAxisPositionTracker constructor to no longer take a pointer to the flex container frame or the reflow state. (It only needed the nsFlexContainerFrame so that it could look up the computed "justify-content" property -- for main-axis spacing of flex items -- and I changed it to just directly take that computed value.  And it didn't actually need its reflow state parameter at all (though it might've at some point in the past); it was just using that for an unimportant assertion.)

(This was a "necessary" side modification because I'm now declaring the MainAxisPositionTracker instance in a place where we don't have direct access to the nsFlexContainerFrame or to its reflow state.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 6: Make ComputeFlexContainerMainSize take a FlexLine

This is another small search-and-replace to make ComputeFlexContainerMainSize take a FlexLine instead of a nsTArray<FlexItem) -- removing another place where ::Reflow has to manually reach into line.mItems.

(In retrospect, this probably should be ordered after "part 2", since it's pretty similar to that. Oh well. Happy to renumber, if you'd like. :) )

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 7: Move ResolveFlexibleLengths() to live on FlexLine

This patch moves the existing ResolveFlexibleLengths() functionality to live on FlexLine (and operate on the line's "mItems", instead of being passed the items array directly as a parameter).

This function contains the "main" flexbox functionality -- distributing free space among flex items according to flexibility.  It's done on a per-flex-line basis, so it makes sense for it to live on FlexLine.  (When bug 939901 adds multi-line layout support, it'll have us looping over all of our lines and calling this on each one.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 8: Make FlexLine store the sums of its items' hypothetical inner & outer main sizes

The flexbox algorithm has several parts that depend on the sum of the hypothetical inner main sizes of a line's FlexItems, and/or the sum of the hypothetical outer main sizes of those items.

("hypothetical" = before we've flexed the children, inner = content-box, outer = margin-box)

This patch makes us calculate these totals up-front, when we're building the FlexLine, so we have them easily available.  This also adds one usage of the outer sum, replacing some old code (SumFlexItemMarginBoxMainSizes) that calculated the sum on-the-fly. (I'll add other usages of these quantities in the next patch here, too, and they'll also be needed in bug 939901, as part of figuring out whether an item fits on a given line or whether it should wrap to a new line.)

Note: This patch only adds a getter for the *outer* sum, because (for now at least) the only usage of the inner sum* will be internal to FlexLine, and hence needs no accessor.

*(this usage will be added in the next patch here, "part 9")

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 9: use cached sums of flexbox inner/outer main sizes to simplify ResolveFlexibleLength

This patch:
 (a) Makes ResolveFlexibleLengths use the inner & outer sums that we now have cached (thanks to the previous patch, part 8), instead of doing math in "ShouldUseFlexGrow" that amounts to doing the same thing.

 (b) Drops the aAxisTracker parameter from ResolveFlexibleLengths, since (a) removes its only usage.

 (c) Renames a boolean variable derived in (a) from "havePositiveFreeSpace" to "isUsingFlexGrow", for clarity about what's actually going on and what the variable actually means.  (I did this in one helper-function that takes this flag as an argument, too.)

Comment 11

[Daniel Holbert [:dholbert]]

...and I think that's all the parts for this bug. :)

(Hopefully the piecewise nature isn't too overwhelming; they patches are mostly small & search-and-replace-ish.  I tried to keep them as modular and understandable-as-a-single-unit as possible.)

(None of these patches should have a functional effect; instead, they lay the groundwork for bug 939901 quite nicely.)

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8337348 [details] [diff] [review]
part 7: Move ResolveFlexibleLengths() to live on FlexLine

>layout/generic/nsFlexContainerFrame.cpp
>-        for (uint32_t i = aItems.Length() - 1; i < aItems.Length(); --i) {
>+        for (uint32_t i = mItems.Length() - 1; i < mItems.Length(); --i) {

OK, since you're only changing aItems -> mItems.

I don't like this loop pattern though.  It's verbose and uint32_t isn't
really correct ('size_type' of the array class is) and the unsigned
wrap-around end condition is hard to see for a casual reader.

For simple loops, where you don't remove entries from mItems, I'd prefer:

          for (auto i = mItems.Length(); i-- ;)

(assuming it's OK to use c++11 'auto' now)

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8337355 [details] [diff] [review]
part 9: use cached sums of flexbox inner/outer main sizes to simplify ResolveFlexibleLength

layout/generic/nsFlexContainerFrame.cpp
>+  // base size, so that when both large and small things are shrinking,
>+  // the large things shrink more).

s/things/items/ perhaps?

r=mats on all patches so far (part 1 to 9).

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #12)
> For simple loops, where you don't remove entries from mItems, I'd prefer:
> 
>           for (auto i = mItems.Length(); i-- ;)
> 
> (assuming it's OK to use c++11 'auto' now)

I asked in #developers if we're safe to use "auto" now, and glandium says "it depends", and "arguably, we should have nsTArray<Foo>::iterator".  He also pointed out that we technically would want to start at Length() - 1 there, or else the first loop iteration reads off the end of the array. (though that doesn't work if mItems is empty -- then you end up with -1 which will likely wrap to +infinity)

In any case; I agree that using a uint32_t loop counter is undesirable (albeit less verbose than nsTArray<FlexItem>::size_type), but I'll hold off on doing any sort of conversion here. :)

Thanks for the quick review turnaround!  I've got a cleaned up version of part 3 ready to post - just need to fold some chunks together.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #13)
> s/things/items/ perhaps?

(Fixed locally. Thanks!)

Comment 16

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> He also pointed out that we technically would want to start at Length() - 1
> there, or else the first loop iteration reads off the end of the array.

No, I think he misunderstood.  The loop condition I propose is 'i', the decrement
happens after the test.  So if Length() is zero we don't enter the loop, and the
test never relies on what the result of decrementing zero is.
What I proposed above is equivalent to:

           for (auto i = mItems.Length(); i ;) {
             i--;
             // 'i' is always a valid index here

Comment 17

[Daniel Holbert [:dholbert]]

Ah, right. I think that makes sense -- thanks for clarifying. Anyway, topic for another bug. :)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: part 3a: Make FlexItem calculate its own baseline offset from cross-start edge

So I'm inserting one helper-patch before part 3. (so we'll now have "part 3a" and "part 3b")

Part 3a (this patch here) makes FlexItem itself responsible for computing the distance between its baseline and the cross-start edge of its margin-box.

(Previously, SingleLineCrossAxisPositionTracker managed this code, but I want to be able to call it without needing a SingleLineCrossAxisPositionTracker, since we need this code when we're simply sizing a line (not positioning anything yet), and I'm moving the line-sizing code out of SingleLineCrossAxisPositionTracker.)

This patch also makes this code (for "get an item's offset from its cross-start margin-box edge") slightly more intelligent, so that it can now handle bottom-to-top *as well as* top-to-bottom cross axes. Previously it only handled top-to-bottom.  (Currently there's no way to get bottom-to-top cross-axes, but multiline with "flex-wrap: wrap-reverse" will add a case that exercises it -- and I've verified that this does the right thing, with my WIP multiline patches).

Comment 19

[Daniel Holbert [:dholbert]]

(Note: part 3a removes a NSCoordSaturatingAdd() usage, in modernizing this code -- I'm pretty sure I don't need that anymore, ever since bug 785468 made baseline alignment actually work correctly.  In particular, that bug added ResolveReflowedChildAscent which resolves ASK_FOR_BASELINE (nscoord_MAX) to something non-nscoord_MAX more aggressively than we did before.)

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: part 3b: Track line cross-size & baseline offset in FlexLine, instead of SingleLineCrossAxisPositionTracker

...and here's the updated version of what was formerly known as "part 3". (now "part 3b").

See comment 4 for description.

New things in this version, beyond what was mentioned in comment 4:
 - This patch makes the FlexLine actually responsible for *computing* (as well as storing) its own cross-size and baseline (which it can now do without a SingleLineCrossAxisPositionTracker, with the benefit of Part 3a).  Specifically, SingleLineCrossAxisPositionTracker::ComputeLineCrossSize has been renamed to FlexLine::ComputeCrossSizeAndBaseline.

 - As a result of that ^ change, we can delay instantiation of the SingleLineCrossAxisPositionTracker in ::Reflow until we're actually positioning items. (we don't need it when we're sizing the line)  So, this patch delays that instantiation now, too. (And then in part 4, the instance gets absorbed into PositionItemInCrossAxis entirely, so it doesn't need to be declared in Reflow anymore).

Comment 21

[Daniel Holbert [:dholbert]]

(Parts 3a / 3b cause bitrot in the subsequent patches; I've fixed that locally, but I won't bother reposting all the individual patches. I'll post a rollup for convenience, though.)

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: rollup patch

Here's a rollup patch of all the (unbitrotted) versions of the attached patches, in case it's useful.

(This includes the s/things/items/ review suggestion from comment 13, too)

Comment 23

[Daniel Holbert [:dholbert]]

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

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> (Note: part 3a removes a NSCoordSaturatingAdd() usage

The debug-only crashtest orange (from reduced assertion-failures) is from this NSCoordSaturatingAdd removal, FWIW.

We had assertions before because the crashtest in question has huge pixel values, which probably produce values larger than nscoord_MAX or nscoord_MIN, and those values failed assertions in NSCoordSaturatingAdd.

Without NSCoordSaturatingAdd, we don't hit those assertions anymore.

Comment 25

[Daniel Holbert [:dholbert]]

Here's a new Try run with the "asserts(12)" annotation removed, which addresses the orange (from unexpected lack of assertions) in Comment 23's Try run: https://tbpl.mozilla.org/?tree=Try&rev=6b60047d4850

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 8338162 [details] [diff] [review]
part 3a: Make FlexItem calculate its own baseline offset from cross-start edge

Looks good, r=mats

I think this should work with negative margins, but do we have any tests
for that?  I can't find any specified negative margins under
layout/reftests/flexbox/

Do we have tests for an over-constrained auto-margin? i.e. where one side
has an auto margin and the item has a specified size that is larger
then the container's size in that dimension -- leading to negative used
margin on the side that has auto.  Or does that not happen for flexbox
like it does for blocks?

I'm thinking of the case "If there is exactly one value specified as 'auto',
its used value follows from the equality."
http://www.w3.org/TR/CSS21/visudet.html#blockwidth

Or perhaps there is no corresponding rule for flexbox?
(8.1 seems to indicate it should be "similar to auto margins in block flow",
but it's vague. http://www.w3.org/TR/css3-flexbox/#auto-margins)

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 8338169 [details] [diff] [review]
part 3b: Track line cross-size & baseline offset in FlexLine, instead of SingleLineCrossAxisPositionTracker

r=mats

ComputeCrossSizeAndBaseline() is only using its aAxisTracker parameter
to get aAxisTracker.GetCrossAxis() -- perhaps it could take just an
AxisOrientationType unless you plan some further use of it?

Comment 28

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #24)
> The debug-only crashtest orange (from reduced assertion-failures) is from
> this NSCoordSaturatingAdd removal, FWIW.

FYI, I'm removing those assertions in bug 943448.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #26)
> I think this should work with negative margins, but do we have any tests
> for that?  I can't find any specified negative margins under
> layout/reftests/flexbox/

FWIW, most of our flexbox reftests actually live in layout/reftests/w3c-css/submitted/flexbox/. (The ones left in reftests/flexbox are the ones that are harder to make work in non-gecko browsers.)

But I think you're right -- we don't have any tests for flex items with negative margins. I filed bug 943459 on adding some.

> Do we have tests for an over-constrained auto-margin? i.e. where one side
> has an auto margin and the item has a specified size that is larger
> then the container's size in that dimension -- leading to negative used
> margin on the side that has auto.  Or does that not happen for flexbox
> like it does for blocks?

We don't, but that's actually simpler in flexbox than it is in blocks. The normative description for what happens to auto margins is here:
  http://dev.w3.org/csswg/css-flexbox/#algo-main-align
Basically, auto margins are treated as 0, unless there's positive free space left over for them, in which case the free space is distributed equally among the auto margins.

Comment 30

[Daniel Holbert [:dholbert]]

(I broadened bug 943459 to cover the sort of auto margin situation you brought up, too.)

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #27)
> ComputeCrossSizeAndBaseline() is only using its aAxisTracker parameter
> to get aAxisTracker.GetCrossAxis() -- perhaps it could take just an
> AxisOrientationType unless you plan some further use of it?

I'm actually intentionally trying to have aAxisTracker be the thing that gets passed around, and then specific functions can pull out the axis off of it that they want.  This makes function invocations simpler, and e.g. it means the constructors for both MainAxisPositionTracker and CrossAxisPositionTracker can just take our axisTracker, and internally they pull out the axis that they care about. If we instead made them take an AxisOrientationType, then we'd have to worry about the possibility that someone might be accidentally passing in the wrong AxisOrientationType.

The FlexItem method added in Part 3a is an exception to this -- it *does* take a AxisOrientationType -- because one of that method's callers is a chunk of code that doesn't have an axis tracker handy, but it does know have the cross axis saved in a member-var. (Maybe I should just add the axis tracker as an argument to that chunk of code, though.)

Comment 32

[Mats Palmgren (inactive)]

OK, that makes sense.

Comment 33

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #31)
> (Maybe I should just add the axis
> tracker as an argument to that chunk of code, though.)

(I'll think about making that change after doing multi-line layout; for now, I don't think it matters much.)

Thanks for the reviews! Landed patches:
https://hg.mozilla.org/integration/mozilla-inbound/rev/535daeab5feb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3ea2d02b17
https://hg.mozilla.org/integration/mozilla-inbound/rev/79dda4faf052
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbfe7e62668
https://hg.mozilla.org/integration/mozilla-inbound/rev/81fd6d2165f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca724f5965c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ec0673d85f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cee108813e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1fe321bbaf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5ae868b923

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/535daeab5feb
https://hg.mozilla.org/mozilla-central/rev/2a3ea2d02b17
https://hg.mozilla.org/mozilla-central/rev/79dda4faf052
https://hg.mozilla.org/mozilla-central/rev/7dbfe7e62668
https://hg.mozilla.org/mozilla-central/rev/81fd6d2165f6
https://hg.mozilla.org/mozilla-central/rev/eca724f5965c
https://hg.mozilla.org/mozilla-central/rev/b1ec0673d85f
https://hg.mozilla.org/mozilla-central/rev/1cee108813e0
https://hg.mozilla.org/mozilla-central/rev/b1fe321bbaf6
https://hg.mozilla.org/mozilla-central/rev/fb5ae868b923