The current DOM Core spec says that if you call replaceChild() with both arguments the same, the argument should be removed from its parent and then re-added at the original position: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-node-replacechild This does not actually match browsers other than IE, as demonstrated by the following test case: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/904 The test case checks whether the node was actually removed by a) registering a DOMNodeRemoved handler and checking if it fires, and b) creating a Range that surrounds the node and checking whether it's collapsed afterwards. Chrome 11 dev logs nothing. Firefox 4.0 and Opera 11 log log: removed for insertBefore log: range collapsed after insertBefore log: removed for appendChild log: range collapsed after appendChild IE 9 logs log: removed for insertBefore log: range collapsed after insertBefore log: removed for replaceChild log: removed for replaceChild log: range collapsed after replaceChild log: removed for appendChild log: range collapsed after appendChild The upshot is that when asked to do a no-op insertBefore() or appendChild(), all engines except WebKit remove the node from the DOM and re-add it, as required by the spec. But for a no-op replaceChild(), Gecko (along with WebKit and Opera) seems to just leave the node alone. Since all engines but IE agree on how to behave, I considered filing a bug against the spec instead. But browser behavior seems pointlessly inconsistent -- the spec's/IE's behavior makes more sense, with all three methods behaving the same. So I'm filing a bug on Gecko instead to change to match IE. If you think the spec should change instead, WONTFIX this and I'll file a spec bug. (Needless to say, this is not likely a bug that is of much importance to users. But I'm writing DOM Range mutation tests now, and I need to know whether we're going to change the spec or not so I know what result to require in the tests.)
So our current behavior is actually consistent for insertBefore and replaceChild. Both are no-ops if the first and second arguments are equal. Note that in our implementation doing the other thing will take some serious surgery on the node insertion code to use the next/previous links instead of indices. Then again, we may want to do that anyway....
(In reply to comment #1) > So our current behavior is actually consistent for insertBefore and > replaceChild. Both are no-ops if the first and second arguments are equal. Hmm, I didn't think to test that case. Updated: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/915 Gecko, WebKit, and Opera all agree on this case too, same as with replaceChild(). IE9 seems to fire a DOM mutation event but not collapse the range (??). So I think everyone should either match IE's behavior and the current spec, or WebKit's behavior. The current spec is slightly simpler, and it rarely matters, so I'd say we should leave the spec as-is, yes?
I don't really have a strong opinion here. As a I said, the current spec is not that trivial to implement, but might not be that bad either.
I sort of agree that the spec definition is the most consistent and thus preferable. Boris, why is this hard to fix? Shouldn't just removing this test and possibly adjust how we adjust indexes do it? http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#4057
The hard part is the index-adjustment, because the reference node we're using right now will suddenly be removed from the parent.
Created attachment 542744 [details] [diff] [review] Patch to fix This should do it. We no longer need the reference node to keep track of our current insert position now that mutation events aren't mucking around with our state any more. In fact, I noticed that we're keeping track of the refContent local way more than we need to. In fact, we only need the reference node to calculate the insert position and to pass to IsAllowedAsChild. So if we change IsAllowedAsChild to take a nsINode instead, then refContent can go away completely. So technically, the only change here needed to fix the bug is to remove the |if (aRefChild == aNewChild)| early-return (and to remove the assertion that checks that we took that shortcut). The rest is cleanup.
Created attachment 542975 [details] [diff] [review] Patch v2 This includes tests and an additional fix. Turns out that for something like a.replaceChild(b, b) we fire DOMNodeRemoved twice for b, which I kind'a suspected. This isn't new (and in fact was worst before this patch since we fired DOMNodeInserted 0 times), but it caused the tests I wrote to trip up. So the patch now fixes this too.
Comment on attachment 542975 [details] [diff] [review] Patch v2 r=me
Oh, and thanks for the speedy review!
Verified fixed on: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0 Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0 Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0 When loading the test case from comment 2 you get: removed for insertBefore(self, null) range collapsed after insertBefore(self, null) removed for replaceChild range collapsed after replaceChild removed for appendChild range collapsed after appendChild removed for insertBefore(self, self) range collapsed after insertBefore(self, self).