This is a bug to track the various issues caused by the following problem: During frame construction we create "pseudo" frames when non-table-related frames are inside a table but not inside a table cell. Similarly, we create "pseudo" frames when a cell is not inside a table. The problem is that if a dynamic modification of some sort happens (changing the display of the frame that forced the creation of the "pseudo" frames, removing it from the DOM, etc) the "pseudo" frames do not get destroyed. They stay in the frame tree. Bug 156888 and bug 97506 (last testcase; the one in bug 97506 comment 14) have testcases that demonstrate the problem. It seems to me that we want to somehow mark a frame that forced pseudo-frames to be created and have a way of getting from that frame to the outermost pseudo-frame it forced. Then ContentRemoved (which both DOM removal and frame reconstruction go through) should tear down not just the primary frame, but also all the pseudos it forced into being. I tried hacking this together, but have found that getting at the newly-constructed frame and the outermost pseudo at the same time is not so easy... ;)

As a note, we have a bunch of "INVALID" bugs that this would actually fix, I think... People like to set table rows to "block", apparently.

I've been thinking... it seems to me that the best thing would be for the nsPseudoFrames to hold a pointer to the "topmost" pseudo frame that has been created. Would that make sense?

I'm not sure if the (unfinished) patch bug 148810 deals with this, but a pseudo frame has the same content as its parent, which is a fairly simple check. I can't remember if there are non-pseudo frames where this is true.

mass reassign to default owner

Could we just set a framestate bit on pseudo table/row/cell/whatever frames? Then when content is removed we can check that its frame is the only child of its parent, and if it is walk up the parents till we get to a frame that doesn't have the bit set, then remove everything below that....

This tries to implement comment 3, while it really detects the removable pseudoframes, the usage is horked and I dont yet understand why.

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

This does at least the job, while most of the testcases are rendered now anyway better after the empty ascent bug got fixed.

That doesn't seem to handle correctly the case when the child being removed has previous siblings but not next sibling, does it?

taking to get this moved

Fixing this should watch out for the issues found in bug 326834.

With Bernd's permission I've made an improved patch that address the remaining problems: 1. look for previous siblings (Boris' comment 9) 2. when dealing with out-of-flow's (bug 339388) we need to adjust the frame also when removing the placeholder. 3. when dealing with captions (bug 293576) I've added a test for the case where we an empty pseudo inner table frame (which shouldn't block the removal of the pseudo outer when removing the last caption) 4. As foreseen by the oracle in comment 11, I did indeed bump into problems with regressing bug 326834. I've added a wallpaper for that (an early return for XUL). I've tested all the attached testcases of the dependent bugs and I think all of them are now fixed. No problems in regression tests etc.

Comment on attachment 224550 [details] [diff] [review] Patch rev. 2 Mats, could you add -p to your diff options, in general? It'd be nice to know where the second chunk of this patch lives without having to open nsCSSFrameConstructor. ;) I'm not sure I follow the "early return for XUL" logic here. XUL with table frame types should behave like HTML with table frame types, no? Why not just have GetPseudoAdjustedChild return an nsIFrame*? That would be better, I think. We change the parentFrame pointer in ContentRemoved, right? Do we need to adjust the first-letter stuff accordingly? I suspect that we do... If nothing else, if I have something like: <div> <span style="display: table-cell"> Text </span> More text </div> And the div has a first-letter style, and then I remove the span, I doubt that the "M" becomes styled with the first-letter style with this patch. >+ // If this is a row group then there should be nothing else than anonymous >+ // cellbased column frame. "nothing other than an" I'd kinda like bernd to review this part of the code... >+ else if (parentPseudo == nsCSSAnonBoxes::cellContent) { Don't you want to set aAdjChildFrame to the table cell in this case? Or is it ok because we just recurse with the cellContent as aChildFrame and fall right through the if/else cascade to the next recursion level? >+ else if (parentPseudo == nsCSSAnonBoxes::tableCol) { >+ NS_ASSERTION(PR_FALSE, "illegal colFrame children detected"); NS_ERROR >+ return; >+ } >+ else if (parentPseudo != nsCSSAnonBoxes::tableRowGroup && Why bother with the else? Looks good other that these issues, but I'd like to see an updated patch.

