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

onbeforenavigate support for confirming navigation, especially via back arrow #1

Open
taion opened this issue Jun 23, 2020 · 9 comments

Comments

@taion
Copy link

taion commented Jun 23, 2020

First, great project – this set of features looks amazing and would simplify life greatly. This looks like this would let router maintainers move away from figuring out APIs to interface with the browser history API like https://github.com/ReactTraining/history and https://github.com/4Catalyzer/farce and instead focus on just the "routing" bits proper.

To the meat of this – one use case that is particularly annoying to implement in a good way right now is blocking navigation in cases where we have e.g. dirty forms – the SPA equivalent of beforeunload event handlers. In general, a modern SPA will often want to show e.g. a custom modal to allow users to confirm whether they actually wish to navigate, which means that we won't know whether the user actually wants to navigate synchronously. Handling this well can require building out some way to "pause" and then "resume" the same navigation action, which potentially involves a lot of finicky state tracking: https://github.com/4Catalyzer/farce/blob/v0.4.1/src/createNavigationListenerMiddleware.js – e.g. what if the user clicks on a link while I'm showing a "are you sure you want to navigate" modal.

Note that this issue especially arises in the context of navigation via the back arrow. If I want to "continue" with a navigation that resolves to a pushState, it seems like I could just manually call history.pushState, but unless I'm missing something, it could be awkward to "block" a navigation triggered by the back button, because window.history.goBack() might trigger onbeforenavigate again.

@taion
Copy link
Author

taion commented Jun 23, 2020

To be more concrete, the pattern that I'm finding tricky/annoying to implement right now looks like:

  • User is on /foo
  • User does SPA navigation, we call history.pushState to /bar
  • User starts filling out a form on /bar
  • User hits browser back arrow

At this point, what I would like to do is:

  • Block the "back" navigation action
  • In JavaScript, show a modal/dialog/whatever to confirm that the user wants to navigate
  • If the user confirms, then actually go back to /foo (and also be 1 entry up in the navigation stack)

The best I can do right now involves immediately reversing the user's navigation action (e.g. by calling history.go(1)... except more finicky because I need to calculate the delta on the transition) and doing a bunch of state management to figure out how to replay the navigation action when the user does confirm navigation, but not get stuck in an infinite loop of triggering the same logic again.

@phistuck
Copy link

I think this is needed. The current way of blocking a history.pushState (or clicking on the browser back button after a history.pushState) navigation is extremely hacky (re-pushing the previous navigation) and can break the history list in cases where the user tries to go more than one entry backward/forward.
What about introducing an OpaqueHistoryItem that we could then go to if the user confirmed that they we can discard the data?
This will change history.go to history.go(numberOrHistoryItem)
And onbeforenavigate would fire even after pushState, and have an event.historyItem with the OpaqueHistoryItem (it can be a non-opaque HistoryItem if it is a same-origin navigation, but that is not important).

I am also fine with firing the event only on same-origin navigations (and just using beforeunload for the other cases).
I am also fine with forgetting the whole thing and simply making beforeunload work for history.pushState navigations. Ugly, but at least the data is not lost and the history list is not getting messy.

@taion
Copy link
Author

taion commented Jun 24, 2020

Alternatively, something like event.fireDefault or event.triggerDefault might work – just to undo the preventDefault. It'd only work for this specific case though.

@phistuck
Copy link

Or similar to the fetch event of service workers, event.waitUntil(promise).

@atotic
Copy link

atotic commented Jun 24, 2020

I call this feature "resume". How about something like this:

history.onbeforenavigate = (e) => {
  // create a history token we can navigate to
  resume = history.createResume(onBeforeNavigateEvent);
  e.preventDefault();
};

// Later, in some other code, user confirms, we are ready to continue
history.resume(resume)

Tricky bits:
Q: what happens if we create different resume tokens for different events, then resume an old one?

Last resume token wins. Older tokens do not trigger navigation in history.resume (throw?).

Q: what happens if multiple event handlers create resume tokens for same event?
They all get the same token. If we want to get fancy, token could have a "outstanding waits" counter, and resume would only work once token goes down to 0.

@tbondwilkinson
Copy link
Collaborator

There are some security concerns with being able to block navigations (especially back navigations), where websites have and continue to abuse the history APIs to prevent users from being able to navigate off their pages.

But I think this specific idea has some precedent in beforeunload, which allows you to display a dialog. We could hew to that API, or think about providing something slightly different.

@taion
Copy link
Author

taion commented Jul 31, 2020

@tbondwilkinson beforeunload already handles the case of the user trying to navigate off my page entirely, I think. I'm more concerned using e.g. back arrow to go between history entries of my SPA, in cases where e.g. I don't fully persist the contents of forms to any more durable storage mechanism. I don't think there ought to be a security concern in that case? It's all within the SPA.

@tbondwilkinson
Copy link
Collaborator

Imagine I'm on a page, and I do a quick history.pushState on page load. Then the user presses back. I catch that back navigation (which is between two states same-origin), and suppress it. The user presses back again, and I suppress it infinitely. The user is now stuck on my page. "security" perhaps isn't the best word to describe this abuse - but you can see how it does clearly lend itself to abuse.

But can you use the proposed beforenavigation event to persist the contents of your form?

@taion
Copy link
Author

taion commented Jul 31, 2020

I see what you mean. Having some limited ability to show a dialog would probably be good enough for me. All else being the same, I'd prefer to show my current styled dialog, but e.g. if I could throw away the code I have to "rewind" the history state changes, and all it cost me was that the dialog looks like the before unload dialog, then I would make that trade.

The "persist form state" option was something we'd considered, but we decided for UX reasons to just require confirmation to exit the form instead – ultimately this is a matter of what my client-side router displays, not directly an issue of the behavior of the history stack – i.e. my router would still have to do some sort of work to stop rendering the form, even after the user has hit "back", as long as he or she is still on my SPA. Blocking the change to the history stack itself is more of a "tidiness" thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants