1054058 - For FlexItems whose ascent (baseline) we might need, store the ascent on the FlexItem, and resolve it lazily

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, if the flex item ends up deriving its baseline from its first child (because it has no baseline-aligned children), it grabs that baseline during our final reflow of that first child.

But we might be skipping that final reflow now, as discussed in bug 1054010. So we need to be able to get the baseline (if we need it) based on an earlier "measuring" reflow of that flex item.

I'm filing this bug on:

 (a) reliably caching the flex item's ascent, if there's a chance the container needs it. (Right now we only do this for baseline-aligned flex items, but we should do it for the container's first child as well.)

 (b) resolving ASK_FOR_BASELINE ascents lazily, to reduce the cost of doing part a.

My patches for bug 1054054 (refactoring out the final flex-item reflow into a helper-function) will layer on top of this bug.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: reliably cache each flex item's ascent, if there's a chance we'll need it

This patch does "(a)" from comment 0 -- it makes us resolve & save the flex items' ascent after each time we reflow them, if there's a chance we might need the ascent.

(For the first two "measuring" reflows, "if there's a chance we might need the ascent" really means "if the item is baseline-aligned, or if it's the first child & hence might be responsible for setting the container's baseline".

For the final reflow pass, we only need to care about if we're the first child, because baseline-alignment is already over at that point.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: resolve ASK_FOR_BASELINE ascents lazily

This patch does (b) from comment 0 -- it makes us resolve ASK_FOR_BASELINE ascents lazily, to remove that from the cost of proactively caching the ascents of flex items that we might (or might not) end up using.

This patch removes the "ResolveReflowedChildAscent" helper function, and folds its logic into the ResolvedAscent() accessor, which we now use instead of directly grabbing the item's saved ascent, to allow for lazy resolution.

(Per my XXXdholbert comment in ResolvedAscent(), we're probably using the wrong writing-mode there, but I'm not worrying about that too much [and am just preserving existing behavior], because we almost certainly mis-handle baselines when there are writing-mode differences in other ways already. I can make that function grab or require the container's writing-mode when we correct for this in bug 1079155.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 3: save itemNormalBPos in a local-var, instead of looking it up dynamically

One more bonus patch here -- this replaces this mess (from the very bottom of both previous parts here):
>        // (We use GetNormalPosition() instead of physicalPosn because we don't
>        // want relative positioning on the child to affect the baseline that we
>        // read from it).
>        flexContainerAscent = item->Frame()->GetLogicalNormalPosition(
>                                outerWM,
>                                childDesiredSize.Width()).B(outerWM) +

...with a local variable that we cache earlier on.

That GetLogicalNormalPosition(...).B(outerWM) call is just trying to get the logical coordinate of this flex item, before relative positioning is applied. Conveniently, we actually have a local variable which represents the item's logical position, so we can just grab the "B(outerWM)" accessor off of that -- though we have to do it before the call to ApplyRelativePositioning.

This is a lot cleaner IMHO, and more importantly for bug 1054054 & bug 1054010, it removes any reliance on 'childDesiredSize' in this chunk of code (which is important in letting me refactor away the flex items' final reflow to become optional).

Comment 4

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=688c5594db4a

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8552908 [details] [diff] [review]
part 1: reliably cache each flex item's ascent, if there's a chance we'll need it

>+  // If this is the first child, save its ascent, since it may be what
>+  // establishes the container's baseline. Also save the ascent if this child
>+  // needs to be baseline-aligned. (Else, we don't care about ascent/baseline.)
>+  if (aFlexItem.Frame() == mFrames.FirstChild() ||

Hmm, what if mFrames.FirstChild() is a placeholder?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #5)
> Hmm, what if mFrames.FirstChild() is a placeholder?

Ooh, good question.

So right now, that actually *can't happen*, because placheolders get wrapped in an anonymous block (to make an "anonymous flex item"), per an earlier version of the spec (though bug 874718 is filed on changing that).

That anonymous flex item behaves like any other flex item -- it establishes the baseline, if it happens to be first.  If the anonymous flex item (a block) *only* contains a placeholder (or placeholders), then it just behaves the same as an empty block, for baseline-establishing purposes -- we end giving the flex item a baseline at the bottom of its own content-box.  This is the same as what happens if the flex item has an explicitly-empty first child, or a first child <div> that only contains placeholders.

So I think this works "correctly", given the abspos behavior that we implement right now (with their placeholders being wrapped in abspos flex items). Definitely a good scenario to keep in mind when we implement the spec's updated abspos behavior in bug 874718, though.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> If the anonymous flex item (a
> block) *only* contains a placeholder (or placeholders), then it just behaves
> the same as an empty block, for baseline-establishing purposes -- we end
> giving the flex item a baseline at the bottom of its own content-box.

Sorry, meant "giving the flex container" (not item) in that last line there.

Comment 8

[Daniel Holbert [:dholbert]]

I filed bug 1124772 with a testcase along the lines of the scenario described in comment 5 - 7 here.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8552908 [details] [diff] [review]
part 1: reliably cache each flex item's ascent, if there's a chance we'll need it

Ah, now I remember, we discussed removing the wrapping of placeholders
before since it also affect Grid.  And I actually have that change in
my patch queue so that's why I thought it was a problem here.

Anyway, replacing the "== mFrames.FirstChild()" checks with a
nsFlexContainerFrame::IsFirstItem(const nsFlexItem&) method seems
like a good idea.  Here or in a future bug as you find convenient.

Comment 10

[Daniel Holbert [:dholbert]]

Good call -- I'll opt to do that in a future bug (probably as part of bug 874718, which will have to do more explicit checking for & separating of placeholders already).

Thanks for the reviews. Landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/3080c6c9ff92
 https://hg.mozilla.org/integration/mozilla-inbound/rev/72fa4790415b
 https://hg.mozilla.org/integration/mozilla-inbound/rev/490ab676c5a4

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3080c6c9ff92
https://hg.mozilla.org/mozilla-central/rev/72fa4790415b
https://hg.mozilla.org/mozilla-central/rev/490ab676c5a4