Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Implement adding event listeners for beforeunload #176

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TomiS
Copy link

@TomiS TomiS commented Apr 14, 2020

The intention is to enable using this browser feature:
https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload

There seems to already be a type for Webapi.BeforeUnloadEvent, but not a attach an event listener for it. So this PR adds it.

Comment on lines 8 to 13
[@bs.get] external returnValue : t => string = "";
/**
* returnValue cannot be unset because BS cannot (afaik) bind `delete e['returnValue'];`
* Setting the value to undefined does not cause expected behavior.
*/
[@bs.set] external setReturnValue: (t, string) => unit = "returnValue";
Copy link
Member

Choose a reason for hiding this comment

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

returnValuesi actually defined onEvent`, which this inherits from. Could you move it there instead?

It;s also possible to write a function that does a delete, but you'll have to use %raw. Something like this should work I think:

let deleteReturnValue: t => unit = [%raw event => {|
  delete event['returnValue']
|}];

Copy link
Author

Choose a reason for hiding this comment

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

Ah, indeed it is defined in Event. I'll move it there and try adding deleting.

@yawaramin yawaramin added the blocked Waiting on someone to get back label Jul 5, 2020
tests and comment

fix comment
@TomiS
Copy link
Author

TomiS commented Sep 22, 2020

Ok, now I changed the returnValue to be inside Event (including set and delete). But I started reading the Event docs more carefully and they mention the value should be a boolean. However in BeforeUnloadEvent docs they talk about non-empty strings. So the same field should contain different type of data depending on the event type, right?

Did I mention I love the web platform...

Copy link
Member

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

Hmm, yeah that's a problem. The DOM specification for Event considers returnValue"historical", whatever that means, so perhaps we could get away with having the setter only accept a string, and to have it only on BeforeUnloadEvent.

But either way the getter can return both boolean and string values, so to be safe it needs to account for that. Not sure if we do that anywhere here already, but if we do this ought to mimic that. Otherwise I think using Js.Types.classify and returning a polymorphic variant is an OK approach.


preventDefault(event);
stopImmediatePropagation(event);
stopPropagation(event);
setReturnValue(event, "Test");
deleteReturnValue(event);
Copy link
Member

Choose a reason for hiding this comment

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

Can you commit the JS artifacts as well so we can ensure this generates correct code?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked Waiting on someone to get back
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants