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.

Sign up for GitHub

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

Jump to bottom

Incorrect implementation of NamedNodeMap/Element.attributes #728

Closed
lukasbach opened this issue Feb 10, 2023 · 5 comments · Fixed by #1006
Closed

Incorrect implementation of NamedNodeMap/Element.attributes #728

lukasbach opened this issue Feb 10, 2023 · 5 comments · Fixed by #1006
Assignees
@capricorn86
Labels
bug Something isn't working

Comments

@lukasbach
Copy link

Describe the bug
There seems to be some inconsistency in how a the attributes array reference is handled by an Element instance which differs from how real browsers handle it. The following example looks like a very rare corner case, but is based on the real implementation of Microsofts Fast Framework, which to my understanding means that happy-dom is currently incompatible with microsoft fast.

TLDR: From my understanding, the issue comes down to Element.attributes being a dynamic getter which returns a new instance NamedNodeMap, and while this map has access to the original element and always returns the most recent attributes when iterating on it, doing an array-style access returns values from the state of the creation of the NamedNodeMap since they were assigned only once, which is incorrect.

To Reproduce
I've reproduced the issue with the following code in a vitest that is running in an happy-dom environment:

    const node = new Element();
    node.ownerDocument = document;
    node.setAttribute("attr1", "value1");
    node.setAttribute("attr2", "value2");
    node.setAttribute("attr3", "value3");
    const attributes = node.attributes;
    console.log(
      "Initial attributes:",
      [...attributes].map((a) => a.name)
    );
    const attr = attributes[1];
    console.log("Removing attr", attr.name);
    node.removeAttributeNode(attr);

    console.log("\n======\nReading on old reference of attributes array:\n");
    console.log(
      "Remaining attributes by iterator:",
      [...attributes].map((a) => a.name)
    );
    console.log("Number of remaining attributes:", attributes.length);
    console.log(
      "Remaining attributes by indexed access:",
      attributes[0]?.name,
      attributes[1]?.name,
      attributes[2]?.name
    );

    console.log(
      "\n======\nOperating on current attributes array (node.attributes)\n"
    );
    console.log(
      "Remaining attributes by iterator:",
      [...node.attributes].map((a) => a.name)
    );
    console.log("Number of remaining attributes:", node.attributes.length);
    console.log(
      "Remaining attributes by indexed access:",
      node.attributes[0]?.name,
      node.attributes[1]?.name,
      node.attributes[2]?.name
    );

This returns the following output:

Removing attr attr2

======
Reading on old reference of attributes array:

Remaining attributes by iterator: [ 'attr1', 'attr3' ]
Number of remaining attributes: 2
Remaining attributes by indexed access: attr1 attr2 attr3

======
Operating on current attributes array (node.attributes)

Remaining attributes by iterator: [ 'attr1', 'attr3' ]
Number of remaining attributes: 2
Remaining attributes by indexed access: attr1 attr3 undefined

Expected behavior
The reference to the attributes array should return the most up-to-date values when doing array-style value read operations on it.

More practical example
While this example looks like it can be worked-around easily, this is based on the implementation of @microsoft/fast-element: https://github.com/microsoft/fast/blob/master/packages/web-components/fast-element/src/templating/compiler.ts#L152

And since that implementation is correct and works in the browser, it is only possible to get this working by fixing the issue in happy-dom. This is how this looks at runtime:

image

This will cause attributes to be attempted to be removed twice, breaking at

happy-dom/packages/happy-dom/src/nodes/element/Element.ts

Line 929 in e91b864

