-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: do not use unload
events in Chromium from App Runner
#30061
Conversation
unload
events in Chromium from App Runnerunload
events in Chromium from App Runner
isBrowserFamily (family) { | ||
return getRunnerConfigFromWindow()?.browser?.family === family |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is expecting a string, but it's being passed an object so this won't ever compare correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd waffled too many times between an obj and a string parameter - adding type declaration to parameter will help catch this in the future. addressed in 4af0b99
…r in chrome add check for msMatchesSelector in jquery patch so unload is only added in ie
@@ -89,7 +89,7 @@ index 773ad95..84ca2f6 100644 | |||
+ // IE/Edge sometimes throw a "Permission denied" error when strict-comparing | |||
+ // two documents; shallow comparisons work. | |||
+ // eslint-disable-next-line eqeqeq | |||
+ if ( preferredDoc != document && | |||
+ if ( docElem.msMatchesSelector && preferredDoc != document && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still relevant? Mostly curious how you tested this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically a cherry-picked patch from jQuery 3.7.1. when manually testing this in chrome, I was still receiving unload errors coming from jquery without this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing the unload warning displaying from our own code anymore. I still see one coming from jQuery but will have to test with the built binary I guess since that's patched.
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
At the point when the Runner's
EventManager
adds global event listeners,setup()
has yet to be called, so there is not an instance of Cypress to use for the browser family check. To fix,getRunnerConfigFromWindow()
is extracted into its own file so it can be imported by bothsrc/runner/index.ts
andsrc/runner/event-manager.ts
without causing a circular dependency.Steps to test
Run a simple test in open mode with the following Cypress config:
Observe that there are no warnings in the browser console about a disallowed
unload
event.This behavior cannot be tested with Cy in Cy tests, as far as I can tell, and there are no unit tests for
app
, so this fix is currently only manually tested.How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?