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.

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

Check-canvas-instance-in-another-window #507

Open
wants to merge 3 commits into
base: beta
Choose a base branch
from

Conversation

pie6k
Copy link

@pie6k pie6k commented Jul 26, 2024

Description of change

Currently, when creating root and checking if target is HTMLCanvasElement we check it with target instanceof HTMLCanvasElement

It will break in quite rare-use case, when rendering content into another window (created with const newWindow = window.open() using React portal.

In this case - created canvas is instance of newWindow.HTMLCanvasElement, not global HTMLCanvasElement (or technically window.HTMLCanvasElement). As a result - the app will crash.

Use case for such a scenario is eg. creating multi-window Electron applications, when child windows are created using window.open(). I described this architecture in detail here: https://pietrasiak.com/creating-multi-window-electron-apps-using-react-portals

Fixes:

I added getIsCanvasElement function that is checking for this case and modified createRoot function to use it when checking if target is canvas.

Pre-Merge Checklist
  • Tests and/or benchmarks are included
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@pie6k pie6k changed the base branch from master to dev July 26, 2024 13:07
@trezy trezy changed the base branch from dev to beta July 26, 2024 17:41
@trezy
Copy link
Collaborator

trezy commented Jul 26, 2024

Great find! I think there's some weirdness on the branch, tho. Can you grab the latest beta branch, cherry-pick your commit onto there, and update this PR? Once you do it should be a pretty quick merge.

@trezy trezy added bug Something isn't working v8 Issues related to Pixi React v8 labels Jul 26, 2024
@trezy
Copy link
Collaborator

trezy commented Jul 30, 2024

@pie6k bumping this, as I'd love to get it merged soon. 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v8 Issues related to Pixi React v8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants