Bug 149546 – Node.replaceChild() does not behave according to the specification

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Node.replaceChild() does not behave according to the specification. In particular, when replacing |child| with |node| we are supposed to remove |node| from its parent *before* removing |child| from its parent:
https://dom.spec.whatwg.org/#concept-node-replace

This leads to incorrect Mutation Records being queued in some cases.

[Chris Dumez]

<rdar://problem/22571887>

[Chris Dumez]

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

[Chris Dumez]

Created attachment 261902 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 261902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261902&action=review

r=me provided you add an iframe test as described below.

> Source/WebCore/ChangeLog:15
> +        This patch reverses the order as per the specification. Our new behavior
> +        matches Firefox's behavior.

You should probably mention that this removes the minor optimization.
Also, we should add a test ensuring that onbeforeload event on iframe is called in the expected order.
i.e. if you're replacing an iframe with another iframe, you should get beforeunload on the newChild before on the refChild.

> Source/WebCore/dom/ContainerNode.cpp:430
> +    Ref<Node> removedChild(oldChild);
> +    removeChild(oldChild, ec);

Can we rename the variable to something like childToBeRemoved since removeChild and removedChild only differs by one character "d"?

[Ryosuke Niwa]

I actually take it back. I can't create a reliable test for this :(

[Chris Dumez]

Created attachment 261911 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 261911 [details]
Patch

Clearing flags on attachment: 261911

Committed r190233: <http://trac.webkit.org/changeset/190233>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.