if (removedAttribute !== attribute) {

Culprit

My understanding is that this is caused by this line:

happy-dom/packages/happy-dom/src/nodes/element/Element.ts

Line 256 in e91b864

return Object.assign(new NamedNodeMap(this), Object.values(this._attributes), this._attributes);

The attributes are assigned to the NamedNodeMap only once when it is created, and will become stale once the actual attributes change.

Device:
vitest 0.27.1, happy-dom 8.2.6

More Details

This could maybe be solved by using a proxy above the named node map, which would use the most recent attribute value when doing an indexed access on it rather than assigning indexed values on creation time. I could maybe look into creating a PR for this, let me know if there is interest.
@lukasbach lukasbach added the bug Something isn't working label Feb 10, 2023
@jaa134
Copy link

jaa134 commented Apr 25, 2023 •
edited

Hi @lukasbach @capricorn86! I am also running into this problem...

I'm using Microsoft's Fast design system to create a web-component library that is tested using the latest versions of Vitest and happy-dom. Registering my custom elements with the window works great, but as soon as I try to render a custom element, an exception is thrown in Element.ts at the same line mentioned above. My tests always fail with Failed to execute 'removeAttributeNode' on 'Element': The node provided is owned by another element.

By adding some logging to the compiled happy-dom code for v9.9.2, I was able to confirm that an autofocus attribute was being removed twice for a custom button element. On the second call, that attribute was not found and the error is thrown.

Let me know If I can be of any help! Here is a full stack trace:

 ❯ HTMLButtonElement.removeAttributeNode node_modules/.pnpm/happy-dom@9.9.2/node_modules/happy-dom/src/nodes/element/Element.ts:939:10
 ❯ HTMLButtonElement.removeAttributeNode node_modules/.pnpm/happy-dom@9.9.2/node_modules/happy-dom/src/nodes/html-element/HTMLElement.ts:342:9
 ❯ HTMLButtonElement.removeAttributeNode node_modules/.pnpm/happy-dom@9.9.2/node_modules/happy-dom/src/nodes/html-button-element/HTMLButtonElement.ts:264:9
 ❯ compileAttributes node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/compiler.js:95:18
 ❯ compileTemplate node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/compiler.js:153:17
 ❯ ViewTemplate.create node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/template.js:47:28
 ❯ ViewTemplate.render node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/template.js:99:27
 ❯ Controller.renderTemplate node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/components/controller.js:345:34
 ❯ Controller.finishInitialization node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/components/controller.js:309:18
 ❯ Controller.onConnectedCallback node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/components/controller.js:222:18

lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
fix: use proxy for attributes getter (capricorn86#728)
25d3742
@lukasbach lukasbach mentioned this issue Apr 25, 2023
Closed
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728:@patch: use proxy for attributes getter
354a57f
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728:@patch: use proxy for attributes getter.
df45930
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728:@patch: use proxy for attributes getter.
8ab4f29
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728:@patch: use proxy for attributes getter.
288f5ff
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728@patch: use proxy for attributes getter.
0f56706
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728@patch: Use proxy for attributes getter.
07fe02f
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728@patch: Use proxy for attributes getter.
26f5838
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728@patch: Use proxy for attributes getter.
caf5db5
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728@patch: Use proxy for attributes getter.
4fdef3f
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728@patch: Use proxy for attributes getter.
eb9bb24
lukasbach added a commit to lukasbach/happy-dom that referenced this issue Apr 25, 2023
@lukasbach
capricorn86#728@patch: Use proxy for attributes getter.
f17155f
@lukasbach
Copy link
Author

Whoops, sorry for the reference spam. Anyway, I made a contribution which would hopefully solve this issue in #876, would be cool if someone can have a look.

@capricorn86
Copy link
Owner

Thank you for contributing @lukasbach! 🌟

I haven't had time to look into this in detail, but my spontaneous thought is that we need to find a solution where we update the Element.attributes property directly and remove the Element._attributes property, if we are going to be able to find a solution which doesn't hurt performance and still updates attributes live in the NameNodeMap.

@capricorn86 capricorn86 self-assigned this Jul 19, 2023
capricorn86 added a commit that referenced this issue Aug 2, 2023
@capricorn86
#728@patch: Improves support for NamedNodeMap, which is used as the E…
f9c9ee9 
…lement.attributes property. It will now reflect any changes done to it on the Element itself.
@capricorn86 capricorn86 linked a pull request Aug 2, 2023 that will close this issue
#728@patch: Improves support for NamedNodeMap, which is used as the E… #1006
Merged
capricorn86 added a commit that referenced this issue Aug 2, 2023
@capricorn86
Merge pull request #1006 from capricorn86/task/728-incorrect-implemen…
c89516a 
…tation-of-namednodemapelementattributes

#728@patch: Improves support for NamedNodeMap, which is used as the E…
@capricorn86
Copy link
Owner

@lukasbach There is now a fix released 🙂

You can read more about the release here:
https://github.com/capricorn86/happy-dom/releases/tag/v10.5.3

@capricorn86
Copy link
Owner

Hi @lukasbach @capricorn86! I am also running into this problem...

I'm using Microsoft's Fast design system to create a web-component library that is tested using the latest versions of Vitest and happy-dom. Registering my custom elements with the window works great, but as soon as I try to render a custom element, an exception is thrown in Element.ts at the same line mentioned above. My tests always fail with Failed to execute 'removeAttributeNode' on 'Element': The node provided is owned by another element.

By adding some logging to the compiled happy-dom code for v9.9.2, I was able to confirm that an autofocus attribute was being removed twice for a custom button element. On the second call, that attribute was not found and the error is thrown.

Let me know If I can be of any help! Here is a full stack trace:

 ❯ HTMLButtonElement.removeAttributeNode node_modules/.pnpm/happy-dom@9.9.2/node_modules/happy-dom/src/nodes/element/Element.ts:939:10
 ❯ HTMLButtonElement.removeAttributeNode node_modules/.pnpm/happy-dom@9.9.2/node_modules/happy-dom/src/nodes/html-element/HTMLElement.ts:342:9
 ❯ HTMLButtonElement.removeAttributeNode node_modules/.pnpm/happy-dom@9.9.2/node_modules/happy-dom/src/nodes/html-button-element/HTMLButtonElement.ts:264:9
 ❯ compileAttributes node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/compiler.js:95:18
 ❯ compileTemplate node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/compiler.js:153:17
 ❯ ViewTemplate.create node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/template.js:47:28
 ❯ ViewTemplate.render node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/templating/template.js:99:27
 ❯ Controller.renderTemplate node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/components/controller.js:345:34
 ❯ Controller.finishInitialization node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/components/controller.js:309:18
 ❯ Controller.onConnectedCallback node_modules/.pnpm/@microsoft+fast-element@1.11.1/node_modules/@microsoft/fast-element/dist/esm/components/controller.js:222:18

@jaa134 your issue should be resolved now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@capricorn86 capricorn86

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

#728@patch: Improves support for NamedNodeMap, which is used as the E… capricorn86/happy-dom
3 participants
@lukasbach @jaa134 @capricorn86