Skip to content

Fix: Properly handle node ownership when using appendChild and insertBefore #920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SrikanthKumarC
Copy link

Fixes a bug where using appendChild or insertBefore with a node from a different ownerDocument would throw a WrongDocument Error. According to the specification, the new node being added should be implicitly adopted by the parent's ownerDocument.

This change updates these methods to first set the ownerDocument of the node being added, and recursively update the ownerDocument of its children to match that of the parent.


Currently, the behavior is as follows: an error is thrown when a node is inserted using appendChild or insertBefore if its ownerDocument differs from that of the parent. This PR fixes that.

Debug log:

js : function call error . . . . . . . . . . . . . . .  �[0m[+0ms]
      name = browser.dom.node.Node._insertBefore
      err = WrongDocument
      args = 
        1: #<HTMLDivElement> (object)
        2: #<HTMLParagraphElement> (object)
      stack = 
        <anonymous>:28

Javascript used:

console.log("--- Starting Automatic Cross-Document Insertion ---");


const parser = new DOMParser();
const newDoc = parser.parseFromString('<div id="new-node"><p>Hey</p></div>', 'text/html');
console.log("A new document has been created in memory using DOMParser:", newDoc);


const newNode = newDoc.getElementById('new-node');

console.log("The new node's initial ownerDocument is:", newNode.ownerDocument);
console.log("The main document is:", document);


const parent = document.getElementById('target-container');
const referenceNode = document.getElementById('reference-node');


parent.insertBefore(newNode, referenceNode);

console.log("\n--- Insertion Complete ---");
console.log("The new node has been inserted into the main document.");
console.log("The new node's updated ownerDocument is:", newNode.ownerDocument);
console.log("Is the new node now a child of the main document's parent?", parent.contains(newNode));

const k = document.getElementById('new-node');
const ptag = k.querySelector('p');
console.log("The new node's p tag owner is: ", ptag.ownerDocument);
console.log("P tags owner is same as main document?", ptag?.ownerDocument === document);

const statusMessage = document.getElementById('status-message');
statusMessage.textContent = "Success! The node was inserted automatically on load. Check the console for details.";

Copy link

github-actions bot commented Aug 3, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@SrikanthKumarC
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@krichprollsch krichprollsch self-requested a review August 4, 2025 07:46
Copy link
Member

@krichprollsch krichprollsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @SrikanthKumarC

Thank you very much for you PR 🙏

I would like to avoid the usage of an arena for traversing children.
Can you consider using the walker instead?

const w = Walker{};
while (true) {
    next = try w.get_next(child, next) orelse break;
    next.owner = self_owner;
}

existing example: https://github.com/lightpanda-io/browser/blob/main/src/browser/dom/node.zig#L224-L234

@SrikanthKumarC
Copy link
Author

Oh yeah that's better. Just refactored it to use the walker instead. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants