Closed Bug 1269045 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox49 --- affected
firefox52 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(
URL
)

Details

Attachments

(5 files)

Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other.
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1269045 part 2: Separate out abspos placeholders when creating FlexItems, and give them a trivial reflow at container's content-box origin.
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1269045 part 3: Stop wrapping placeholder frames in anonymous flex items.
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1269045 part 4: Drop now-obsolete parameter from anonymous flex/grid-item wrapping functions.
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
"part 2b" (interdiff to reflow placeholders at content-box origin)
6.87 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review 
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.)
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
Blocks: 1269046
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)
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/
(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.)
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+
(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 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 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+
(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.)
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).
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+
(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.
Depends on: 1313421
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.
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
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
Blocks: 874713
Depends on: 1320484
Depends on: 1330990
Blocks: 1341030
Depends on: 1345873
Depends on: 1350580
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: