devrelm / resize-observer

Other
98 stars 12 forks source link

Don't use window constructors in instance checks #98

Closed Joozty closed 2 years ago

Joozty commented 2 years ago

Constructors are not shared between window instances. So for example if you have an HTML element in an iframe this check will fail:

iframeDiv instanceof window.Element

Read more here

I suggest using a different strategy to check if the given parameter is an Element. For example, using tagName

MWE:

import { ResizeObserver } from "resize-observer";

document.getElementById("app").innerHTML = `
<iframe></iframe>
`;

const resizeObserver = new ResizeObserver((entries) => {
  for (let entry of entries) {
    console.log(entry);
  }
});

const iframe = document.querySelector("iframe");

resizeObserver.observe(iframe.contentDocument.body);

iframe.contentDocument.body.style.width = "300px";

Codesanbox here

devrelm commented 2 years ago

Great catch, and an interesting problem.

I would like to avoid duck-typing, so I looked up a way to get the window that an element is in using defaultView:

target instanceof target.ownerDocument.defaultView.Element

I'll have a PR open shortly.

Joozty commented 2 years ago

That won't solve it. I am afraid there is no better solution than tagName. You could store all window objects to some array and then check instanceof but it is much more complicated.

See:

const iframe = document.createElement("iframe");
document.body.append(iframe);
const divElement = iframe.contentDocument.createElement("div");

document.body.append(divElement);

console.log(divElement instanceof divElement.ownerDocument.defaultView.Element);
console.log(divElement.ownerDocument)
devrelm commented 2 years ago

Before reopening this, I want to make sure I completely understand.

You're creating an element in the context of a child iframe's document, but then appending that element to the parent document?

I didn't even know that that it was possible to create an element in one frame and use it in another. What is a use-case for doing such a thing?

In the meantime, the fix I mentioned above has been deployed in v1.0.3

Joozty commented 2 years ago

It's not really my use case but there are some applications that work like that. I work on a script that is used on many websites and from the error logs I can see that some websites mix elements between iframes. Obviously, it's not widely used but still the real ResizeObserver works with the example I provided above so I guess the polyfill should be as close as possible to it especially when the fix is quite easy.

devrelm commented 2 years ago

Fair enough. I'm out of time to work on this today, but how about instead of checking tagName we go with checking target.nodeType === Node.ELEMENT_NODE?

Joozty commented 2 years ago

that's even better

devrelm commented 2 years ago

This has been fixed in #100, published with v1.0.4.