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

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected, firefox52 fixed)

Details

(URL)

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
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.)
(Assignee)

Updated

3 years ago
Summary: Stop wrapping abspos children of flex container in anonymous flex items → [css-flex] Stop wrapping abspos children of flex container in anonymous flex items
(Assignee)

Updated

3 years ago
Blocks: 1269046
(Assignee)

Comment 1

3 years ago
Created 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.

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/
Attachment #8749420 - Flags: review?(mats)
Attachment #8749421 - Flags: review?(mats)
Attachment #8749422 - Flags: review?(mats)
Attachment #8749423 - Flags: review?(mats)
(Assignee)

Comment 2

3 years ago
Created 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 commit: https://reviewboard.mozilla.org/r/50929/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50929/
(Assignee)

Comment 3

3 years ago
Created attachment 8749422 [details]
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/
(Assignee)

Comment 4

3 years ago
Created attachment 8749423 [details]
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/
(Assignee)

Comment 5

3 years ago
(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.)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
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
Attachment #8749420 - Flags: review?(mats) → review+
Attachment #8749421 - Flags: review?(mats)
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 on attachment 8749422 [details]
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.

https://reviewboard.mozilla.org/r/50931/#review47635
Attachment #8749422 - Flags: review?(mats) → review+
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
Attachment #8749423 - Flags: review?(mats) → review+
(Assignee)

Comment 10

3 years ago
(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 hidden (spam)
(Assignee)

Comment 12

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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).
Attachment #8749421 - Flags: review?(mats) → review+
(Assignee)

Comment 16

2 years ago
(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.)
(Assignee)

Comment 17

2 years ago
Created attachment 8800390 [details] [diff] [review]
"part 2b" (interdiff to reflow placeholders at content-box origin)
Attachment #8800390 - Flags: review?(mats)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 years ago
Here's the MozReview interdiff that corresponds to comment 16, folding in "part 2b":
  https://reviewboard.mozilla.org/r/50929/diff/3-4/
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)
Attachment #8800390 - Flags: review?(mats) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

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

Thanks for the review!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

2 years ago
(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.
(Assignee)

Updated

2 years ago
Depends on: 1313421
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

2 years ago
mozreview-review
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

2 years ago
Pushed by dholbert@mozilla.com:
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
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7c24f445542062d41f97cba2dd8a45bd3a468b98 in hopes of unbusting Windows builds.

Comment 45

2 years ago
Pushed by dholbert@mozilla.com:
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

2 years ago
Blocks: 874713

Updated

2 years ago
Duplicate of this bug: 1316317
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1317345
(Assignee)

Updated

2 years ago
Depends on: 1320484

Updated

2 years ago
Depends on: 1330990
(Assignee)

Updated

2 years ago
Blocks: 1341030

Updated

2 years ago
Depends on: 1345873

Updated

2 years ago
Depends on: 1350580
You need to log in before you can comment on or make changes to this bug.