(In reply to comment #13) > Mats, could you add -p to your diff options, in general? Oops, sorry, I always do but I forgot it in this case... > I'm not sure I follow the "early return for XUL" logic here. XUL with table > frame types should behave like HTML with table frame types, no? Yes, what I wanted to say is that appearantly some frame trees for XUL are sensitive in what you do with them... at least I don't understand how to deal with this case. But I was probably overly pessimistic and I have limited the wallpaper to <listboxbody>/<listitem> in this patch. If you know any details on why it crashes I'd be happy to try to fix it. I think we can spawn off a separate bug on this case though. > Why not just have GetPseudoAdjustedChild return an nsIFrame*? Fixed > We change the parentFrame pointer in ContentRemoved, right? Yes >Do we need to adjust the first-letter stuff accordingly? I'm not sure what you mean... the |RecoverLetterFrames()| near the end of the patch uses the new value for |parentFrame| and the code looks ok to me... Testcase #1 makes 'M' the first letter when removing the <span> or setting its display to 'none'. I can't think of a case where removing a first-letter/first-line frame could trigger the removal of a pseudo table frame (if that's what you were thinking of). > I'd kinda like bernd to review this part of the code... Bernd wrote this bit in his original patch. FWIW, I think it looks fine, but I don't claim to know much about the possible table frame trees. > >+ else if (parentPseudo == nsCSSAnonBoxes::cellContent) { > > Don't you want to set aAdjChildFrame to the table cell in this case? Or is it > ok because we just recurse with the cellContent as aChildFrame and fall right > through the if/else cascade to the next recursion level? Yes, we would handle it on the next level, but I changed the code slightly for this block now, we can test directly if the grand parent is a pseudo and if not return directly, and in this case the Type should be tableCellFrame, right? (In other words, a cellContent pseudo is either in a cell or pseudo cell and it's always the only child and it should never be removed unless the cell is.) > NS_ERROR Fixed > Why bother with the else? Because if we took any of the previous forks we could fall through and we don't want to test |parentPseudo| again?

I should also say that I found a sequence of events in bug 339388 that is not completely fixed by any of these patches. It's rather complex and I don't think it is anything we can fix in GetPseudoAdjustedChild(), no matter how we write it. I'll keep bug 339388 open and explain the details there. The patch fixes the simple testcases (#2 and #3) in that bug though.

(In reply to comment #15) > (In reply to comment #13) > > Mats, could you add -p to your diff options, in general? > > Oops, sorry, I always do but I forgot it in this case... You can create a ~/.cvsrc file with default options (even on windows, although where cygwin thinks your homedir is may be a little interesting). For example, mine is: cvs -z3 -q diff -pNu12 update -P checkout -P

Cool, thanks. I'll do that.

> If you know any details on why it crashes I'd be happy to try to fix it. Kids of listbox don't go through normal frame construction. See the NotifyListBoxBody call at the top of ContentRemoved. You want to modify that code to get the updated child frame and pass _that_ to NotifyListBoxBody. Otherwise we'll first pass in the wrong frame there (so NotifyListBoxBody will ignore it due to the fix for bug 326834) and then will mess with the listbox's kids manually, which is bad. > the |RecoverLetterFrames()| near the end of the patch uses the new value for > |parentFrame| But the old value for |haveFLS|, no? And the old value for |containingBlock|. I'll attach a testcase where the latter causes a crash. If it's fixed, the former causes an incorrect frame model.

(In reply to comment #19) Ok, I see what you mean, I'll fix those issues. There also a bug in the existing first-letter code: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1235&root=/cvsroot&mark=9965#9963 In case |childFrame| is an out-of-flow we'd want the containing block of its placeholder instead... I'll fix that too while I'm here...

Changes since last revision: 1. pass a pseudo adjusted frame to NotifyListBoxBody() 2. use the containing block of the placeholder when removing first-letter frames when |childFrame| is out-of-flow. 3. re-calculate |haveFLS| and |containingBlock| if remove a pseudo ancestor instead the original frame. 4. instead of having a "if (childFrame) {" cover most of ContentRemoved() I changed it to an early return instead.

Comment on attachment 225377 [details] [diff] [review] Patch rev. 4 (diff -w) I just got this when running some evil tests with this patch: ###!!! ASSERTION: can't remove inner frame: 'nsLayoutAtoms::captionList == aListName', file nsTableOuterFrame.cpp, line 268 so this patch still has some problem...

I guess we should not remove an empty pseudo inner table if the outer table still has a caption... I'm guessing it's ok for the inner to be empty though?

Yeah, the inner can be empty in that case.

>I guess we should not remove an empty pseudo inner table if the outer table >still has a caption... Its mandatory to have the inner once we have the outer, we need it at least for the bizarre parent style relationship that both have. >I'm guessing it's ok for the inner to be empty though? Yep, thats normal you can even size it :-), OK the chance are high that your nick name has somewhere a 'hixie' in it when you do this but its legal.

Changes since last revision: 1. don't remove the inner table frame unless the outer is too 2. include 'bcTableCellFrame' in the assertion that verifies the parent of 'cellContent'

Mats, I'm going to try to get to this in the next few days, but if that doesn't happen it'll be after July 13. :(

Comment on attachment 225694 [details] [diff] [review] Patch rev. 5 (diff -w) >Index: layout/base/nsCSSFrameConstructor.cpp >+ RemoveLetterFrames(presContext, mPresShell, frameManager, containingBlock); So we remove letter frames on the containing block of parentFrame. >+ // If we have pseudo frames that can go away when removing |frameToRemove| >+ // then remove them too. If so, adjust |containingBlock| accordingly for the >+ // RecoverLetterFrames() call below. But possibly recover on some other block? That seems wrong to me... Other than that, the patch looks good.

(In reply to comment #32) > But possibly recover on some other block? That seems wrong to me... I was thinking of the case where we have: <block> <pseudos> <frame-to-remove> <sibling> If any of the <pseudos> is the containing block then we will do RemoveLetterFrames() on that and then when it is removed we need to create first-letter frames in <block> because <pseudos> could have been blocking us from reaching <sibling>... but you're right, we would need to do a RemoveLetterFrames() on the new containing block for that to work correctly. So, we could do both RemoveLetterFrames() + RecoverLetterFrames() when the containing block changed or we could simply skip RecoverLetterFrames(). I think I'll do the latter for now... although in general, I think that removing <pseudos> could enable new opportunities for first-letter's in <block> and we should do more to restore them according to the new frame tree. But, first-letter is quite broken as is already and I'm guessing the reflow-branch is the solution to that problem?

Reflow branch won't affect any of this code, actually. I'll try to get to this review in the next few days. Thanks for beating on this, Mats!

Comment on attachment 228591 [details] [diff] [review] Patch rev. 6 (diff -w) I'm sorry I'm being so slow with these. :( Next one should be a lot faster; I've put this on my "get this done quick" list... >Index: layout/base/nsCSSFrameConstructor.cpp >+GetPseudoAdjustedChild(nsIFrame* aParentFrame, >+ nsIFrame* firstChild = aParentFrame->GetFirstChild(nsnull); >+ if (aChildFrame->GetNextSibling() || >+ (aChildFrame != firstChild && >+ aChildFrame->GetType() != nsLayoutAtoms::tableCaptionFrame)) { >+ return aChildFrame; // The child has a sibling (caption is handled below). So consider the following markup: <div style="display: table-row"> <div style="float: right"> </div> </div> This will create an anonymous cell and put the float in it. Now say I remove the float. We'll get into GetPseudoAdjustedChild, discover that firstChild (the placeholder) is not aChildFrame, and bail out. That seems wrong. >+ else if (parentPseudo == nsCSSAnonBoxes::table) { >+ if (nsLayoutAtoms::tableColGroupFrame == aChildFrame->GetType()) { >+ if (firstChild) { >+ return aChildFrame; // There is still a row group frame. How can we hit this case? Wouldn't we have bailed earlier because firstChild (which is a row group in this case) would not equal aChildFrame (which is a colgroup)? Maybe just assert !firstChild here (and make this whole if body #ifdef DEBUG)? >@@ -9887,41 +9969,50 @@ >+ nsIFrame* pseudoAdjustedChildFrame = childFrame; >+ if (childFrame) { So I've been wondering... Instead of adding the several calls to GetPseudoAdjustedChild(), wouldn't it make more sense to do: childFrame = mPresShell->GetPrimaryFrameFor(aChild); if (!childFrame) { frameManager->ClearUndisplayedContentIn(aChild, aContainer); return NS_OK; } childFrame = ::GetPseudoAdjustedChild(childFrame->GetParent(), childFrame); NS_ASSERTION(childFrame, "Shouldn't get null"); in the two places where we currently get childFrame (here, and after we remove letter frames), and then leave all the other code in this method as-is? It seems like that would do closest to what we want (call DeletingFrameSubtree on the right frames, do IsFrameSpecial checks on the right frames, etc). It would also be a lot easier to check correctness, I think... Is there some reason we can't do this? >+ // then remove them too. In that case, avoid doing RecoverLetterFrames() >+ // below if |containingBlock| is one the pseudos. And this part could be nixed, I think, if we just used the pseudo-adjusted frame as childFrame... If this code does stay, "one of the".

Comment on attachment 228591 [details] [diff] [review] Patch rev. 6 (diff -w) r- per comment 37.

What exactly happened to this patch? It's been sitting around for over a year.

>What exactly happened to this patch? The patch was r-'ed and nobody did work on this. If you have spare cycles please go ahead, I can assist with comments and bz probably is also happy to get this moved but its only a matter of work (testing is the interesting part).

The code in bug 373610 might help with testing a patch for this bug.

I am taking this, if I can't get it done I will give it back.

we show a magenta and a orange square with this patch

that shows that attachment 363542 [details] [diff] [review] does not remove the pseudo when a -moz-popup was wrapped. Boris what else can one stuff into the pseudos and then remove it?

The -moz-popup failure is caused by the code in nsCSSFrameConstructor::ConstructFrameInternal where we first create a pseudo frame based on the display type inside AdjustParentFrame and later do not create a frame for it. if (data) { #ifdef MOZ_XUL if ((data->mBits & FCDATA_IS_POPUP) && adjParentFrame->GetType() != nsGkAtoms::menuFrame && !aState.mPopupItems.containingBlock) { return NS_OK; } #endif /* MOZ_XUL */ The same might be triggered by the svg check above that code. Later we go through the cleanup of undisplayed content and since there is no frame to remove there will be no pseudo removal.

Yeah, now that we get the FrameConstructionData in all cases before we call AdjustParentFrame (we used to do the by-display datas after when I did the initial FrameConstructionData landing) we should hoist the popup and SVG checks to before AdjustParentFrame, I think. File a separate bug for that (blocking this one) and either assign to me or take yourself as you prefer? It should even be possible to write reftests for it against trunk, and certainly against trunk with this patch, as you demonstrated.

OK, there's no issue with SVG here. For the popup issue I filed bug 480017.

Comment on attachment 364716 [details] [diff] [review] patch >+++ b/layout/base/nsCSSFrameConstructor.cpp >+GetPseudoAdjustedChild(nsIFrame* aParentFrame, >+ nsIFrame* aChildFrame) Fix the indent. >+ // if the childFrame gets removed all the pseudo frames that are between >+ // childframe and the real parent can be removed >+ // so we seek upwards a frame which is either not a table pseudo frame "So we look up until we get to a frame which is either ..." >+ // or is table pseudo frame which has more valid children than the one needed "or is a table pseudo ..." >+ return aChildFrame; // The child has a sibling or the parent has definetely "definitely" >+ // XXXbernd should check all childlists of the block frame if there any other >+ // children than the childframe or is the entrance test for only one children >+ // on the main childlist enough?? The main childlist check is enough, since all other childlists involve having an in-flow child (placeholder). In fact, checking other childlists would be wrong, since we could have lots of floats on the float list whose placeholders are all descendants of the same in-flow inline child, say. >+ if (parentPseudo == nsCSSAnonBoxes::table && result == aParentFrame) { >+ // The inner table should not be removed unless the outer is too. >+ result = aChildFrame; >+ } >+ return result; Weird indent on the '}' there. >@@ -8458,17 +8560,21 @@ nsCSSFrameConstructor::ContentRemoved(ns >+ return NS_OK; >+ } >+ childFrame = ::GetPseudoAdjustedChild(childFrame->GetParent(), >+ childFrame); >+ NS_ASSERTION(childFrame, "Shouldn't get null"); Weird indent. >@@ -8595,19 +8704,29 @@ nsCSSFrameConstructor::ContentRemoved(ns >+ nsIFrame* adjustedPlaceholder = >+ ::GetPseudoAdjustedChild(placeholderParent, placeholderFrame); Hmm.... Is this needed? If so, why? How do we handle this testcase: <div style="display: table-row"> <span>abc<span style="float: left"></span></span> </div> and then removing the floating span? It seems like GetPseudoAdjustedChild() on the floating block will return the cell in this case, so we'll remove that... and then the text will disappear. Can you double-check? In general, it seems like we should not be calling GetPseudoAdjustedChild() on out-of-flow frames... >+ PRBool removePseudo = (adjustedPlaceholder != placeholderFrame); > ::DeletingFrameSubtree(frameManager, placeholderFrame); > rv |= frameManager->RemoveFrame(placeholderParent, > nsnull, placeholderFrame); >+ if (removePseudo) { >+ placeholderParent = adjustedPlaceholder->GetParent(); >+ ::DeletingFrameSubtree(frameManager, adjustedPlaceholder); >+ rv |= frameManager->RemoveFrame(placeholderParent, >+ nsnull, adjustedPlaceholder); >+ } This part doesn't make much sense to me. >+++ b/layout/reftests/bugs/reftest.list >+== 280708-1a.html 280708-1-ref.html # bug 473824 Don't need the comment any more, do you?

I was thinking some more. It seems to me that it's never correct to pass an out-of-flow to GetPseudoAdjustedChild. So the right control flow should be: 1) Get primary frame. 2) If it's out of flow, get its placeholder. 3) Pass the placeholder, or the primary frame if it's in-flow to GetPseudoAdjustedChild. 4) If GetPseudoAdjustedChild is different from the frame passed to it, use its result as the child frame. (the check in item 4 is needed to keep from using the placeholder instead of the primary frame when no adjustment is needed). Alternately, GetPseudoAdjustedChild could get the placeholder and use it for its various checks. Either way, I guess...

And thinking about it even more, we have another problem with removals that this patch doesn't address. Consider this testcase: <!DOCTYPE html> <html> <script> window.onload = function() { var n = document.getElementById("n"); n.parentNode.removeChild(n); } </script> <body> a<span style="display: table-cell"></span> <span style="display: table-cell" id="n"></span>b </body> </html> When loaded, this should presumably look like: a b whereas with this patch it'll look like "ab". This didn't use to be a problem back when we shipped whitespace out to before the table, but it really is an issue. So maybe what we should do is reframe the parent content any time a frame is removed which has a pseudo parent and is its first or last primary child list child (with some extra hackery for captions). That would remove a lot of the complexity in the patch, since that's aimed at removing as little as possible and ends up not reframing enough, imo. Thoughts?

If we do take an approach more similar to that, then the right place to do it is probably in MaybeRecreateContainerForIBSplitterFrame (which we should rename accordingly).

I've spun off the first-letter issue that Mats rolled into his original patch into bug 484400.

Bernd suggested that we should switch the patch and review roles, so here goes. This patch just switches to what I propose in comment 54. It includes bernd's reftest, reenables the one existing reftest that was marked "fails" due to this bug, and adds some tests exercising each clause of IsTablePseudo (with the exception of the "tableCell" clause, which can't be tested using kids; I need that for my patch for bug 148810) as well as the clauses for captions and colgroups in MaybeRecreateContainerForFrameRemoval.

Oh, and I plan to add more tests based on the testcases of the various bugs this bug blocks; I just figured I'd start the review rolling first.

Comment on attachment 368566 [details] [diff] [review] Proposed patch per comment 54 This is much more elegant than what was attempted before. And OMG this looks so damned easy we could have done this 7 years ago. +// Return whether the given frame is a table pseudo-frame Please expand the comment and explain that table cell content and outer table anon boxes are always created and only in a few cases are relally pseudo related. Also the special case for a single caption where we create outer and a empty inner frame should be mentioned. The code will handle this correctly but it is far from obvious. // table pseudo-frames, recreate the relevant frme subtree s/frme/frame/ I would move the testcase that I wrote to the same directory as the other tests the directory was not present when I worked on this. Although that's a proposal not a instruction.

Expanded that comment. Fixed typo. Moved tests into the table-anonymous-boxes directory, and put tests for various bugs this blocks in there too. We couldn't have done this 7 years ago, because the function I'm changing got added a lot later than that (by roc) to fix {ib} issues similar to this. ;)

OK, pushed <http://hg.mozilla.org/mozilla-central/rev/d0d980778f01>. Seems to have stuck with no test failures.

Is this something we want to take on 1.9.1 as well? Setting wanted-? to get the driver's opinion.

Hmm. This might be reasonably safe, actually. It doesn't seem to depend on the other parts of frame construction that I changed. Some of the tests might, of course. The fix for bug 148810 does, on the other hand. David, do you think this is enough worth it, without the more-correct handling of insertion from bug 148810, that it's worth taking this patch on branch? I think it might be, given how long it'll be at this point until we ship gecko > 1.9.1.x

I suspect there are a bunch of cases that it would help, e.g., toggling something between 'none' and some other (incorrect?) value of 'display', so I think I'd be in favor of taking it, although I wouldn't call it a blocker.

I included some tests that landed before this bug that we're mostly passing on branch; I had to only mark one of those failing. I did have to mark a bunch of the tests I added for this bug (though none from the dependent bugs) failing too, because we screw up ContentInserted if the parent is a pseudo frame and we did the weird "ship whitespace out to before it" thing. This patch does fix all cases of people toggling between none and some weird display type in actual tables.

Comment on attachment 376969 [details] [diff] [review] Branch merge r=dbaron

Comment on attachment 376969 [details] [diff] [review] Branch merge It might be worth taking this on branch. It fixes a number of web compat issues, and we expect more to appear as more sites start using this stuff because IE8 supports it. There are no known regressions from this fix, and conceptually it's reasonably safe...

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b4f9568dd47b