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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
P1
normal
RESOLVED FIXED
17 years ago
Last year

People

(Reporter: karnaze, Assigned: bzbarsky)

Tracking

(Depends on 1 bug, {testcase})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P3])

Attachments

(3 attachments, 2 obsolete attachments)

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.
Blocks: 121142
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.0.1
OS: Windows 2000 → All
Hardware: PC → All
Blocks: 30378, 68061
Blocks: 72500
Posted patch wip patch (obsolete) — Splinter Review
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).
Blocks: 162063
Whiteboard: WIP PATCH
Target Milestone: mozilla1.0.1 → mozilla1.3alpha
Adding helpwanted keyword per comment 2.
Keywords: helpwanted
Target Milestone: mozilla1.3alpha → Future
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).
mass reassign to default owner
Assignee: karnaze → table
Status: ASSIGNED → NEW
QA Contact: amar → madhur
Target Milestone: Future → ---
Target Milestone: --- → Future
Keywords: testcase
Blocks: 203923
Blocks: 234021
Blocks: 245748
Blocks: 257199
No longer blocks: 257199
*** Bug 257199 has been marked as a duplicate of this bug. ***
Blocks: 274211
Blocks: 281526
Blocks: 302113
Blocks: 325543
Assignee: layout.tables → nobody
QA Contact: madhur → layout.tables
Blocks: 352937
Duplicate of this bug: 420196
marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.
Flags: wanted1.9.1?
Priority: P2 → P1
Assignee: nobody → dbaron
Whiteboard: WIP PATCH → [Hixie-P3] WIP PATCH
Blocks: 368932
Adding my testcase from bug 368932 which might well be a dupe...
Flags: wanted1.9.1? → wanted1.9.1+
Duplicate of this bug: 450410
Duplicate of this bug: 467191
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.)
Blocks: 467978
Duplicate of this bug: 470541
Blocks: 371054
Blocks: 326545
Blocks: 448111
Posted patch Proposed fix (obsolete) — Splinter Review
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.
Assignee: dbaron → bzbarsky
Attachment #86323 - Attachment is obsolete: true
Attachment #368567 - Flags: superreview?(roc)
Attachment #368567 - Flags: review?(bernd_mozilla)
Keywords: helpwanted
Summary: Handle content appended, inserted, deleted for pseudo (anonymous) frames. → [FIX]Handle content appended, inserted, deleted for pseudo (anonymous) frames.
Whiteboard: [Hixie-P3] WIP PATCH → [Hixie-P3]
+      nextSibling->GetStyleContext()->GetPseudoType() !=
+        aFrame->GetStyleContext()->GetPseudoType()) {

How can aFrame->GetStyleContext()->GetPseudoType() be non-null? Couldn't this just be a check against null?
is it guaranteed that we do not recreate frames twice inside nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval ?

I am concerned about the two "adjacent" calls to RecreateFramesForContent
Blocks: 394402
Blocks: 484448
No longer blocks: 162063
Depends on: 162063
> 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?
Blocks: 208305
Bernd, see comment 18.
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?
It's moot; that check has been removed.  But before that it was just a bad merge against the patch from bug 162063.  ;)
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.
I am not sure that the function name  WipeContainingBlock reflects what is going on inside if the pseudo handling is added.
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?
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/
No longer blocks: 203923
Duplicate of this bug: 467978
No longer blocks: 302113
Attachment #368567 - Attachment is obsolete: true
Attachment #369372 - Flags: superreview?(roc)
Attachment #369372 - Flags: review?(bernd_mozilla)
Attachment #368567 - Flags: superreview?(roc)
Attachment #368567 - Flags: review?(bernd_mozilla)
Attachment #369372 - Flags: superreview?(roc) → superreview+
Attachment #369372 - Flags: review?(bernd_mozilla) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/6b3cc966ef52.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 246669
Blocks: 363326
Blocks: 443616
Depends on: 497575
Blocks: 500992
Duplicate of this bug: 503548
Duplicate of this bug: 294065
Blocks: 387227
Duplicate of this bug: 509990
Duplicate of this bug: 523096
Target Milestone: Future → mozilla1.9.2a1
Depends on: 541374
Blocks: 297484
You need to log in before you can comment on or make changes to this bug.