386014 - [FIXr]"ASSERTION: Should have found a parent before this" with a position:fixed table and captions

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Jesse Ruderman]

###!!! ASSERTION: Should have found a parent before this: 'aProspectiveParent->GetStyleContext()->GetPseudoType() == nsCSSAnonBoxes::viewportScroll', mozilla/layout/generic/nsFrame.cpp, line 5540

This assertion also showed up in bug 378240, which is now fixed.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

So... is it me, or is dynamically inserting a caption into a table generally broken on trunk?

In any case, this assert is firing for a good reason: we're using the wrong style context parent for the caption here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Looks like dynamic caption insertion in this case got broken by bug 309322.  But on trunk only, not branch...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, it hasn't landed on branch...

Comment 5

[Boris Zbarsky [:bzbarsky]]

I filed bug 389924 on that issue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

The idea here is to make ContentInserted more like ContentAppended and the codepath which constructs a table frame and then its kids.  For both ContentAppended and that codepath, the parent used for ConstructFrame() when doing the caption is the _inner_ table frame (for ContentAppended() because GetFrameFor() gets the content insertion frame).  The caption construction code adjusts this as needed later.

Blow by blow:

Hunk 1: Remove code that's no longer needed because we never pass outer tables to AdjustParentFrame.  Fix some indentation.

Hunk 2: Replace the code that finally gets the right parent for the caption by a function that does exactly what this code is doing.

Hunk 3: Remove code that's no longer needed because we never pass outer tables to GetAdjustedParentFrame.

Hunk 4: Comment change

Hunk 5: Remove code that is not needed because AdjustParentFrame handles that situation (and better, since it checks for special kids).

Hunk 6: First part that's needed to actually fix this bug: when getting a parent based on prev or next sibling, get that parent's content insertion frame.  This makes our prospective parent the same as other codepaths.

Hunk 7: Same as hunk 5.

Hunk 8: Adjust check for valid prevSibling a little, since now parentFrame may not actually be prevSibling->GetParent()

Hunk 9: Same as hunk 8, for captions.

The rest is tests.  Test 1a (the situation in this testcase) fails without this patch, because the background is not actually inherited from the right place.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274254 [details] [diff] [review]
Proposed fix

Fix incorrect style context parenting for captions when using ContentInserted.  Mostly removes redundant code, actually, and makes this codepath act like our other (more commonly used) codepaths for captions.  Risk is low.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 274254 [details] [diff] [review]
Proposed fix

a19=dbaron

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.