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

Event: Use .preventDefault() in beforeunload #5626

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

mgol
Copy link
Member

@mgol mgol commented Feb 10, 2025

Summary

So far, a result of an event handler has been assigned to the returnValue of the original event by jQuery. Initially, one could pass a message the browser will then display to the user. Since that got abused a lot, every browser stopped using the provided string and they all now provide a generic message. From the browsers supported in v4, only IE 11 would still display the message.

Incidentally, IE 11 is the only browser from our supported ones which respects the value returned from a beforeunload handler attached by addEventListener; other browsers do so only for inline handlers, so not setting the value directly shouldn't reduce any functionality.

This looks like a good moment to stop passing the message through and just call event.preventDefault() without extra checks which is shorter. This used to not work in Chrome but it got implemented in Chrome 119.

Unfortunately, it's hard to test this event in unit tests since it blocks page dismissal.

Size difference:

main @667321eb2d1c4328b993c25fbe2342a01ec4f87f
   raw     gz     br Filename
   -37    -17    +15 dist/jquery.min.js
   -37    -18     -7 dist/jquery.slim.min.js
   -37    -17    +31 dist-module/jquery.module.min.js
   -37    -19    -10 dist-module/jquery.slim.module.min.js

Checklist

Sorry, something went wrong.

@mgol mgol added Event Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Feb 10, 2025
@mgol mgol self-assigned this Feb 10, 2025
@mgol mgol marked this pull request as ready for review February 10, 2025 23:17
So far, a result of an event handler has been assigned to the `returnValue`
of the original event by jQuery. Initially, one could pass a message the browser
will then display to the user. Since that got abused a lot, every browser
stopped using the provided string and they all now provide a generic message.
From the browsers supported in v4, only IE 11 would still display the message.

Incidentally, IE 11 is the only browser from our supported ones which respects
the value returned from a beforeunload handler attached by `addEventListener`;
other browsers do so only for inline handlers, so not setting the value directly
shouldn't reduce any functionality.

This looks like a good moment to stop passing the message through and just call
`event.preventDefault()` without extra checks which is shorter. This used to
not work in Chrome but it got implemented in Chrome 119.

Unfortunately, it's hard to test this event in unit tests since it blocks page
dismissal.
@mgol mgol force-pushed the beforeunload-preventDefault branch from b0cd151 to 8a0f144 Compare February 10, 2025 23:22
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

The reasoning makes sense to me, and the comment is informative.

@timmywil timmywil removed Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Feb 17, 2025
@mgol mgol merged commit 7c123de into jquery:main Feb 18, 2025
17 checks passed
@mgol mgol deleted the beforeunload-preventDefault branch February 18, 2025 19:54
@mgol mgol added this to the 4.0.0 milestone Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants