The default bug view has changed. See this FAQ.

a.replaceChild(b, b) does not remove and re-insert the affected node

VERIFIED FIXED in mozilla7

Status

()

Core
DOM
--
trivial
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: ayg, Assigned: sicking)

Tracking

(Depends on: 1 bug)

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.)

Updated

6 years ago
OS: Linux → All
Hardware: x86 → All
Version: 2.0 Branch → Trunk
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.
Assignee: nobody → jonas
Status: NEW → ASSIGNED
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.
Attachment #542744 - Attachment is obsolete: true
Attachment #542975 - Flags: review?(bzbarsky)
Comment on attachment 542975 [details] [diff] [review]
Patch v2

r=me
Attachment #542975 - Flags: review?(bzbarsky) → review+
Checked in http://hg.mozilla.org/integration/mozilla-inbound/rev/c2f48684b9f5
Oh, and thanks for the speedy review!
http://hg.mozilla.org/mozilla-central/rev/c2f48684b9f5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Comment 12

6 years ago
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).
Status: RESOLVED → VERIFIED

Updated

5 years ago
Depends on: 725784

Updated

5 years ago
Depends on: 726440
You need to log in before you can comment on or make changes to this bug.