Skip to content

Conversation

RobRendell
Copy link
Contributor

@RobRendell RobRendell commented Apr 9, 2024

Fixes #29

Hi! Me again! It's been a while :)

With the latest versions of react-new-window and react-reverse-portal (react-new-window 1.0.1 and react-reverse-portal 2.1.1), I'm now getting the problem with exceptions being thrown because a DIV element is not an instanceof HTMLElement in Chrome, Firefox and Edge.

It turns out that the HTMLElement and SVGElement objects are not actually globals - they're values on the window object. Each window object has its own instances of those objects, which is why instanceof is returning false for cross-window tests. You apparently get the same behaviour if you test an element in an iframe.

So, to get it working, I've made this PR which gets the parent window of the domElement (using the technique described in this stackoverflow answer), and then tests if domElement instanceof window.HTMLElement or domElement instanceof window.SVGElement. This works for me in the above three browsers when I tested rendering the reverse portal in a new window.

The code needs to cast things to any unfortunately, because Typescript doesn't know about the ancient parentWindow field that's needed if you want to support IE8. Even if you very reasonably don't care about that, Typescript also doesn't believe you can use Window.HTMLElement and Window.SVGElement in instanceof tests (they're interfaces as far as it's concerned).

Anyway, hopefully this change looks good to you. Let me know if you have any questions!

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RobRendell! This looks good, definitely happy to support that use case, just a couple of points on the details.

src/index.tsx Outdated
const document = domElement.ownerDocument as any;
// Cast document to `any` because Typescript doesn't know about the legacy `Document.parentWindow` field, and also
// doesn't believe `Window.HTMLElement`/`Window.SVGElement` can be used in instanceof tests.
const window = document.defaultView ?? document.parentWindow; // Fallback on `parentWindow` in order to support IE8 and earlier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to use different names, rather than shadowing the document and window globals, to avoid confusion later on. Maybe ownerDocument/ownerWindow, or parent or similar?

Also it looks like ownerDocument and defaultView can be null too, in some niche circumstances. Not totally clear if these are reachable in sane usage, and there's not much we could do about it, but it would be useful to check for that and throw a clear error (something like Can't validate element, ownerDocument not available is fine) just in case it is possible. That way this is easier to find & debug, rather than being a confusing 'wrong element type' validation message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about shadowing the real globals - I'll rename them.

Also, good catch about the potential null-ness. I reckon in the case they're null, we could just fall back on the global document/window - what do you think? That would revert to the current behaviour, of always testing instanceof against the global window.HTMLElement and window.SVGElement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that makes sense! Thanks 👍

@pimterry pimterry merged commit 2aa574e into httptoolkit:master Apr 11, 2024
@pimterry
Copy link
Member

Nice work, thanks @RobRendell! Great to get this fixed. I'll release as v2.1.2 in just a second.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rendering portal in a new window causes error: "html" portalNodes must be used with html elements, but OutPortal is within DIV
3 participants