window.onbeforeunload not warning when navigating or on popstate
bryceosterhaus opened this issue · 17 comments
Expected behaviour
Adding window.onbeforeunload = () => 'test'; on a page would allow for default browser behavior of warning the user when they reload or navigate away from the page.
Actual behaviour
Adding the onbeforeunload only warns when refreshing the page, not navigating to a new page on on popstate
Steps to reproduce
- Run email example from repo
- Add this snippet to https://github.com/liferay/senna.js/blob/master/examples/email/dudu.html#L191
<script>
window.onbeforeunload = function() {
return 'test';
};
</script>- Navigate to the dudu.html page in the browser
- Hit the 'back' button or click on link to navigate to another page
- See that no warning message is thrown.
Browsers affected
All major browsers seem to be affected by this.
Sounds like a valid issue. Any idea how to handle onbeforeunload on SPA apps?
Not entirely sure. But I know when we used senna v0.4, it worked as expected for us.
I'm investigating that. But it seems to just dispatch an event named beforeunload with the expected event object. We also need to listen to the event and prevent navigation when necessary.
The MDN says: The beforeunload event is fired when the window, the document and its resources are about to be unloaded. The document is still visible and the event is still cancelable at this point.
Hey, @bryceosterhaus. Thank you for heads up. Could you explain how it was implemented? I have Senna v0.4.0 but I couldn't make it work as you said. I did this way:
- I've opened the email example
- I've run the snippet you sent in the console
- I've navigated to several web pages
- I've hit the back button.
Hey @fernandosouza, I'm not totally sure why it worked for us before. I dont think we did anything in particular. All we added was something along the lines of
window.onbeforeunload = function() {
return 'some warning message';
};There could have potentially been a bug in our code that made it work on v0.4. Its likely that it might have never worked out of the box with senna.
Hey, @bryceosterhaus. For me don't make sense this code work with Senna.
The onbeforeunload specification says:
The WindowEventHandlers.onbeforeunload event handler property contains the code executed when the beforeunload is sent. This event fires when a window is about to unload its resources. The document is still visible and the event is still cancelable.
From what I know, the way Senna works it never unload the document.
What we can do is to call window.onbeforeunload as Chema said for this case.
Hmm. Maybe what we need to do is add an api that would allow the ability to alert when changing the page. We would need to handle for back/fwd, refresh, and navigate.
For our case specifically, it is for warning people if they have unsaved content. When not using senna, you would use window.onbeforeunload which is why we used that before.
onbeforeunload would handle the refresh case, for the rest, beforeNavigate should do the trick. If everyone agrees it's something that make sense to be added to senna, we could encapsulate that.
+1 on this issue, we have a similar use case as Bryce, and are using beforeNavigate to check if there are unsaved changes. But if we would like to use something like window.onbeforeunload to account for these use cases:
- Reload the page.
- Navigate away via clicking another URL
- Click "Close tab"
- Manually enter another URL into the nav bar
None of these are caught by the beforeNavigate event.
I would say it could be done like this:
- Before to navigate to another page, Senna checks if there is a function, attached to this event and executes it.
- It checks the returned result and if false - stops navigating.
- Since
beforeunloadcan be added viawindow.addEventListenertoo, we will have to monkey patchaddEventListener(andremoveEventListener) to cover this case too.
Having spent a good deal of time understanding the Event API and the related bits to BeforeUnloadEvent, I've come to the conclusion that the real challenge would rather be to programmatically emit the BeforeUnloadEvent and make the browser to display the Confirm modal. BeforeUnloadEvent constructor cannot be called and initEventAPI is deprecated for ages. Even if we ported the event handling into Senna JS and patched the native addEventListener and removeEventListener (though I think that'd be a educated decision) we'd have to compose our custom Confirm modal.
My proposal is to add ability to specify the content of the Confirm modal like as follows:
<div id="beforeunload-modal" data-senna-beforeunload-modal>
<h1>Are you sure you want to leave the page?</h1>
<button data-senna-beforeunload-modal-confirm>Yes</button>
<button data-senna-beforeunload-modal-dismiss>No</button>
</div>I'd cycled through existing solutions and it doesn't seem to be a widespread or straightforward workaround.
Hey @vbence86, that you very much for taking the time to look into this and share your findings. I agree with you that this is not straightforward nor a widespread approach either. I try to avoid patching native methods as much as possible. In this case, though, it might be necessary.
@brunobasto I'm currently working on a possible solution that demonstrate one way of getting around this issue. I'll post my Pull Request in due course that will serve as a base to kick around the idea.
@brunobasto Yes, this has been fixed by #222. @bryceosterhaus if you want to check it please go ahead and use the most recent master.
Awesome! Thanks, @vbence86. I"m closing it for now. We can reopen if @bryceosterhaus find any issues if the current fix.