Lib: fix onbeforeunload handler breaking navigation (#1436) #2159
Open
hhugo wants to merge 3 commits into
Open
Lib: fix onbeforeunload handler breaking navigation (#1436) #2159hhugo wants to merge 3 commits into
hhugo wants to merge 3 commits into
Conversation
hhugo
force-pushed
the
fixunloadevent
branch
2 times, most recently
from
c3936a5 to
ea37016
Compare
Member
Author
|
@TyOverby, this PR breaks some virtual_dom tests. Can you comment maybe ? |
Member
Author
|
@vouillon, any though ? |
hhugo
force-pushed
the
fixunloadevent
branch
2 times, most recently
from
d0b8fdb to
5f1dfc6
Compare
All modern browsers show a confirmation dialog when the beforeunload handler returns any value other than null or undefined. Dom.handler returned Js._true as the JS value true, which incorrectly triggered the dialog even when the handler intended to allow navigation. Fix by detecting beforeunload events at runtime in Dom.handler and Dom.full_handler: Js._true returns undefined to JS (allowing navigation without a dialog), while Js._false continues to call preventDefault and also sets event.returnValue for legacy browsers. Dom.invoke_handler normalizes undefined back to Js._true. Also adds Dom.beforeUnloadEvent and Dom_html.beforeUnloadEvent class types with the returnValue property, and types onbeforeunload to use beforeUnloadEvent instead of the generic event type. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Js.Unsafe.pure_js_expr returns a JS value, not an OCaml function, so the previous form could not call the predicate. Use Js.Unsafe.fun_call to invoke it with the event as argument. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1436.
Dom.handlerconverts the OCamlbool treturn value to a JavaScript return value. When abeforeunloadhandler returnsJs._true(allow navigation), the JS valuetrueis passed to the browser. Modern browsers interpret any non-null, non-undefined return value from abeforeunloadhandler as a request to show the "Leave page?" confirmation dialog. This means that even when the OCaml handler intended to allow navigation, the browser would show a blocking dialog.Changes
The fix detects
beforeunloadevents at runtime inDom.handlerandDom.full_handler:Js._true(allow navigation), returnundefinedto the browser instead oftrue, so no dialog is shown.Js._false(block navigation), also setevent.returnValue = ""for legacy browser compatibility.Dom.invoke_handler, normalizeundefinedback toJs._trueso callers that chain handlers still see a properbool tvalue.Also adds
Dom.beforeUnloadEventandDom_html.beforeUnloadEventclass types with thereturnValueproperty, and typesonbeforeunloadto usebeforeUnloadEventinstead of the genericeventtype.