-
Notifications
You must be signed in to change notification settings - Fork 187
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
[BUG] semantics of (onbeforeunload, Dom_html.handler) incompatible with major browsers #1436
Comments
I think I understand the issue. let unsafe_cast (x : ('c, 'a -> 'b) Js.meth_callback) : ('c, 'a) Dom.event_listener = Obj.magic x;;
let prompt_before_unload () : unit =
let cb (e : Dom_html.event Js.t) = Dom.preventDefault e; Js._false in
Dom_html.window##.onbeforeunload := unsafe_cast (Js.Unsafe.callback cb);;
let resume_before_unload () : unit = Dom_html.window##.onbeforeunload := Dom.no_handler |
Thanks a lot, @hhugo ! Your approach looks better indeed :) I'll test this as soon as possible in learn-ocaml. Just asking in the meantime: do you believe the Anyway, more testing is needed! (and sorry if I can't send more feedback quickly enough…) |
I'm not sure, you should keep this issue open until one figure out what to do. |
Describe the bug
Adding/removing hooks for the beforeunload event with
Dom_html.window##.onbeforeunload := Dom_html.handler …
is not working out-of-the-box with both Chromium, Firefox, and Safari. So I had to devise the following hack (unsafe JS code):— Source code: https://github.com/ocaml-sf/learn-ocaml/blob/a6e4c5e6/src/app/learnocaml_exercise_main.ml#L292-L310
Expected behavior
I open this PR in case it is possible for you to tweak
Dom_html.handler
, and be able to directly use js_of_ocaml's abstractions (notJs.Unsafe.js_expr
) for this use case, and stay compatible with (Chromium, Firefox, Safari…)FTR → I had sketched a possible explanation of why one such tentative code doesn't work in Firefox here:
ocaml-sf/learn-ocaml#467 (comment)
Anyway, if ever you'd think it is already possible to implement the snippet above without
Js.Unsafe.js_expr
(meaning there's no "bug" over there), I'm taker of any idiomatic snippet 👍Versions
Version of packages used to reproduce the bug:
(I'll be happy to provide more details on the versions used if need be)
Cc @yurug FYI
The text was updated successfully, but these errors were encountered: