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.

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!

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

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

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.

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.

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?

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] 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.

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.

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]

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.

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

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] 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.

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

Depends on D161548

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

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

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.

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

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