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 doesn't track lost child #456

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

Document doesn't track lost child #456

edemaine opened this issue Nov 1, 2022 · 6 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, documents do not seem to realize that they've lost their (only) child when they get reparented.

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

const doc = new DOMParser().parseFromString('<svg/>', 'image/svg+xml');
const oldRoot = doc.firstChild;
const newRoot = doc.createElement('svg');
newRoot.appendChild(oldRoot);
console.log('After first move, document has', doc.childNodes.length, 'children (should be 0)');
doc.appendChild(newRoot);
console.log('After second move, document has', doc.childNodes.length, 'children (should be 1)');

Expected behavior
newRoot.appendChild(oldRoot) should change the oldRoot's parent to be the newRoot, which should mean that the document loses its child. Running the above code in Chrome's console confirms this behavior. Instead I get:

xmldom 0.8.4 instead says that the document still has 1 child after the first move, so the second move fails according to the new one-child-per-document rule. (Error: Hierarchy request error: Only one element can be added and only after doctype)

xmldom 0.8.3 somehow says 1 child after both moves, which is why I didn't see this issue before, but it's still wrong (should be 0 after the first move).

Runtime & Version:
xmldom version: 0.8.4
runtime version: Node v18.7.0
other related software and version: WIndows 11

@edemaine edemaine added bug Something isn't working needs investigation Information is missing and needs to be researched labels Nov 1, 2022
@karfau
Copy link
Member

karfau commented Nov 2, 2022

Thank you for the detailed report.

There was some error in the code sample you added, so I first had some trouble understanding it. But when I checked the stackblitz, I was able to understand it.
That is why I updated the code sample to match the stackblitz code.

There are two issues that are potentially related: #135 #145
I will check that.

As far as I can see at first glance, as soon as move 1 works correctly, move 2 should also work as expected.

I will have to decide which of the two bugs you reported to handle first, but both are top prio now.

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 4, 2022

Something really strange happened:
after I released 0.9.0-beta.6 I updated the verison in a fork of your stackblitz for this ticket, because I had a suspicion that the fix would also fix this issue.
But in the console that was provided, the error persisted.

Today I finally had time to add your reproduction scenario as a testcase.
But everything worked, nothing was thrown.
It was really confusing.
So I saved and reloaded the stackblitz and also there the errors are gone.

So the good news it I just need to port the fix to the other versions, while the bad thing is, that could already have been done some days ago.

@karfau karfau closed this as completed Nov 4, 2022
@karfau karfau linked a pull request Nov 4, 2022 that will close this issue
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

@ewrayjohnson
Copy link

ewrayjohnson commented Nov 8, 2022

In 0.8.6 this is not completely fixed! The (Error: Hierarchy request error: Only one element can be added and only after doctype) is still generated (in my case hundreds of times) greatly affecting performance.
This code from dom.js at line 479, produces the error:

	appendChild:function(newChild){
		return this.insertBefore(newChild,null);
	}

This slightly modified code from the original above produces the error:

    const doc = new DOMParser().parseFromString('<?xml version="1.0" encoding="UTF-8" standalone="yes"?>' +
      '<Properties></Properties>');
    const oldRoot = doc.firstChild;
    const newRoot = doc.createElement('svg');
    newRoot.appendChild(oldRoot);
    console.log('After first move, document has', doc.childNodes.length, 'children (should be 0)');
    doc.appendChild(newRoot);
    console.log('After second move, document has', doc.childNodes.length, 'children (should be 1)');

@karfau
Copy link
Member

karfau commented Nov 8, 2022

@ewrayjohnson thank you for reporting, I will have a closer look today, to decide if I need to reopen this issue or we should have a separate one.
Thank you for providing the changed input.

@karfau
Copy link
Member

karfau commented Nov 8, 2022

@ewrayjohnson the firstChild that you are moving into the svg tag is it processing instruction (<?xml version="1.0" encoding="UTF-8" standalone="yes"?>), so after that there is still the Properties element in the DOM, so no new elements can be added.
This is the expected behavior.

if you want to remove the first element you should be able to use documentElement instead of firstChild:
https://stackblitz.com/edit/js-xmldom-template-k94zdq?devToolsHeight=33&file=index.js

If you want to provide a different example, which is closer to what you are trying do do, please report a new issue.

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