-
Notifications
You must be signed in to change notification settings - Fork 286
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
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
krichprollsch
left a comment
There was a problem hiding this 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
|
Oh yeah that's better. Just refactored it to use the walker instead. thanks |
krichprollsch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change.
Here is a little non-blocking suggestion.
It would be also great to:
- add a comment before both
ifstatements to explain the intention of changing the owner - write a JS test case at the end of the file, similar to https://github.com/lightpanda-io/browser/blob/main/src/browser/polyfill/webcomponents.zig#L39-L65 You could use
Browser.DOM.node.owneras test name. You can run it specifically withT=Browser.DOM.node.owner make test
src/browser/dom/node.zig
Outdated
| child.owner = self_owner; | ||
| var current = child; | ||
| while (try w.get_next(child, current)) |current_node| { | ||
| current_node.owner = self_owner; | ||
| current = current_node; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about:
| child.owner = self_owner; | |
| var current = child; | |
| while (try w.get_next(child, current)) |current_node| { | |
| current_node.owner = self_owner; | |
| current = current_node; | |
| } | |
| var current = child; | |
| while (true) { | |
| current.owner = self_owner; | |
| current = try w.get_next(child, next) orelse break; | |
| } |
|
Thanks for the suggestion. I’ve added the tests and simplified the walker as you recommended. Sorry for the trouble, I'm still getting the hang of Zig |
|
No worries, thank you very much for your contribution 🙏 |
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:
Javascript used: