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)

defect
Not set
normal

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.
>  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.)
(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.)
(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 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 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+
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.
You need to log in before you can comment on or make changes to this bug.