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

Consider cancelling pending forward navigations when going back #13

Closed
esprehn opened this issue Feb 3, 2021 · 6 comments · Fixed by #68
Closed

Consider cancelling pending forward navigations when going back #13

esprehn opened this issue Feb 3, 2021 · 6 comments · Fixed by #68
Labels
foundational Foundational open questions whose resolution may change the model dramatically

Comments

@esprehn
Copy link

esprehn commented Feb 3, 2021

Queued forward navigations should automatically cancel when executing a backwards navigation.

ex. In the photo gallery example:

  1. Start on /photos/2
  2. Click Next button which starts a navigation to /photos/3 with pushNewEntry(callback)
  3. Navigation event fires and respondWith() starts a 500ms animation
  4. Click Next 9 more times queuing 9 callbacks with pushNewEntry(callback)
  5. Click Back button taking you to /photos/1
  6. No more navigations happen, all 9 callbacks are cancelled.

The expected user behavior is not that the app goes back to /photos/1 and then jumps forward 9 times, but that all pending forward navigations are cancelled whenever going backwards.

I don't think this should be done manually through an AbortSignal. In practice the queued navigations are likely to come out of third party libraries or unrelated components.

@tbondwilkinson
Copy link
Contributor

Could this be handled by the router itself? Or are you suggesting this is universally desirable?

I think the router (which knows about pending navigations) should be responsible for cancelling the push navigations on a back click. I'm not convinced the framework itself should do this behavior.

@esprehn
Copy link
Author

esprehn commented Feb 11, 2021

@tbondwilkinson I think this does need to be done by the platform because navigations come from multiple uncoordinated libraries on the page.

I'm not sure I understand what you mean by the router knowing about pending navigations. There's no router here, just calls to pushNewEntry() from libraries all over the page. The spec does not expose a way to cancel a navigation, or get the list of pending navigations currently. You could imagine adding that of course, though I think this behavior makes sense as the default because I can't imagine the common scenario is one where users expect queued forward navigations to continue after clicking back. No existing web apps or native apps I'm aware of do that?

@tbondwilkinson
Copy link
Contributor

I do think that most applications will choose to have one central navigate listener installed on the page that will do the basic work of routing, even if there are multiple uncoordinated libraries on the page.

@esprehn
Copy link
Author

esprehn commented Feb 11, 2021

@tbondwilkinson I think you're misunderstanding the ticket here. The spec provides no facility for that central navigate listener to inspect the pending navigation stack or to cancel a pending navigation. Calls to pushNewEntry() go into a queue you can't interact with. There's also no way to know when the sequence of navigate events comes in should you want to cancel them, and that starts to make the router abstraction fairly complex.

I think requiring a sophisticated router that is observing all forward and backward navigation and picking and choosing what to cancel would be unfortunate and likely leads to a lot of buggy web apps compared to native. I don't think any real app jumps forward like this after going back.

@tbondwilkinson
Copy link
Contributor

Sorry, I'm not being clear.

  1. I agree that the spec does not currently provide mechanisms to discard queued navigations. I think that's discussed here: https://github.com/WICG/app-history#queued-up-single-page-navigations, but it's not really fleshed out what the right API is, and there's some todos. So there's definitely work to be done.
  2. What I'm trying to respond to is that I'm not sure I agree that it would be right to discard certain queued navigations by default (rather than implementing an API that allows application code to respond to a push by discarding navigations), but I could be convinced with more examples, and as long as there aren't counter-examples where this would prevent applications from doing things that are reasonable.

@tbondwilkinson
Copy link
Contributor

tbondwilkinson commented Feb 11, 2021

Part of my concern with this is that browsers have chosen different default behaviors for this exact use case today:

https://docs.google.com/document/d/1Pdve-DJ1JCGilj9Yqf5HxRJyBKSel5owgOvUJqTauwU/edit

This is certainly broken. I think the only two choices are making it explicit in the spec, and really spending the time to spell out every edge case, and then making tests to ensure all browsers are consistent, or just punting and making the application do the cancellation.

But I'm starting to come-around to thinking that reasonable defaults is more desirable than making the application router do more work. Certainly less likely to be buggy, as you point out.

@domenic domenic added the foundational Foundational open questions whose resolution may change the model dramatically label Feb 17, 2021
domenic added a commit that referenced this issue Mar 11, 2021
Closes #38 by making it explicit that the stop button is meant to trigger the navigate event's AbortSignal.

Closes #10 by getting rid of queued-up navigations, their associated event, and their associated callback variants to update() and push(). Instead, navigations during other navigations interrupt the ongoing one. As the new example tries to show, this seems to work pretty well.

Closes #13 by canceling all pending navigations when another one starts, of which the case discussed there is one example.
domenic added a commit that referenced this issue Mar 11, 2021
Closes #38 by making it explicit that the stop button is meant to trigger the navigate event's AbortSignal.

Closes #10 by getting rid of queued-up navigations, their associated event, and their associated callback variants to update() and push(). Instead, navigations during other navigations interrupt the ongoing one. As the new example tries to show, this seems to work pretty well.

Closes #13 by canceling all pending navigations when another one starts, of which the case discussed there is one example.
domenic added a commit that referenced this issue Mar 16, 2021
Closes #38 by making it explicit that the stop button is meant to trigger the navigate event's AbortSignal.

Closes #10 by getting rid of queued-up navigations, their associated event, and their associated callback variants to update() and push(). Instead, navigations during other navigations interrupt the ongoing one. As the new example tries to show, this seems to work pretty well.

Closes #13 by canceling all pending navigations when another one starts, of which the case discussed there is one example.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
foundational Foundational open questions whose resolution may change the model dramatically
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants