I uploaded a testcase of pure XML, styled with CSS. The selectors are dependent on attributes, so I can switch between two rulesets. One is consisting of a float (the "-") and the blocks. Clicking the "-" triggers XBL to set the "collapsed" attribute, which triggers the float and the first and the third div to get display inline. (And change the text node to "+".) Clicking again removes the attribute, getting back to the old CSS rules. But I seem to get dangling frames, each time I cycle thru this. Not one, for each cycle one.

A simpler testcase would be nice.

Yeah... if it's possible to do this without xbl (eg using XHTML elements and onlick), that would be great. Also, do you need all of the CSS rules in that sheet?

"onlick"? that sounds pretty perverted... Do we really have a lick-event? :)

Yeah, it's basically the same impl as onscratch-n-sniff.

I teared down the testcase and while doing so, I realized that everything works fine as long as the float is not anonymous content. So the testcase has two chunks now, one that is not working with XBL and one that is not working, with explicit content. I added buttons that do the attribute setting and removing for each of the two. The CSS is now just inside a xhtml:style.

Where do frames for insertion points get constructed? Are we ending up with an inline insertion point frame and sticking a block in it, leading to a nasty IB situation?

This fixes it. nsCSSFrameConstructor::FindPreviousSibling uses the index with a ChildIterator, so nsCSSFrameConstructor::RecreateFramesForContent should use one when getting the index, otherwise we don't take into account the anonymous content. Note that there's other callers to nsIContent::IndexOf in nsCSSFrameConstructor, maybe they should be converted to use a ChildIterator too? The parent change looks like it doesn't have any effect, but it seems to be what was intended.

Peter, is this also fixed by the patch in bug 289377? It looks to me like it should be... I really don't like making the meaning of "aIndexInContainer" vague like this patch does (it means different things in different cases with this patch).

(In reply to comment #10) > Peter, is this also fixed by the patch in bug 289377? It looks to me like it > should be... It isn't. > I really don't like making the meaning of "aIndexInContainer" vague like this > patch does (it means different things in different cases with this patch). I'm fine with looking for a different way fix it, but we *will* have to deal with the fact that nsCSSFrameConstructor::FindPreviousSibling uses aIndexInContainer with an iterator (which does take into account the anonymous content), while nsCSSFrameConstructor::FindPreviousSibling does not.

Wait. Back up. Which FindPreviousSibling call are we concerned with here? The one with the XXX comment before it at line 9014 (in ContentInserted)? And the problem is that since aIndexInContainer is the index in the non-anonymous child list this code completely fails? If that's the case, we should fix ContentInserted, not just fix up RecreateFramesForContent to work around ContentInserted bugs...

Comment on attachment 181703 [details] [diff] [review] v1 Ok, so what should aIndexInContainer be in calls to ContentRemoved/ContentInserted? ContentAppended for example uses an iterator to get the index that it passes to ContentInserted, while RecreateFramesForContent uses nsIContent::IndexOf. GetAdjustedParentFrame (called by ContentInserted) uses nsIContent::ChildAt with that index, while FindPreviousSibling (also called by ContentInserted) uses an iterator.

> Ok, so what should aIndexInContainer be Given that these are basically just the nsIDocumentObserver notifications, it should be whatever nsIDocumentObserver promises. Then within these methods we should use child iterators as appropriate, _after_ resolving the insertion point (which last should also update the index appropriately, if this stuff worked right). The call from ContentAppended to ContentInserted with the mixed-up index sounds like it's trying to cover up the 50% case and is likely the primary reason ContentInserted hasn't been fixed before now. :(

Fixed by checkin for bug 307394. Checked in test.