Skip to content

Properly dispose of web ComposeWindow on removing canvas from DOM #2126

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

Open
wants to merge 1 commit into
base: jb-main
Choose a base branch
from

Conversation

cedrickcooke
Copy link

@cedrickcooke cedrickcooke commented May 16, 2025

Prior to this, web's ComposeWindow.dispose() function was never called. Now, dispose is called when the ComposeWindow's canvas is removed from the html document. This approach allows for proper cleanup in cases where a web SPA uses compose for some (but not all) logical pages.

Testing

Before implementing this work, I updated ComposeWindowLifecycleTest.allEvents to pass consistently. Then I implemented the disposal call, and extended the allEvents test to trigger a disposal and check the new lifecycle events.

Release Notes

Fixes - Web

  • Dispose of ComposeWindow when removed from document.

@ApoloApps
Copy link

ApoloApps commented May 16, 2025

Calling dispose in beforeunload is wrong. This event can be used to show a dialog confirming if the user wants to close the tab, but the user can click cancel and tab closing will be prevented which will leave you with no content since it has been disposed

@cedrickcooke cedrickcooke changed the title Properly dispose of web ComposeWindow on appropriate events Properly dispose of web ComposeWindow on removing canvas from DOM May 16, 2025
@cedrickcooke
Copy link
Author

Good callout, I've updated things to only use the canvas removal trigger

@cedrickcooke
Copy link
Author

Sorry for the delay on that.

In my original local testing the window was grabbing focus when launched, so the compose window was initialized in the RESUMED state. It seems like on the test runners the window isn't initially focused, so it was stuck on STARTED.

I've updated the test to send an extra focus event before the first check for RESUMED. If the window is already focused this is a no-op, and if the window is not focused this will push it into the RESUMED state. Then the test performs a full blur/focus loop as before.

@cedrickcooke cedrickcooke force-pushed the cedrickc/web-compose-window-disposal branch from 369cd33 to 025c1b5 Compare July 14, 2025 18:14
@cedrickcooke
Copy link
Author

Please forgive the force-push. This has been updated to work with the shadow root changes. While doing so, I also updated this test to use the createComposeWindow function like the other tests.

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.

2 participants