1269045 - [css-flex] Stop wrapping abspos children of flex container in anonymous flex items

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

This is the first piece of bug 874718 (updating our code for flex container abspos children, to match the current spec).

In this bug here, I intend to:
 - Remove our "wrap abspos placeholders in anonymous flex items" code & assumptions (from an older spec versions).
 - Exempt these placeholders (which are now direct chilren of a nsFlexContainerFrame) from being treated as FlexItems / participating in flex layout.
 - Exempt these placeholders from "order" sorting, so we can preserve the status quo there (which is correct but only accidentally so -- see bug 1264147 comment 3).

This bug does *not* cover the alignment of abspos children using "align-self" / "justify-content". I'll file a separate bug on that, which will depend on this bug.  (I intend to land these bugs at the same time, but I think it makes sense to logically separate them, in part because the alignment stuff will probably be shared with abspos grid children.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other.

This patch makes the following specific changes:
 (1) Adds an early-return to both versions of the IsOrderLEQ function, to treat placeholder children as LEQ everything (including each other). This will tend to sort them to the beginning of the child list, which is unimportant but fine. More importantly, though: this means our "order"-sorting code won't reorder placeholders *with respect to each other* (since our sort algorithm is stable). So their painting order won't be affected by the "order" property, which is required by the spec.
 (2) Drops some nsPlaceholderFrame::GetRealFrameFor() calls -- they're unnecessary, since any placeholder frames will have prompted us to return earlier.

One caveat to (2): this patch does leave a few "nsPlaceholderFrame::GetRealFrameFor()" calls in place, *for the moment*.  These remaining calls are for handling placeholders that are wrapped, i.e. inside of anonymous flex items. These calls are still needed to avoid assertion-failures (i.e. to get a consistent ordering) at this point, but they'll be removed in a later patch in this same bug, when we stop wrapping placeholders in anonymous flex items.

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

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

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

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

This patch also:
 * Removes some now-unnecessary code from nsFlexContainerFrame, which was for jumping from wrapped-placeholders to their out-of-flow frames (for DOM comparisons). This code is now unnecessary because placeholders won't be wrapped anymore.
 * Marks some reftests with abspos content as "fails" for the time being. These tests will be fixed (and probably rewritten a bit) in bug 1269046, which is where we'll implement the correct static position for abspos children of a flex container.

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

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

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

Comment 5

[Daniel Holbert [:dholbert]]

