148810 - [FIX]Handle content appended, inserted, deleted for pseudo (anonymous) frames.

Status: RESOLVED FIXED

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

Description

[karnaze (gone)]

The existing implementation of constructing pseudo frames (e.g. a cell inserted 
into a table causes a pseudo row and row group frame to be contstructed) does 
not work correctly when content is appended or inserted. Also, when content is 
removed, pseudo frames likely do not get destroyed when they should. This is a 
meta bug which will result in a fair amount of changes to frame construction and 
table code.

Comment 1

[karnaze (gone)]

Attachment: test case with a very few examples (need a lot more)

Comment 2

[karnaze (gone)]

Attachment: wip patch

I'm attaching this patch so it doesn't get lost. There are more test cases to
be developed, pseudo frames need to go away when their last child goes away,
the regression tests need to be run (bug 113235-1.html has a problem, and since
the child lists have changed, there are probably going to be differences on
every file).

Comment 3

[Greg K.]

Adding helpwanted keyword per comment 2.

Comment 4

[Greg K.]

Note that the summary doesn't cover all the ramifications of this problem.
Content that simply takes time to load or is interrupted by something will
exhibit this problem (see attachment 94811 [details] on bug 121142).

Comment 5

[karnaze (gone)]

mass reassign to default owner

Comment 6

[Bernd]

*** Bug 257199 has been marked as a duplicate of this bug. ***

Comment 8

[Damon Sicore (:damons)]

marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.

Comment 9

[Marc Diethelm]

Attachment: Removing and appending rows in a table

Adding my testcase from bug 368932 which might well be a dupe...

Comment 12

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

I think we can get away here with something much like ReframeContainingBlock, which we use for block-within-inline situations.

However, I think both ReframeContainingBlock and this would benefit a lot from the ability to reconstruct all the descendants of a particular frame without rebuilding that frame itself (at least assuming that for the frame in question, GetPrimaryFrameFor(mContent)->GetContentInsertionFrame() == this).  This would give us the ability to prevent these reconstruct-something-higher from cascading upwards in a number of cases.  (For example, in this case, it would prevent us from ever having to reconstruct stuff that's an ancestor of the primary frame of the content parent, since we could just reconstruct that frame's descendants.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This fixes this bug for me, as far, as well as the various bugs this blocks.  The patch adds some tests, but not nearly exhaustive; I also still need to add tests from this bug and various blocked bugs.

The patch depends on the patch in bug 162063.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      nextSibling->GetStyleContext()->GetPseudoType() !=
+        aFrame->GetStyleContext()->GetPseudoType()) {

How can aFrame->GetStyleContext()->GetPseudoType() be non-null? Couldn't this just be a check against null?

Comment 17

[Bernd]

is it guaranteed that we do not recreate frames twice inside nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval ?

I am concerned about the two "adjacent" calls to RecreateFramesForContent

Comment 18

[Boris Zbarsky [:bzbarsky]]

> How can aFrame->GetStyleContext()->GetPseudoType() be non-null?

Hmm.  I guess since we're guaranteed aFrame is the primary frame, that can't happen.  Which means this check should just go away.  I was trying to limit to the case where we're just removing a pseudo-frame before I changed the way bug 162063 is working.  Good catch.

> is it guaranteed that we do not recreate frames twice

Not sure what you mean.  I suspect the answer is "no".

> I am concerned about the two "adjacent" calls to RecreateFramesForContent

Which ones?  Right now we have returns after the RecreateFramesForContent calls, no?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Bernd, see comment 18.

Comment 20

[Bernd]

yep, 

another question why do you check aFrames pseudo type instead of inflows pseudo type 

+      nextSibling->GetStyleContext()->GetPseudoType() !=
+        aFrame->GetStyleContext()->GetPseudoType()) {

or is this moot as outlined in comment #18?

Comment 21

[Boris Zbarsky [:bzbarsky]]

It's moot; that check has been removed.  But before that it was just a bad merge against the patch from bug 162063.  ;)

Comment 22

[Boris Zbarsky [:bzbarsky]]

And to be clear, I'll post an updated patch soon; I just want to finish writing tests for the bugs depending on this one...  Hopefully tomorrow.

Comment 23

[Bernd]

I am not sure that the function name  WipeContainingBlock reflects what is going on inside if the pseudo handling is added.

Comment 24

[Boris Zbarsky [:bzbarsky]]

We already have code that does what the pseudo handling does in that function, for what it's worth: the "situation #1" for XUL boxes.  Or do I misunderstand the concern?

Comment 25

[Bernd]

No you did understand it perfectly, I always assumed from the function name that this is something special for blocks and the last time I looked at this function (2005) it was more or less like this. I am not insisting on a name change but it seems to me that this function is walking up till it finds a "real" frame which is not necessarily a block.

regardless please fix the comment at

10745   
10746   // Before we go and append the frames, we must check for two
10747   // special situations.

s/two/three/

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments, now with more tests

Comment 28

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/6b3cc966ef52.