solidjs / solid-docs-legacy

OLD documentation for SolidJS and related packages, replaced by https://github.com/solidjs/solid-docs-next
https://www.solidjs.com/
MIT License
188 stars 152 forks source link

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

Open ITenthusiasm opened 10 months ago

ITenthusiasm commented 10 months ago

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

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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. :thinking: Though it might be unexpected behavior for devs coming from Svelte. (Not necessarily a bad thing.)

ryansolid commented 10 months ago

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.