Skip to content
This repository has been archived by the owner on Sep 17, 2024. It is now read-only.

[Bug]: use:* Directives Use Elements Whose ownerDocument Differs from window.document #278

Open
ITenthusiasm opened this issue Oct 27, 2023 · 7 comments
Labels
documentation Improvements or additions to documentation

Comments

@ITenthusiasm
Copy link

ITenthusiasm commented Oct 27, 2023

Describe the bug

The element passed to a use:* directive has an ownerDocument that differs from the application's Document (i.e., window.document) even though the element belongs to the application's Document. This prevents directives like the following from working:

function listenersDirective(element: HTMLElement) {
  element.ownerDocument.addEventListener("click", handleClick);
  // ...

  onCleanup(() => {
    element.ownerDocument.removeEventListener("click", handleClick);
    // ...
  });
}

This discrepancy is problematic in certain types of Applications and NPM Packages related to the Web. It's not immediately clear what exactly element.ownerDocument is pointing to. It points to a Document, but it definitely doesn't point to window.document.

Your Example Website or App

https://playground.solidjs.com/anonymous/5ddfcd3c-103f-4283-aff6-6720325bfbd5

Steps to Reproduce the Bug or Issue

  1. Create any valid Solid Directive
  2. Compare the element.ownerDocument in the Solid Directive with the global window.document and find that the two are not referentially equivalent

The Solid.js Playground Link provided above exemplifies this problem.

Expected behavior

The HTMLElement passed to any given Solid Directive on the Web should have an ownerDocument that's [referentially] equivalent to the application's Document (i.e., window.document).

Platform

  • OS: MacOS
  • Browser: Chrome
  • Version: 118.0

Additional context

This bug is not observable after the application has finished rendering. If you open the devtools and select the element that was given a directive, then element.ownerDocument === window.document will yield true in the console even though it yielded false during the call to the Solid Directive.

@ITenthusiasm ITenthusiasm changed the title use:* Directives Use Elements Whose ownerDocument Differs from window.document [Bug]: use:* Directives Use Elements Whose ownerDocument Differs from window.document Oct 27, 2023
@ITenthusiasm
Copy link
Author

Since the question was raised on Discord, it's also worth pointing out that elements created with document.createElement that have not yet been attached to the DOM still have an ownerDocument which points to the application's Document.

const div = document.createElement("div");
console.assert(div.ownerDocument === document); // Succeeds

Svelte's Actions are also able to recognize the correct ownerDocument of an element.

@ITenthusiasm
Copy link
Author

Sidenote: Note sure if solidjs/solid#1740 is related since I didn't investigate the code. But if it touches ownerDocuments, then this bug is probably worth keeping in mind.

@ryansolid
Copy link
Member

ryansolid commented Oct 30, 2023

I think this comes down to how elements are created as Solid isn't doing anything directly to mess with this. cloneNode vs createElement and when owner is applied. Until cloned nodes are attached then they don't have an owner I'm gathering. We do actions and refs at this time so that they can be passed through. So technically they don't have an owning document at this point. Now it is possible that document.importNode would straight up solve this as well (although it is slower).

Until we can make a decision on importNode vs cloneNode I recommend attaching stuff like this in onMount as it is unlikely we do anything else to try to mitigate this.
https://playground.solidjs.com/anonymous/caad4663-af2b-41b8-888e-0035d8f536f3

ITenthusiasm referenced this issue in enthusiastic-js/form-observer Oct 30, 2023
@ITenthusiasm
Copy link
Author

From some quick testing in the browser, it seems to me that nodes created with cloneNode still have the proper ownerDocument. Does it depend on which specific node was cloned?


In any case, using onMount seems to work fine for my use case. I'm really thankful that your framework's primitives are usable just about anywhere (compared to something like React.useEffect). It makes life a lot easier in situations like these.

If this doesn't get fixed/changed (depending on if it's seen as a bug), it might be worth pointing this out in the documentation. Not sure if it's too obscure/specific to mention, though. 🤷🏾‍♂️

@ryansolid
Copy link
Member

Yeah I think it is because the elements are coming from template elements. I don't know what else would do it. We really aren't doing anything special.

@ITenthusiasm
Copy link
Author

ITenthusiasm commented Oct 31, 2023

Yeah that seems to check out.

In the browser:

let template = document.createElement("template");
template.innerHTML = "<span>Hello</span>";
console.log(template.content.children[0].ownerDocument === document); // `false`

I guess this isn't necessarily a bug in that case. 🤔 Though it might be unexpected behavior for devs coming from Svelte. (Not necessarily a bad thing.)

@ryansolid
Copy link
Member

ryansolid commented Nov 1, 2023

Thanks for the update.

Those Svelte Devs may be in for a surprise with Svelte 5 depending on timing of when they fire actions. They might fire them after connected. But Svelte 5 basically adopts Solid's render model.

In terms of how to manage this. We might use importNode in the future but I guess for now this is a documentation issue. We generally say refs and directives fire before nodes are connected to the DOM, but that can have other side effects it turns out.

@ryansolid ryansolid added the documentation Improvements or additions to documentation label Nov 2, 2023
@ryansolid ryansolid transferred this issue from solidjs/solid Nov 3, 2023
ITenthusiasm referenced this issue in enthusiastic-js/form-observer Nov 5, 2023
The test actually _was not_ failing due to a
bug in Vitest or Solid Testing Library. This
test was actually (rightly) failing because of
solidjs/solid-docs#278. This bug has been fixed
by fdf61ed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants