1799354 - MutationRecord missing for Element.replaceChildren

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[ozixtheorange]

Attachment: bug.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:106.0) Gecko/20100101 Firefox/106.0

Steps to reproduce:

See the following test case: https://jsfiddle.net/azmisov/96fp4gsj/, or the attached file

Actual results:

Calling Element.replaceChildren(child) does not include a "removedNodes" MutationRecord for child.

Expected results:

A MutationRecord should have been given for the removal of child.

Comment 1

[Peter Magyari (Desktop QA)]

Reproducible across all current Firefox versions.

I'm setting a component in order to get the development team to take a look over this issue.
If this isn't the best component for it, feel free to set it to a more suitable one.

Thank you for reporting!

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/dom/base/nsINode.cpp#2146 that 'false' looks suspicious.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

:WeirdAl, since you are the author of the regressor, bug 1626015, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 4

[Alex Vincent [:WeirdAl]]

It's been a very long while since I implemented ParentNode.replaceChildren(), but based on reading over the original Phabricator request I submitted, I think this is not actually a regression, just a bug in the original implementation.

:smaug suggests, correctly I think, the existing tests in ParentNode-replaceChildren.html didn't cover this specific case. There was only one MutationObserver test in this file to begin with.

Having changed nsAutoMutationBatch mb(this, true, true); locally in nsINode::ReplaceChildren, I don't think that's the correct fix - or at least, not the complete one. It failed the WPT test with assert_equals: mutations.length expected 1 but got 2. Now, this assumes the original test was correct - which is a dangerous assumption. I didn't really change the existing test except to add descriptive text.

With the change, and the reporter's test adjusted to show:
console.log(```parent: ${r.target.id} added: ${Array.from(r.addedNodes).map(n => n.nodeName)}; removed: ${Array.from(r.removedNodes).map(n => n.nodeName)}```);

the reporter's test reports in the console:

"parent: a added: ; removed: #text"
"parent: b added: #text; removed: "

Without the change, the test reports in the console:

"parent: b added: #text; removed: "

So in summary:

• the WPT shows two nodes removed, two nodes added, in one mutation record with one parent (though I would've liked the test to show which nodes)
• this test shows one mutation record when it clearly should show two, each with different parents.
• I don't know enough about the nsAutoMutationBatch class to be sure of the right fix.

Comment 5

[Alex Vincent [:WeirdAl]]

I can envision another combination to test, where the arguments to parent.replaceChildren come from different parent nodes. That would be an interesting stress test for the mutation observer.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Aha, https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/dom/base/nsINode.cpp#1878,1880-1881 makes this behave interestingly. The same code is effectively in the spec too.
So, either the MutationRecord for the removal should happen because of ConvertNodesOrStringsIntoNode call (in case there are multiple nodes) or it will happen as part of step 7.1 in https://dom.spec.whatwg.org/#concept-node-insert

So, do we need nsAutoMutationBatch at all?

Comment 7

[Alex Vincent [:WeirdAl]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #6)

So, do we need nsAutoMutationBatch at all?

Without it, we get three MutationRecords on the WPT, which for a single call to .replaceChildren() seems completely wrong. Two of those are for individual node removals, and the third is for the pair of node insertions. This also would violate the specification stating "Remove all parent’s children, in tree order, with the suppress observers flag set."

I've tried several different combinations of interactions with the mutation batch, and none of them pass both the reporter's test and the existing WPT.

I question, now that I think about it, whether this might be a bug in the DOM specification from WHATWG. Consider: the spec currently requires in the last step a single mutation record with all the nodes removed and added in a single mutation record. If we call .replaceChildren(...node.children), the mutation observer doesn't tell the user if a child node was first added and then removed, or removed and then added. From the observer's perspective, it's a single operation. It might be safer if it were two mutation records.

Or we might need to design a more detailed, less code-reuse implementation for the observer implementation, which would be over my head.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

In https://dom.spec.whatwg.org/#concept-node-replace-all step 6 calls insert, which in 7.1 will adopt. So that creates a record for the removal from the old parent.
And if there are multiple nodes to be replacing the old stuff, it is already https://dom.spec.whatwg.org/#converting-nodes-into-a-node which creates the record.
Then there should be a separate record which includes all the removed nodes (removed from the target of .replaceChildren) and all the added nodes.

So, there should be two records, per spec, if the new nodes are initially bound to some parent node.

Comment 9

[ozixtheorange]

I would expect there to be two MutationRecords as well. Perhaps I didn't make that super clear with the test case.

MutationRecord 1: target = a, removedNodes = [text]
MutationRecord 2: target = b, addedNodes = [text]

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Problem is that when doing nested MutationBatch with nsAutoMutationBatch mb(this, true, true);
https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/dom/base/nsINode.cpp#2735,2772,2780
ends up creating extra record with the wpt test.

In the spec it doesn't happen because of the step 8 in insert. And the testcase in this bug works per spec because of insert 7.1.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1799354 - MutationRecord missing for Element.replaceChildren, r=peterv

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

That wip patch is just an idea. Too late here to think whether it breaks something :)

Comment 13

[Alex Vincent [:WeirdAl]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #12)

That wip patch is just an idea. Too late here to think whether it breaks something :)

Both tests - the original WPT test and the new one provided by :ozixtheorange - pass with this patch.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

While testing this, tryserver revealed also DOMNodeRemoved events are missing with replaceChildren. Oops :)
Fixing.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1799354, replaceChildren should fire DOMNodeRemoved when needed (and not assert about 'Want to fire DOMNodeRemoved event, but it's not safe'), r=peterv

Depends on D161548

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1626015

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a99b11ed2a9
MutationRecord missing for Element.replaceChildren, r=peterv
https://hg.mozilla.org/integration/autoland/rev/e8ab5334fddd
replaceChildren should fire DOMNodeRemoved when needed (and not assert about 'Want to fire DOMNodeRemoved event, but it's not safe'), r=peterv

Comment 18

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36993 for changes under testing/web-platform/tests

Comment 19

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/8a99b11ed2a9
https://hg.mozilla.org/mozilla-central/rev/e8ab5334fddd

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:smaug, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox108 to wontfix.

For more information, please visit auto_nag documentation.

Comment 21

[Peter Magyari (Desktop QA)]

I've verified the fix using Nightly 109.0a1 (20221117093901) on MacOS 11 and Windows 10.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I don't think this is needed for beta. We've had this behavior for a long time.

Comment 23

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 24

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot