Last Comment Bug 148810 - [FIX]Handle content appended, inserted, deleted for pseudo (anonymous) frames.
: [FIX]Handle content appended, inserted, deleted for pseudo (anonymous) frames.
Status: RESOLVED FIXED
[Hixie-P3]
: testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: P1 normal with 14 votes (vote)
: mozilla1.9.2a1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
: 257199 281526 294065 420196 450410 467191 467978 470541 503548 509990 (view as bug list)
Depends on: 497575 162063 541374
Blocks: 30378 68061 72500 121142 208305 234021 245748 246669 274211 281526 297484 325543 326545 352937 363326 368932 371054 372649 387227 394402 443616 448111 467978 472870 484448 500992
  Show dependency treegraph
 
Reported: 2002-06-03 09:15 PDT by karnaze (gone)
Modified: 2011-01-26 11:04 PST (History)
36 users (show)
roc: wanted1.9.1+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case with a very few examples (need a lot more) (2.35 KB, text/html)
2002-06-04 16:43 PDT, karnaze (gone)
no flags Details
wip patch (216.83 KB, patch)
2002-06-04 16:53 PDT, karnaze (gone)
no flags Details | Diff | Splinter Review
Removing and appending rows in a table (1.11 KB, text/html)
2008-07-01 10:19 PDT, Marc Diethelm
no flags Details
Proposed fix (61.07 KB, patch)
2009-03-20 12:06 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Updated to comments, now with more tests (75.88 KB, patch)
2009-03-25 14:50 PDT, Boris Zbarsky [:bz]
bernd_mozilla: review+
roc: superreview+
Details | Diff | Splinter Review

Description karnaze (gone) 2002-06-03 09:15:49 PDT
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) 2002-06-04 16:43:11 PDT
Created attachment 86317 [details]
test case with a very few examples (need a lot more)
Comment 2 karnaze (gone) 2002-06-04 16:53:44 PDT
Created attachment 86323 [details] [diff] [review]
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. 2003-03-23 20:20:29 PST
Adding helpwanted keyword per comment 2.
Comment 4 Greg K. 2003-03-24 18:26:27 PST
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) 2003-03-31 11:29:43 PST
mass reassign to default owner
Comment 6 Bernd 2004-08-29 23:37:15 PDT
*** Bug 257199 has been marked as a duplicate of this bug. ***
Comment 7 Kevin Yank 2008-03-01 14:35:53 PST
*** Bug 420196 has been marked as a duplicate of this bug. ***
Comment 8 Damon Sicore (:damons) 2008-06-04 16:53:55 PDT
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 2008-07-01 10:19:52 PDT
Created attachment 327624 [details]
Removing and appending rows in a table

Adding my testcase from bug 368932 which might well be a dupe...
Comment 10 Boris Zbarsky [:bz] 2008-10-01 21:50:33 PDT
*** Bug 450410 has been marked as a duplicate of this bug. ***
Comment 11 Bernd 2008-11-30 01:33:59 PST
*** Bug 467191 has been marked as a duplicate of this bug. ***
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-12-02 20:37:49 PST
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 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-12-14 09:03:27 PST
*** Bug 281526 has been marked as a duplicate of this bug. ***
Comment 14 Bernd 2008-12-20 09:48:51 PST
*** Bug 470541 has been marked as a duplicate of this bug. ***
Comment 15 Boris Zbarsky [:bz] 2009-03-20 12:06:52 PDT
Created attachment 368567 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2009-03-21 00:59:12 PDT
+      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 2009-03-21 10:28:58 PDT
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 [:bz] 2009-03-22 12:14:03 PDT
> 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 [:bz] 2009-03-23 12:08:20 PDT
Bernd, see comment 18.
Comment 20 Bernd 2009-03-23 23:21:49 PDT
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 [:bz] 2009-03-24 06:38:02 PDT
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 [:bz] 2009-03-24 21:04:05 PDT
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 2009-03-24 22:41:16 PDT
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 [:bz] 2009-03-24 22:46:37 PDT
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 2009-03-24 23:40:45 PDT
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 26 Boris Zbarsky [:bz] 2009-03-25 14:21:55 PDT
*** Bug 467978 has been marked as a duplicate of this bug. ***
Comment 27 Boris Zbarsky [:bz] 2009-03-25 14:50:41 PDT
Created attachment 369372 [details] [diff] [review]
Updated to comments, now with more tests
Comment 28 Boris Zbarsky [:bz] 2009-03-26 18:36:46 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/6b3cc966ef52.
Comment 29 Boris Zbarsky [:bz] 2009-07-11 01:31:03 PDT
*** Bug 503548 has been marked as a duplicate of this bug. ***
Comment 30 Bernd 2009-07-19 08:59:40 PDT
*** Bug 294065 has been marked as a duplicate of this bug. ***
Comment 31 philippe (part-time) 2009-08-13 01:33:55 PDT
*** Bug 509990 has been marked as a duplicate of this bug. ***
Comment 32 Kevin Brosnan 2009-10-19 07:39:40 PDT
*** Bug 523096 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.