873172 - Assertion failure: "Frame list should've been sorted in reflow" with flexbox, :after, table-cell

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: nsLayoutUtils::IsFrameListSorted<IsOrderLEQWithDOMFallback>(mFrames) (Frame list should've been sorted in reflow), at layout/generic/nsFlexContainerFrame.cpp:1111

Similar to bug 826483, which dholbert fixed two weeks ago.

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Daniel Holbert [:dholbert]]

So, bug 867454 added a special-case for flex container's children that have ":after".

However, in this case, the :after frame isn't a *child* of the flex container -- it's wrapped inside several anonymous table boxes. (since it's a table-cell frame)

We should be able to use the existing logic in GetContentForComparison() to descend through those anonymous frames.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch converts the existing "GetContentForComparison" helper-function into a more targeted "GetFirstNonAnonBoxDescendant" method, and calls it slightly earlier, so that we can benefit from it when checking for ::before and ::after.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1a [just tweaked a comment]

Oops; noticed a stale (& now duplicate) comment about << Same "order" value >>. 

I tweaked it to instead say "Usual case" (as opposed to the Special case above it, for ::before/::after)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750747 [details] [diff] [review]
fix v1a [just tweaked a comment]

Ah, very nice!  r=me

Comment 6

[Daniel Holbert [:dholbert]]

Thanks!

Try run, with the patches for this bug and Bug 825810:
https://tbpl.mozilla.org/?tree=Try&rev=21312c3ec51b

Comment 7

[Daniel Holbert [:dholbert]]

Apparently I just eyeballed the reftest instead of actually running it, so it turns out it fails, but it fails in a way that's actually expected, so it's a test-bug and not a code-bug.

The test fails because we're ignoring "align-self" on the ::before/::after table-cell and table-row elements. This is correct behavior, because align-self only applies to flex items, and the cell/row aren't themselves flex items -- they get wrapped in tables, and the *tables* are the flex items.

So, I'm just going to rename the reftest to "flexbox-with-pseudo-elements-3.html" (instead of "-1b") and give it its own reference case.  (The reference case will be similar to flexbox-with-pseudo-elements-1-ref.html, but without the "align-self" settings.)

(Note: the reftest also happens to aborts in debug builds when the patch for bug 825810 is also applied, as it was in that Try run -- but that's an issue in that bug's patch & it has a relatively straightforward solution -- I'll discuss that abort issue over on bug 825810.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v1b [fixed reftest] [r=bz]

Here's the patch with that fixed.  Carrying forward r=bz.

Try run with that fixed (just reftests, & just this patch -- not bug 825810 anymore):  https://tbpl.mozilla.org/?tree=Try&rev=8a071ff5e5b4

Comment 9

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e10e157323

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56e10e157323