Skip to content
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

Document.replaceChild fails #455

Closed
edemaine opened this issue Nov 1, 2022 · 7 comments · Fixed by #457
Closed

Document.replaceChild fails #455

edemaine opened this issue Nov 1, 2022 · 7 comments · Fixed by #457
Labels
bug Something isn't working needs investigation Information is missing and needs to be researched

Comments

@edemaine
Copy link

edemaine commented Nov 1, 2022

Describe the bug
Since the 0.8.4 update, it's no longer possible to replace the (only) child of a document via replaceChild.

To Reproduce
https://stackblitz.com/edit/js-xmldom-template-3uxg9i?file=index.js

const xmldom = require('@xmldom/xmldom');
const doc = new xmldom.DOMParser().parseFromString('<svg/>', 'image/svg+xml');
doc.replaceChild(doc.createElement('svg'), doc.firstChild);

Expected behavior
I expect the (only) child of the document to change, as this still respects the one-child rule for documents. Instead I get:

Uncaught Error: Not found: child not in parent
    at _insertBefore (C:\Users\edemaine\Projects\svgtiler\node_modules\@xmldom\xmldom\lib\dom.js:772:9)
    at Document.insertBefore (C:\Users\edemaine\Projects\svgtiler\node_modules\@xmldom\xmldom\lib\dom.js:908:3)
    at Document.replaceChild (C:\Users\edemaine\Projects\svgtiler\node_modules\@xmldom\xmldom\lib\dom.js:478:8) {
  code: 8
}

Runtime & Version:
xmldom version: 0.8.4 (doesn't occur on 0.8.3)
runtime version: Node v18.7.0
other related software and version: Windows 11

Additional context
A workaround is to remove the old child and then append the old child:

doc.removeChild(doc.firstChild);
doc.appendChild(doc.createElement('svg'));

I also tested the following code in Chrome's DOMParser, and it works fine.

const doc = new DOMParser().parseFromString('<svg/>', 'image/svg+xml');
doc.replaceChild(doc.createElement('svg'), doc.firstChild);
@edemaine edemaine added bug Something isn't working needs investigation Information is missing and needs to be researched labels Nov 1, 2022
@pedro-l9
Copy link

pedro-l9 commented Nov 1, 2022

I faced this same issue, upon some quick investigation it seems that it started on version 0.7.7 when there was a major change on dom.js, ever since the _insertBefore function no longer works as expected. The parent verification logic seems to be broken

if (child && child.parentNode !== parent) {

Edit: It actually wasn't the same issue but I believe that it happened for the same reason. I had this error thrown when trying to replace a child of the second node of a document.

@edemaine
Copy link
Author

edemaine commented Nov 1, 2022

I don't think it's the same reason, as my code worked in 0.8.3. I assume it has to do with the newly implemented "one child per document" rule.

@pedro-l9
Copy link

pedro-l9 commented Nov 1, 2022

Indeed I wasn't aware of such a rule, I stumbled into this error when using Samlify, which uses xmldom internally. I mapped the issue to this point of Samlify's code:
https://github.com/tngan/samlify/blob/0950407d57bb1c5a93985a1d9b1bd2fd9182d1d5/src/libsaml.ts#L681

The condition that breaks is the one I mentioned previously when verifying the node's parent and it's the only place I could find in the code where the mentioned exception could be thrown. I mentioned version 0.7.7 because it was where this condition was added and from what I could experience 0.7.6 was the only version where I could get this operation to work. I don't know what might be happening to your code that makes it work with version 0.8.3.

Are you able to check if this condition exists in the version of the library that you're using and why it doesn't break?

@karfau
Copy link
Member

karfau commented Nov 2, 2022

Thank you both for the detailed information.
Both 0.7.7 and 0.8.4 received the same change, which 0.7.6 and 0.8.3 did not have.
I will prioritize fixing this issue.

karfau added a commit that referenced this issue Nov 3, 2022
karfau added a commit that referenced this issue Nov 3, 2022
which is slightly different from the checks required when inserting a
node.

fixes #455 (only on the master branch for now, I want to test if this
also takes care of #456)

https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity
https://dom.spec.whatwg.org/#concept-node-replace
@karfau
Copy link
Member

karfau commented Nov 3, 2022

Fix for this one is available in 0.9.0-beta.6. I will continue to work on the #456 before releasing the 0.8.x and 0.7.x versions.

karfau added a commit that referenced this issue Nov 5, 2022
which is slightly different from the checks required when inserting a node.
Also fixes the missing `Document.ownerDocument`, which prevented proper deletion of childNodes from `Document`.

fixes #455 (only on the master branch for now, I want to test if this
also takes care of #456)

https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity
https://dom.spec.whatwg.org/#concept-node-replace
karfau added a commit that referenced this issue Nov 5, 2022
which is slightly different from the checks required when inserting a node.
Also fixes the missing `Document.ownerDocument`, which prevented proper deletion of childNodes from `Document`.
Also fixes properly disconnecting removed children from it's previous parent.

fixes #455 (only on the master branch for now, I want to test if this
also takes care of #456)

https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity
https://dom.spec.whatwg.org/#concept-node-replace
@karfau
Copy link
Member

karfau commented Nov 5, 2022

Versions 0.7.9 and 0.8.6 have been released with a fix for this bug

@edemaine
Copy link
Author

edemaine commented Nov 7, 2022

Thank you! I confirm that 0.8.6 fixed both of my problems. With the now proper child detection, it even helped me find a bug in my code! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs investigation Information is missing and needs to be researched
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants