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

Forward/Back doesn't trigger navigate could cause restoration headaches #53

Closed
tbondwilkinson opened this issue Mar 2, 2021 · 8 comments · Fixed by #56
Closed

Forward/Back doesn't trigger navigate could cause restoration headaches #53

tbondwilkinson opened this issue Mar 2, 2021 · 8 comments · Fixed by #56
Labels
foundational Foundational open questions whose resolution may change the model dramatically

Comments

@tbondwilkinson
Copy link
Contributor

One primary use case for listening to events is to restore UI.

Imagine for instance, you push a new URL, and in a navigate handler respond to that URL change by rendering a new full page UI and showing it on the page.

Then, say someone navigates back. How are you supposed to update your UI to remove the full page UI you added in response to the last URL update? Should you be listening to currentchange? If you do decide to listen to currentchange to handle this use case, how do you differentiate between back/forward navigations and other navigations that are handled by the navigate event handler?

I think there's a few options to solve this:

  1. Do trigger navigate on back/forward but do NOT allow it to be intercepted.
  2. Introduce a new event that handles this specific case (so it would be mutually exclusive with navigate), something like navigatedirection.
  3. Add more fields to currentchange so you could know the cases where a currentchange is sent but a navigate is not .

There may be other options as well

@domenic
Copy link
Collaborator

domenic commented Mar 2, 2021

See also #32 where this restriction is discussed more generally. But this particular use case is worth discussing in more detail.

The big foundational question you're getting at is, should UI rendering be done in navigate or in currentchange? We've been guiding people toward doing it as part of intercepting navigate, using event.respondWith(). And that seems like a better architecture. Telling them that they need to switch to a different strategy for back/forward navigations seems subpar.

I wonder if maybe the solution is to fire navigate, and allow event.respondWith(), but not allow event.preventDefault()? I suspect that I had the two conflated in my mind.

This could get a bit strange, e.g.:

  1. The user arrives at https://example.com/foo (history entry A)
  2. The user cross-document navigates to https://example.com/bar (history entry B)
  3. The user clicks back, but a navigate handler intercepts with event.respondWith() and converts it into a same-document navigation.

What would happen here from the JS page's point of view is that:

  • The Document stays the same
  • location.href updates to https://example.com/foo
  • appHistory.current becomes appHistory.entries[0]
  • A bunch of events fire

What would have to happen under the covers in the browser is that we update history entry A's document to be the same as history entry B's document. This kind of modification of past entries is unprecedented, and I wonder if it would cause issues. /cc @natechapin

A fallback position would be that we don't allow event.respondWith() (and maybe don't fire navigate?) for cross-document cases like the above. But that would still allow intercepting same-document back navigations, so scenarios like the following would still work:

  1. The user arrives at https://example.com/foo
  2. The user same-document navigates to https://example.com/bar because https://example.com/foo is a SPA
  3. The user clicks back, but a navigate handler intercepts with event.respondWith() and does appropriate rendering work to render /foo.

@tbondwilkinson
Copy link
Contributor Author

I think part of this is that we need to differentiate between a new navigation (i.e. pushing a new URL onto the stack) and a directional navigation (i.e. back/go/forward).

I think the directional navigations need to be more restrictive in what we allow. You should be able to respond to these events to update the content of the DOM, but as you mention in your example, I don't know that we want people to be able to convert cross-document directional navigations into same-document navigations. That feels reaaaally wild.

We have a few cases:

  1. Any navigate that will result in a new history entry should be in full control of the page, because the page itself is launching that navigation.
  2. Any directional navigate that is same-document+same-origin is in full control of the page, because the page itself owns both endpoints of that navigation. But because this is a directional navigate, I do not think we want to allow full respondWith() controls to change the URL or state, e.g.
  3. Any directional navigate that is cross-document (so back/forward to a different document) we need to decide restrictions for, but certainly they should NOT be convertible into a same-origin nav, so a full respondWith is out of the question (but maybe you could delay the directional nav from occurring with a Promise, like the beforeunload case).

This is seeming to me that maybe we need to bifurcate the navigate event entirely, into navigatations that are adding a new page (where we have lots and lots of controls) vs directional navigations, which are really more about restoration

@domenic
Copy link
Collaborator

domenic commented Mar 2, 2021

Good breakdown. A couple notes:

Any directional navigate that is same-document+same-origin is in full control of the page, because the page itself owns both endpoints of that navigation. But because this is a directional navigate, I do not think we want to allow full respondWith() controls to change the URL or state, e.g.

"Full control" needs the caveat that we can allow updating the content with respondWith(), but I don't think we can allow preventDefault() easily because of back-trapping abuse. That is, we need to make sure the user clicking "back" actually moves them through the history list, so that they can make progress toward escaping the site entirely with enough back presses.

And indeed, about changing the URL or state, this is making me reconsider the discussion in #5 about using respondWith() for that.

This is seeming to me that maybe we need to bifurcate the navigate event entirely, into navigatations that are adding a new page (where we have lots and lots of controls) vs directional navigations, which are really more about restoration

I lean toward keeping them unified. In particular, consider code such as

appHistory.addEventListener("navigate", e => {
  if (!e.sameOrigin || e.hashChange) {
    return;
  }
  if (routesTable.has(e.destination.url)) {
    const routeHandler = routesTable.get(e.destination.url);
    e.respondWith(routeHandler());
  }
});

I think this code works mostly fine even for directional navigations. In particular it works fine for SPA app same-document directional navigations. Let's say /foo is in the routing table. Then it doesn't really matter whether you're reaching /foo via a link or via a back navigation: you still want to go the respondWith() route.

It fails for cases like my cross-document nav upthread, where the app has decided to let /foo -> /bar be a cross-document nav, but /foo is in the SPA routing table at /bar. That's a bit of an edge case, but I can imagine it arising.

I think it would be fixed by:

Then you would do

appHistory.addEventListener("navigate", e => {
  if (!e.canRespond || e.hashChange) {
    return;
  }
  if (routesTable.has(e.destination.url)) {
    const routeHandler = routesTable.get(e.destination.url);
    e.respondWith(routeHandler());
  }
});

and the unified event would work perfectly.

@tbondwilkinson
Copy link
Contributor Author

tbondwilkinson commented Mar 2, 2021

"Full control" needs the caveat that we can allow updating the content with respondWith(), but I don't think we can allow preventDefault() easily because of back-trapping abuse. That is, we need to make sure the user clicking "back" actually moves them through the history list, so that they can make progress toward escaping the site entirely with enough back presses.

Right, though as always I do wonder whether this might be security theater, as there are other ways to trap users pretty easily without doing anything with responding to the back event (for example, pushing an extra entry in currentchange immediately after a back finishes with the same URL that was removed). We might be better served focusing on giving users control to escape a page altogether rather than trying to limit the JavaScript API. For example, instead of limiting abusive use of alert(), the browser detects and enables the user to prevent the page from showing more. There COULD be instances where blocking the back is actually desirable from a user perspective, so maybe we should let the user decide when they're unhappy with how the page is behaving.

And indeed, about changing the URL or state, this is making me reconsider the discussion in #5 about using respondWith() for that.

Yeah, I wonder about this too. I kind of lost track where we decided the timing of the URL update as well, as this plays into it.

and the unified event would work perfectly.

I do think that people might want to know whether the navigate is a brand new navigate or not. I bring this up knowing it's degenerate, but you could imagine someone reusing the same URL twice in the history stack - and the router could get confused seeing the same URL thinking that they're navigating backwards when in reality they're pushing a new URL that happens to have the same route.

There are other differences between new navigates and directional navigates, like directional navigate's destination entry will already have a unique key.

Another idea is that we do keep navigate() as is, but we introduce a new event that's earlier (preparenavigate, maybe) that allows you to modify the url/state for new navigations, such that you could do some amount of url/state munging before the navigate handler runs which is the primary router. Then, we take away the power for the navigate handler to update the url/state or do a redirect (minus completely cancelling the default and kicking off a new navigate). This would also allow the "correct" URL to show before the navigate handler, if we decide that the URL must update before the navigate handler runs.

@domenic domenic added the foundational Foundational open questions whose resolution may change the model dramatically label Mar 3, 2021
@domenic
Copy link
Collaborator

domenic commented Mar 3, 2021

I do think that people might want to know whether the navigate is a brand new navigate or not. I bring this up knowing it's degenerate, but you could imagine someone reusing the same URL twice in the history stack - and the router could get confused seeing the same URL thinking that they're navigating backwards when in reality they're pushing a new URL that happens to have the same route.

With the current design, you should be able to tell using appHistory.entries.includes(event.destination).

Another idea is that we do keep navigate() as is, but we introduce a new event that's earlier (preparenavigate, maybe) that allows you to modify the url/state for new navigations, such that you could do some amount of url/state munging before the navigate handler runs which is the primary router.

Yeah, this might be the right route toward solving #5.

@tbondwilkinson
Copy link
Contributor Author

With the current design, you should be able to tell using appHistory.entries.includes(event.destination).

That's true I suppose but it feels like it's a foundational use case for routers to restore a previous page, vs. navigating to a new page. A lot of routers would store things in memory or elsewhere to make sure that navigations can be snappy. Making it clear how to do that from an API perspective (i.e. a field on the navigate event to indicate if it's a directional navigate or a new entry, but the initiator may help with that), or at least having an example in the explainer would be good.

@domenic
Copy link
Collaborator

domenic commented Mar 3, 2021

I'd love to see an example, ideally small enough to port into the explainer, of how code would behave differently in such scenarios. Would you be able to write one up?

@tbondwilkinson
Copy link
Contributor Author

Roughly...?

appHistory.addEventListener('navigate', (event) => {
  event.respondWith((async () => {
    if (this.currentPage) {
      await this.currentPage.transitionOut();
    }
    if (event.destination.key && this.previousPages.has(event.destination.key)) {
      await this.previousPages.get(event.destination.key).transitionIn();
    } else {
      await this.renderPage(event.destination);
    }
  })());
});

domenic added a commit that referenced this issue Mar 3, 2021
* Closes #53 by allowing respondWith() for same-document back/forward navigations (but not yet allowing preventDefault(); that's #32).
* Closes #51 by transitioning from event.sameOrigin to event.canRespond, which has updated semantics.
* Does not fire navigate events for document.open(), as per some initial implementation investigations that's more trouble than it's worth.
* Adds an example for special back/forward handling in the navigate event, per #53 (comment).
* Makes the appendix table a bit more precise about userInitiated, and expands it to cover cancelable and canRespond.
domenic added a commit that referenced this issue Mar 4, 2021
* Closes #53 by allowing respondWith() for same-document back/forward navigations (but not yet allowing preventDefault(); that's #32).
* Closes #51 by transitioning from event.sameOrigin to event.canRespond, which has updated semantics.
* Does not fire navigate events for document.open(), as per some initial implementation investigations that's more trouble than it's worth.
* Adds an example for special back/forward handling in the navigate event, per #53 (comment).
* Makes the appendix table a bit more precise in various ways, and expands it to cover cancelable and canRespond.
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.

2 participants