The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Layout: Tables
P1
normal
RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: karnaze (gone), Assigned: bz)

Tracking

(Depends on: 1 bug, {testcase})

Trunk
mozilla1.9.2a1
testcase
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)

(Reporter)

Description

15 years ago
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.
(Reporter)

Updated

15 years ago
Blocks: 121142
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.0.1

Updated

15 years ago
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Updated

15 years ago
Blocks: 30378, 68061

Updated

15 years ago
Blocks: 72500
(Reporter)

Comment 1

15 years ago
Created attachment 86317 [details]
test case with a very few examples (need a lot more)
(Reporter)

Comment 2

15 years ago
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).
Blocks: 162063
(Reporter)

Updated

15 years ago
Whiteboard: WIP PATCH
Target Milestone: mozilla1.0.1 → mozilla1.3alpha

Comment 3

14 years ago
Adding helpwanted keyword per comment 2.
Keywords: helpwanted
Target Milestone: mozilla1.3alpha → Future

Comment 4

14 years ago
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).
(Reporter)

Comment 5

14 years ago
mass reassign to default owner
Assignee: karnaze → table
Status: ASSIGNED → NEW
QA Contact: amar → madhur
Target Milestone: Future → ---

Updated

14 years ago
Target Milestone: --- → Future

Updated

14 years ago
Keywords: testcase
Blocks: 203923
Blocks: 234021
Blocks: 245748
Blocks: 257199

Updated

13 years ago
No longer blocks: 257199

Comment 6

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

Updated

12 years ago
Blocks: 274211
Blocks: 281526
Blocks: 302113
Blocks: 325543
Assignee: layout.tables → nobody
QA Contact: madhur → layout.tables
Blocks: 352937
Blocks: 372649

Updated

9 years ago
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

Updated

9 years ago
Assignee: nobody → dbaron
Whiteboard: WIP PATCH → [Hixie-P3] WIP PATCH

Updated

9 years ago
Blocks: 368932

Comment 9

9 years ago
Created attachment 327624 [details]
Removing and appending rows in a table

Adding my testcase from bug 368932 which might well be a dupe...
Flags: wanted1.9.1? → wanted1.9.1+
Duplicate of this bug: 450410

Updated

8 years ago
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.)

Updated

8 years ago
Blocks: 467978
Duplicate of this bug: 281526

Updated

8 years ago
Duplicate of this bug: 470541

Updated

8 years ago
Blocks: 371054

Updated

8 years ago
Blocks: 326545
Blocks: 472870
Blocks: 448111
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.
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?

Comment 17

8 years ago
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.

Comment 20

8 years ago
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.

Comment 23

8 years ago
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?

Comment 25

8 years ago
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
Created attachment 369372 [details] [diff] [review]
Updated to comments, now with more tests
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+

Updated

8 years ago
Attachment #369372 - Flags: review?(bernd_mozilla) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/6b3cc966ef52.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 246669
Blocks: 363326
Blocks: 443616

Updated

8 years ago
Depends on: 497575

Updated

8 years ago
Blocks: 500992
Duplicate of this bug: 503548

Updated

8 years ago
Duplicate of this bug: 294065
Blocks: 387227

Updated

8 years ago
Duplicate of this bug: 509990

Updated

8 years ago
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.