Closed
Bug 1313421
Opened 8 years ago
Closed 8 years ago
When synthesizing flex baseline from first item, don't trust child-list, since child might be a placeholder
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(3 files)
In some scenarios, we synthesize the flex container's baseline from its first flex item: https://drafts.csswg.org/css-flexbox-1/#flex-baselines Right now, we do that via testing our flex-items to see if they're mFrames.FirstChild(), as we reflow them: for (const FlexLine* line = lines.getFirst(); line; line = line->getNext()) { for (const FlexItem* item = line->GetFirstItem(); item; item = item->getNext()) { [...] // If this is our first child and we haven't established a baseline for // the container yet (i.e. if we don't have 'align-self: baseline' on any // children), then use this child's baseline as the container's baseline. if (item->Frame() == mFrames.FirstChild() && flexContainerAscent == nscoord_MIN) { flexContainerAscent = itemNormalBPos + item->ResolvedAscent(); https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/layout/generic/nsFlexContainerFrame.cpp#4246 BUT: as of bug 1269045, we'll start having nsPlaceholderFrames mixed among our mFrames frame-list. (Until now, they've always been wrapped in anonymous blocks, so they'd never be direct children.) So after bug 1269045, mFrames.FirstChild() might be a placeholder, in which case the mFrames.FirstChild() check that I quoted above will be bogus (and we won't end up correctly resolving the flex container's baseline from its first item). We can fix this by simply making our first-item check against the actual first flex item from our |lines| structure (the list of flex lines, which contain all our flex items). We can make this change before bug 1269045 lands, too, which will be nice because then bug 1269045 won't break this when it does land.
Assignee | ||
Comment 1•8 years ago
|
||
> in which case the mFrames.FirstChild() check that I quoted above will be bogus
(To be clear: it'll be bogus because placeholders are not flex items, and we won't even be iterating across them in this loop. So that equality comparison will simply never succeed, if mFrames.FirstChild() happens to be a placeholder.)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
(Heads-up brad: this might bitrot bug 1221524 (last-baseline support) slightly -- sorry! -- hopefully it won't bitrot it too much. We need this fix, independent of bug 1221524's new functionality, in order for bug 1269045 / bug 1269046 to be landable -- and I'd like to get those in ASAP. Otherwise, I probably would've layered this on top of bug 1221524.)
Comment 6•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #5) > (Heads-up brad: this might bitrot bug 1221524 Not a problem. I'll clean up proposed patches for bug 1221524 when this lands.
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8805260 [details] Bug 1313421 part 1: Unconditionally cache a FlexItem's ascent, after it's been reflowed. https://reviewboard.mozilla.org/r/89042/#review88196
Attachment #8805260 -
Flags: review?(mats) → review+
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8805261 [details] Bug 1313421 part 2: Use flex container's FlexLine linked-list to determine the first flex item, rather than its child-frame list. https://reviewboard.mozilla.org/r/89044/#review88198
Attachment #8805261 -
Flags: review?(mats) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Thanks! Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b1c7520300148b3a4b85f905b60d1237c48931d
Comment 10•8 years ago
|
||
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c565ba669d96 part 1: Unconditionally cache a FlexItem's ascent, after it's been reflowed. r=mats https://hg.mozilla.org/integration/mozilla-inbound/rev/ae632144724f part 2: Use flex container's FlexLine linked-list to determine the first flex item, rather than its child-frame list. r=mats https://hg.mozilla.org/integration/mozilla-inbound/rev/1658a195f669 part 3: Extend single-flex-item reftest to test scenario with abspos child before flex item.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c565ba669d96 https://hg.mozilla.org/mozilla-central/rev/ae632144724f https://hg.mozilla.org/mozilla-central/rev/1658a195f669
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•