(Note: I'm logically separating this from bug 1269046, but I intend to land them together -- i.e. the patches here won't land until we've got the static position correctly implemented over on bug 1269046.)

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8749420 [details]
Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other.

https://reviewboard.mozilla.org/r/50927/#review47633

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

https://reviewboard.mozilla.org/r/50929/#review47629

::: layout/generic/nsFlexContainerFrame.cpp:3975
(Diff revision 1)
>                                     const FlexboxAxisTracker& aAxisTracker)
>  {
>    aStatus = NS_FRAME_COMPLETE;
>  
>    LinkedList<FlexLine> lines;
> +  AutoTArray<nsIFrame*, 1> placeholderKids;

Can you explain what you gain by this, rather than just using nsTArray<nsIFrame*> ?

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

https://reviewboard.mozilla.org/r/50931/#review47635

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8749423 [details]
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

https://reviewboard.mozilla.org/r/50933/#review47637

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #7)
> > +  AutoTArray<nsIFrame*, 1> placeholderKids;
> 
> Can you explain what you gain by this, rather than just using
> nsTArray<nsIFrame*> ?

Hmm, I guess not much. It lets us avoid heap allocation when there's exactly 1 abspos thing, but I suppose that's not much of a help.

I expect abspos stuff here to be an uncommon case, so probably best that I use nsTArray (with no reserved stack space) as you're implicitly suggesting, I think.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

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

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

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

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8749423 [details]
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

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

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

https://reviewboard.mozilla.org/r/50929/#review47641

Looks fine, thanks.  I'd expect the common case is zero placeholders (by far).

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Created attachment 8749421 [details]
> MozReview Request: Bug 1269045 part 2: Separate out abspos placeholders when
> creating FlexItems, and give them a trivial reflow pass at 0,0. r?mats

So, it turns out to be more convenient to bug 1269046 if we reflow these placeholders at the *flex container's content-box origin*. Then, in bug 1269046, we can just compute the static position as an *offset* (based on the child's size & the container's size & the align properties), and add that offset to the placeholder's position to determine the static position.

So: I'd like to update part 2 here to reflow these children at 0,0. I'll post an interdiff patch, for review convenience, and then I'll update the MozReview stack here. (MozReview may or may not be able to show useful interdiffs; it sometimes shows unrelated changes after a patch stack has been rebased.)

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: "part 2b" (interdiff to reflow placeholders at content-box origin)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8749420 [details]
Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other.

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

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

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

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

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

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8749423 [details]
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

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

Comment 22

[Daniel Holbert [:dholbert]]

Just pushed the unbitrotted versions of the original patches. Now I'll push one more update with "part 2b" folded into "part 2" (which should produce a useful interdiff, if you want to use mozreview UI instead of splinter to see the "part 2b" changes).

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50929/diff/3-4/

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50931/diff/3-4/

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8749423 [details]
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50933/diff/3-4/

Comment 26

[Daniel Holbert [:dholbert]]

Here's the MozReview interdiff that corresponds to comment 16, folding in "part 2b":
  https://reviewboard.mozilla.org/r/50929/diff/3-4/

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 8800390 [details] [diff] [review]
"part 2b" (interdiff to reflow placeholders at content-box origin)

nit: I slightly prefer CSS Align (without the dash) to refer to the spec by name (two places)

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8749420 [details]
Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50927/diff/3-4/

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50929/diff/4-5/

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50931/diff/4-5/

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8749423 [details]
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50933/diff/4-5/

Comment 32

[Daniel Holbert [:dholbert]]

Fixed: https://reviewboard.mozilla.org/r/50929/diff/4-5/

Thanks for the review!

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 8749420 [details]
Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50927/diff/4-5/

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50929/diff/5-6/

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50931/diff/5-6/

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8749423 [details]
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50933/diff/5-6/

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #27)
> nit: I slightly prefer CSS Align (without the dash) to refer to the spec by
> name (two places)

(Just noticed that I didn't reply to this, but I did address it, in https://reviewboard.mozilla.org/r/50929/diff/4-5/ )

My latest mozreview push here (comment 33) was just to rebase this patch-stack on top of current mozilla-inbound -- in particular, rebasing on top of bug 1309119, which made minor tweaks to the same nsCSSFrameConstructor anonymous-flex-item-wrapping code that this bug is touching.

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8749420 [details]
Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50927/diff/5-6/

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8749421 [details]
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50929/diff/6-7/

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50931/diff/6-7/

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8749423 [details]
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50933/diff/6-7/

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

https://reviewboard.mozilla.org/r/50931/#review88190

::: layout/reftests/w3c-css/submitted/flexbox/reftest.list:53
(Diff revision 7)
> -fails == flexbox-baseline-single-item-001a.html flexbox-baseline-single-item-001-ref.html # bug 1269045
> -fails == flexbox-baseline-single-item-001b.html flexbox-baseline-single-item-001-ref.html # bug 1269045
> +== flexbox-baseline-single-item-001a.html flexbox-baseline-single-item-001-ref.html
> +== flexbox-baseline-single-item-001b.html flexbox-baseline-single-item-001-ref.html
>  

My latest mozreview-push is just to rebase this patch-stack on top of bug 1313421 (and newer mozilla-inbound changes as well; unfortunately mozreview shows some of those in the interdiff).

The relevant change is this ^^ reftest.list fails-annotation removal, for the tests from bug 1313421, which fail (due to getting a baseline from a trivial anonymous-flex-item-around-a-placeholder) until this patch here stops us from wrapping placeholders in anonymous flex items.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a391e3e163
part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa8199183fc
part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/2162d5c9fb54
part 3: Stop wrapping placeholder frames in anonymous flex items. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57f9ac1435e
part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions. r=mats

Comment 44

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7c24f445542062d41f97cba2dd8a45bd3a468b98 in hopes of unbusting Windows builds.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86fbc1fb2818
part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1135160f859
part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/707b2ab5879d
part 3: Stop wrapping placeholder frames in anonymous flex items. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/78495666fd8c
part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions. r=mats

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/86fbc1fb2818
https://hg.mozilla.org/mozilla-central/rev/d1135160f859
https://hg.mozilla.org/mozilla-central/rev/707b2ab5879d
https://hg.mozilla.org/mozilla-central/rev/78495666fd8c