7.35 KB, patch
|Details | Diff | Splinter Review|
10.50 KB, patch
|Details | Diff | Splinter Review|
1.73 KB, patch
|Details | Diff | Splinter Review|
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Created attachment 8552908 [details] [diff] [review] 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.)
Attachment #8552908 - Flags: review?(mats)
Created attachment 8552914 [details] [diff] [review] 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.)
Attachment #8552914 - Flags: review?(mats)
Created attachment 8552932 [details] [diff] [review] 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).
Attachment #8552932 - Flags: review?(mats)
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?
(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.
(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.
I filed bug 1124772 with a testcase along the lines of the scenario described in comment 5 - 7 here.
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.
Attachment #8552908 - Flags: review?(mats) → review+
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
https://hg.mozilla.org/mozilla-central/rev/3080c6c9ff92 https://hg.mozilla.org/mozilla-central/rev/72fa4790415b https://hg.mozilla.org/mozilla-central/rev/490ab676c5a4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.