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

Specify traversal and overhaul ongoing navigation tracking #109

Merged
merged 5 commits into from
Jul 27, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented May 7, 2021

This commit introduces a much more robust framework for tracking ongoing navigations. This fixes #130, for example, by appropriately keeping the signal around so that we can signal abort when necessary. It also ensures every same-document navigation ends in either navigationsuccess or navigationerror, with a few unified spec paths.

With this framework in hand, we can specify goTo()/back()/forward().


Preview | Diff

spec.bs Outdated Show resolved Hide resolved
@domenic
Copy link
Collaborator Author

domenic commented May 7, 2021

Per #110 I don't think appHistory.goTo()/back()/forward() will ever trigger the "allowed to navigate" check. So instead the potential error cases are unload/beforeunload/user refuses to allow the document to be unloaded.

Edit: per further discussions there I now realize it will.

@domenic
Copy link
Collaborator Author

domenic commented May 7, 2021

Probably navigate() should have the same "bails out during unload" behavior.

I'll block this on whatwg/html#5666

@jakearchibald
Copy link

What would you expect appHistory.forward(); appHistory.back() to do? Right now those methods look up state synchronously (current index), but ultimately navigations queue in some way.

appHistory.forward() also looks up the entry list size, which may be incorrect (in the same way history.length can be incorrect). We can try to keep it up to date like history.length, but maybe an async approach is better?

How I see traversal working with whatwg/html#6315:

  1. Let navigable be the current document's navigable.
  2. Let traversable be navigable's traversable.
  3. Queue the rest of the steps on that traversable navigable's session history traversal queue:
  4. Let targetStep be the result of getting the previous/next/goTo step given the current navigable (see below).
  5. If targetStep is a failure, reject promise.
  6. Apply the history step targetStep with checkForUserCancelation set to true and initiatorToCheck set to document's browsing context.

There are a few ways this can 'fail', and I'm not sure what you'd like to do for each of these cases:

  • The current browsing context is not allowed to navigate one of the navigables involved in this traversal. This causes the whole traversal to be abandoned.
  • One of the documents-to-unload asks for a beforeunload prompt to be shown, and the user presses cancel. This causes the whole traversal to be abandoned.
  • The traversal is cancelled in favour of another traversal, or the user presses 'stop' (I haven't factored this into the spec yet, but the current spec has it in a hand-wavey and incorrect way)
  • The network returns a Content-Disposition, 204, or 205 when fetching the document for the target entry for this navigable. This will result in the current navigable sticking with the same active session history item, but other navigables may successfully navigate as part of the traversal.
  • Other navigables involved in the traversal stick with their current active entry due to a Content-Disposition, 204, or 205 response.
  • Other navigables involved in the traversal display an error document due to some other fetch failure.

Getting the previous step given the current navigable:

  1. Let currentEntry be navigable's active session history entry.
  2. Let navigableEntries be navigable's session history entries.
  3. Filter navigableEntries so it contains contiguous entries that are part of the same origin and session. (I don't have an algorithm for this right now)
  4. If navigableEntries[0] is currentEntry, return failure.
  5. Let allSteps be all history steps in navigable's traversable navigable within navigable's BCG's session.
  6. Return the greatest number in allSteps that is less than currentEntry's step. This ensures we're going the minimum number of steps back to get to the previous entry for this navigable.

I'm not 100% sure if we should be using the active session history entry or the current session history entry. It kinda depends on what you expect to happen if back() is called, but the result is a 204, then back() is called again. If a 204 is returned, the 'current' history entry changes, but the 'active' entry stays the same.

Getting the next step given the current navigable:

  1. Let currentEntry be navigable's active session history entry.
  2. Let navigableEntries be navigable's session history entries.
  3. Filter navigableEntries so it contains contiguous entries that are part of the same origin and session. (I don't have an algorithm for this right now)
  4. If navigableEntries[navigableEntries.length - 1] is currentEntry, return failure.
  5. Return the step of the item in navigableEntries that appears after currentEntry.

Getting the goTo step given the current navigable and a key:

  1. Let currentEntry be navigable's active session history entry.
  2. Let navigableEntries be navigable's session history entries.
  3. Filter navigableEntries so it contains contiguous entries that are part of the same origin and session. (I don't have an algorithm for this right now)
  4. Let targetEntry be the entry in navigableEntries that has key == key (I guess this is where we'll put it)
  5. If targetEntry is currentEntry ???
  6. Return targetEntry's step.
@domenic
Copy link
Collaborator Author

domenic commented May 12, 2021

What would you expect appHistory.forward(); appHistory.back() to do? Right now those methods look up state synchronously (current index), but ultimately navigations queue in some way.

Good question. The second call should cancel the first (probably using something like the current spec's "Remove any tasks queued by the history traversal task source that are associated...") and thus win. And then we should propagate the failure back to the return value of the first call.

appHistory.forward() also looks up the entry list size, which may be incorrect (in the same way history.length can be incorrect). We can try to keep it up to date like history.length, but maybe an async approach is better?

I think we should do both. If the synchronously-accessible appHistory.canGoForward/appHistory.canGoBack are false, then appHistory.forward()/appHistory.back() should fail immediately. But yeah, they might also fail asynchronously.

How I see traversal working with whatwg/html#6315:

Thank you so much for outlining this and the accompanying algorithms!! I think I will try to just copy them in, linking to your draft PR. Trying to do anything rigorous given the current spec is an exercise in futility IMO.

The current browsing context is not allowed to navigate one of the navigables involved in this traversal. This causes the whole traversal to be abandoned.

Reject promise with "SecurityError" DOMException.

One of the documents-to-unload asks for a beforeunload prompt to be shown, and the user presses cancel. This causes the whole traversal to be abandoned.

Reject promise with an "AbortError" DOMException.

The traversal is cancelled in favour of another traversal, or the user presses 'stop' (I haven't factored this into the spec yet, but the current spec has it in a hand-wavey and incorrect way)

Reject promise with an "AbortError" DOMException.

The network returns a Content-Disposition, 204, or 205 when fetching the document for the target entry for this navigable. This will result in the current navigable sticking with the same active session history item, but other navigables may successfully navigate as part of the traversal.

Probably treat this as a success (see #95 for what that means). Especially since, IIUC, the plan is for this to update the "current" entry, even if it doesn't update the "active" one?

Other navigables involved in the traversal stick with their current active entry due to a Content-Disposition, 204, or 205 response.

Other navigables involved in the traversal display an error document due to some other fetch failure.

Definitely a success.

@jakearchibald
Copy link

What would you expect appHistory.forward(); appHistory.back() to do? Right now those methods look up state synchronously (current index), but ultimately navigations queue in some way.

Good question. The second call should cancel the first (probably using something like the current spec's "Remove any tasks queued by the history traversal task source that are associated...") and thus win. And then we should propagate the failure back to the return value of the first call.

That makes as much sense as anything. Ugh, I just gave some of this a quick test:

  • history.back(); history.back() - Seems to always go(-2)
  • history.forward(); history.forward() - Seems to always go(2)
  • history.back(); history.forward() - Seems to always go(-1)
  • history.forward(); history.back() - no-op if forward navigation is another page. go(1) if forward navigation is same document.

I'm sure there are more subtleties, and I only tested Chrome. Pretty sure this won't be interoperable.

appHistory.forward() also looks up the entry list size, which may be incorrect (in the same way history.length can be incorrect). We can try to keep it up to date like history.length, but maybe an async approach is better?

I think we should do both. If the synchronously-accessible appHistory.canGoForward/appHistory.canGoBack are false, then appHistory.forward()/appHistory.back() should fail immediately. But yeah, they might also fail asynchronously.

I can't tempt you to make canGoForward/canGoBack async? 😄

But yeah, this won't be hard to spec, as the hooks are already there for history length and index.

Thank you so much for outlining this and the accompanying algorithms!!

It was a good test of the central "go here" algorithm!

The network returns a Content-Disposition, 204, or 205 when fetching the document for the target entry for this navigable. This will result in the current navigable sticking with the same active session history item, but other navigables may successfully navigate as part of the traversal.

Probably treat this as a success (see #95 for what that means). Especially since, IIUC, the plan is for this to update the "current" entry, even if it doesn't update the "active" one?

Yeah, that's correct. I think "success" here is reasonable, even though it might be weird that a cross-document navigation is a success without changing page.

If we want to avoid this edge case, we can introduce a new traversal mode that creates an error document in these cases, so the page will change.

This isn't a problem for navigations that create a new history entry, or reloads, as the network stuff happens before trying to modify the history tree, and if the result is a 204/5/content-disposition then the history tree isn't modified at all.

spec.bs Outdated

1. If [=this=]'s [=AppHistory/current index=] is −1, then return [=a promise rejected with=] an "{{InvalidStateError}}" {{DOMException}}.

1. If [=this=]'s [=AppHistory/entry list=][[=this=]'s [=AppHistory/current index=]]'s [=AppHistoryEntry/session history entry=]'s [=session history entry/app history key=] equals |key|, then return [=a promise resolved with=] undefined.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not correct; it needs to check all the error conditions first. So this should move to "performing an app history traversal".

@domenic
Copy link
Collaborator Author

domenic commented May 13, 2021

Wow, the current spec draft doesn't actually return a promise for the success case. No wonder it's relatively simple. It needs to do similar things to how navigate() works, with "fire a navigate event" dealing with the promise.

@domenic
Copy link
Collaborator Author

domenic commented May 14, 2021

Some easy questions:

Let navigable be the current document's navigable.

Is this pointer defined anywhere in the draft?

Let targetStep be the result of getting the previous/next/goTo step given the current navigable (see below).

By this you mean navigable, not traversable?

Trickier question:

Getting the previous step given the current navigable:

Getting the next step given the current navigable:

Getting the goTo step given the current navigable and a key:

I was thinking we could unify these into just the goTo algorithm, since we already know what the next/previous key are. I don't believe it's possible for the notion of next/previous key to get out of sync, because anything that affects the next/previous key would have to navigate the current frame and we'd know about it.

Similarly,

Filter navigableEntries so it contains contiguous entries that are part of the same origin and session. (I don't have an algorithm for this right now)

I'm not sure the filtering is necessary since we already filtered the list of possible keys?

@domenic
Copy link
Collaborator Author

domenic commented May 14, 2021

Let currentEntry be navigable's active session history entry.

Should this be current or active? I think current?

@domenic
Copy link
Collaborator Author

domenic commented May 14, 2021

I'm realizing that I'm going to need an updated version of "traverse the history by a delta" in order to specify navigate events properly for history.back()/etc., and thus make sure I also specify them in a way that works for appHistory.back()/etc.

Would you be able to prioritize

JAKE-TODO: this needs to be rewritten to figure out the target step and run "apply the history step"

? I gave it a shot myself, but I'm unsure about the details of translating a delta into a step.

@jakearchibald
Copy link

Let navigable be the current document's navigable.

Is this pointer defined anywhere in the draft?

There's <span data-x="bc-navigable">containing navigable</span>, so you can go via the browsing context.

Let targetStep be the result of getting the previous/next/goTo step given the current navigable (see below).

By this you mean navigable, not traversable?

Yes. It has to be the navigable, since app history back/forward is specific to the navigable. history.back/forward could just target the traversable.

Getting the previous step given the current navigable:
Getting the next step given the current navigable:
Getting the goTo step given the current navigable and a key:

I was thinking we could unify these into just the goTo algorithm, since we already know what the next/previous key are.

I think it might be more complicated. For example:

  1. Navigate to page with 2 iframes - All three history entries are step 1
  2. Navigate iframe-1 to /foo - step 2
  3. Navigate iframe-2 to /bar - step 3
  4. Navigate iframe-1 to /hello - step 4

If iframe-1 calls back, I think we need to go to step 3, right? The idea is to traverse the minimum number of steps to activate that history entry, rather than go to the step where that history entry was created. Hmm, that means my goTo algorithm is wrong, although my back algorithm is right.

Filter navigableEntries so it contains contiguous entries that are part of the same origin and session. (I don't have an algorithm for this right now)

I'm not sure the filtering is necessary since we already filtered the list of possible keys?

Hmm, I'm not sure. The navigableEntries are filtered by origin, but the steps are only filtered by session. The steps are overall steps in the traversable, which will include other iframe and the top level, whereas the navigableEntries are the once the app history API has access to.

Let currentEntry be navigable's active session history entry.

Should this be current or active? I think current?

I'm still not 100% sure what to do here. But yeah, let's go for current for now.

Would you be able to prioritize

JAKE-TODO: this needs to be rewritten to figure out the target step and run "apply the history step"

Yeah, shall do

@jakearchibald
Copy link

Let currentEntry be navigable's active session history entry.

Should this be current or active? I think current?

Ok, I've thought about this some more, and I think this should be "active". In fact, I might even remove the concept of "current", since I think it's risky and can be replaced with something else.

Take the following history timeline:

Screenshot 2021-05-17 at 15 07 40

If we're on step 4, and want to go back(), but /foo returns a Content-Disposition/204/205, we still move the history pointer to step 3. This is done to avoid trapping the user in one session history entry.

However, when we go back() again, to step 2, only the second iframe should try to navigate. My initial spec was along the lines of "if your destination history entry is different to your active history entry, then you need to navigate", but that would try to navigate the first iframe again too, so I created the split between "current" and "active" history entries. However, I think I can achieve this by replacing "current history entry" with "get the history entry associated with the current history step", along with some warning saying "you probably want the active history entry".

For example:

Screenshot 2021-05-17 at 15 20 46

If we're on step 2, and go back(), but //example.com/ returns a 204, you wouldn't expect history.state to be {foo: 'bar'}, as that would be leaking data across origins. That's what would happen if history.state came from the "current history entry".

app-history doesn't have this risk, as it's limited to the current frame and can't cross origins, but it seems best for consistency that app-history gets its state from the active history entry. Otherwise, you can end up with "current" state that was never associated with the active document.

That raises questions over how appHistory.back() etc should behave:

Screenshot 2021-05-17 at 15 41 14

Let's say we're on step 3, but we're still showing /hello because /bar returned a 204.

history.back() operates like the back button, so it seems obvious that this should traverse to step 2 (/foo), even though, prior to calling history.back(), history.state would be whatever's associated with the entry in step 4. Essentially, history.back() decreases the step number by 1 (although in reality it'll skip steps that no longer have entries associated with them, due to removed iframes).

This isn't so clear with appHistory.back(), which deliberately scopes its view to a single navigable. Options:

  • Apply step 2. This means going back two entries compared to what's in appHistory.current, which is kinda weird.
  • Stay on step 3, but re-attempt to make /bar the active entry by requesting /bar again, which may result in another download/204/205.

I think the latter is more consistent with what app-history is trying to do. Maybe this also means that appHistory.back() should reject if the result is an unchanged appHistory.current, even if it successfully changes the history entry in other navigables.

@domenic
Copy link
Collaborator Author

domenic commented May 17, 2021

If iframe-1 calls back, I think we need to go to step 3, right? The idea is to traverse the minimum number of steps to activate that history entry, rather than go to the step where that history entry was created. Hmm, that means my goTo algorithm is wrong, although my back algorithm is right.

Correct, the intention would be to go to step 3. So I guess the key is to realize that a key uniquely identifies a session history entry, but does not uniquely identify a joint session history step. Makes sense.

I still think we should spec goTo and then spec back/forward in terms of those, though, if possible. I guess goTo would, given key:

  • Find all steps which contain the entry with key key
  • Pick the step S such that |S - currentStep| is smallest.

Hmm, I'm not sure. The navigableEntries are filtered by origin, but the steps are only filtered by session. The steps are overall steps in the traversable, which will include other iframe and the top level, whereas the navigableEntries are the once the app history API has access to.

Is the worry that, given a key mapping to multiple eligible steps, the closest eligible step might be... cross-origin? Different session? I'm not sure what the problem is... An example case would help (and we could convert it into a web platform test), like the one for the previous question which set me straight.

Ok, I've thought about this some more, and I think this should be "active". In fact, I might even remove the concept of "current", since I think it's risky and can be replaced with something else.

Hmm. So in your proposal, what is "get the history entry associated with the current history step" used for and what is active used for? From what I can tell:

  • current = joint session history backs (e.g. history.back() and the user pressing the reload button?). Maybe reloads?
  • active = history.state, appHistory.current, appHistory.back()... Also, it'll stay in sync with document.URL, although somewhat by coincidence.

If we do that, I agree "Stay on step 3, but re-attempt to make /bar the active entry by requesting /bar again, which may result in another download/204/205" seems like the natural behavior: it'll just try to re-traverse to the key which is previous to appHistory.current, which is consistent with my general desire to spec back() in terms of goTo().

Maybe this also means that appHistory.back() should reject if the result is an unchanged appHistory.current, even if it successfully changes the history entry in other navigables.

This would be ideal, I agree.

@jakearchibald
Copy link

I still think we should spec goTo and then spec back/forward in terms of those, though, if possible. I guess goTo would, given key:

  • Find all steps which contain the entry with key key
  • Pick the step S such that |S - currentStep| is smallest.

Taking another swing at it: Getting the goTo step given the current navigable and a key:

  1. Let traversable be navigable's traversable.
  2. Let activeEntry be navigable's active session history entry.
  3. Let navigableEntries be navigable's session history entries.
  4. Filter navigableEntries so it contains contiguous entries that are part of the same origin and session. (I don't have an algorithm for this right now).
  5. Let targetEntry be the entry in navigableEntries that has key == key.
  6. If targetEntry is activeEntry ???.
  7. If targetEntry's step is greater than traversable's step, return targetEntry's step.
  8. Let afterTarget be the item after targetEntry in navigableEntries.
  9. Let allSteps be all history steps in traversable within navigable's BCG's session.
  10. Return the greatest number in allSteps that is less than afterTarget's step.

Is the worry that, given a key mapping to multiple eligible steps, the closest eligible step might be... cross-origin? Different session? I'm not sure what the problem is... An example case would help (and we could convert it into a web platform test), like the one for the previous question which set me straight.

I might be getting confused. Here's where my mind's at:

Screenshot 2021-05-19 at 12 47 02

Here we've got 7 history entries (two of them in an iframe), 6 steps, and 5 entries that are linked to the top level navigable.

All steps are part of the same session. The session only changes if the user performs a manual cross-origin navigation outside the page.

If we're on step 6, the top level can only go to k3, k4 and k7 (which is the current step). Because:

  • k5 and k6 are associated with a different navigable (although k5 is on the same step as k4).
  • k1 and k2 are part of the same navigable, but they're not same-origin-contiguous with the current entry, even though k1 is same origin.

To enforce this it seems best to filter the history entries associated with the current navigable given a current step. That returns the entries with keys k3, k4 and k7.

When going from step 6 to key k4 we actually want to go to step 5, since this is the minimum movement. To do this, we:

  1. Let targetEntry be the entry in navigableEntries that has key == k4.
  2. Let afterTarget be the item after targetEntry in navigableEntries. This is the item with key k7 and step 6.
  3. Let allSteps be all history steps in traversable within navigable's BCG's session. This is 1-6 inclusive.
  4. Return the greatest number in allSteps that is less than afterTarget's step (6). So we return 5.

Filtering all history steps to a particular session probably isn't required, but I'd like to make the session a required argument for security reasons. Only the browser back/forward button should be able to bypass the session check.

Hmm. So in your proposal, what is "get the history entry associated with the current history step" used for and what is active used for? From what I can tell:

  • current = joint session history backs (e.g. history.back() and the user pressing the reload button?). Maybe reloads?
  • active = history.state, appHistory.current, appHistory.back()... Also, it'll stay in sync with document.URL, although somewhat by coincidence.

Yeah, that looks right to me. I'd need to do some tests of reload to be sure.

@domenic
Copy link
Collaborator Author

domenic commented May 19, 2021

If we're on step 6, the top level can only go to k3, k4 and k7 (which is the current step). Because:

Agreed. However, appHistory.entries().map(e => e.key) only contains k3, k4, and k7. I.e. the filtering has already been done. So I don't think we need to filter in the traversal step.

We do need to check that the key we're given is in (the spec equivalent of) appHistory.entries().map(e => e.key), since otherwise a cooperating iframe could give you an invalid key. But that check can be done synchronously.

Now, in implementations, probably we want to do the check in the browser process for security reasons... and maybe the spec should reflect that too. But I'm not sure it's necessary.

@domenic
Copy link
Collaborator Author

domenic commented May 19, 2021

navigable's BCG's session.

I don't see a definition for navigable's BCG in https://whatpr.org/html/6315/history.html#navigables ... should there be one?

@domenic
Copy link
Collaborator Author

domenic commented May 19, 2021

If targetEntry's step is greater than traversable's step, return targetEntry's step.

traversable's step is currently named "current session history step". Naming-wise, should that change to "active"? Is it equivalent to/redundant with the navigable's active session history entry's step?

In particular:

Let afterTarget be the item after targetEntry in navigableEntries.

I think you're assuming this always exists because you're handling three cases:

  • targetEntry step > "current step": return targetEntry's step
  • targetEntry step = "current step": ???
  • targetEntry step < "current step": do more complicated dance involving afterTarget

But this only works if the same notion of "current step" is used for all three, which I'm not sure is true.

I was initially surprised at how asymmetric the > and < cases are, but upon reflection it makes sense: if you're going forward, it's easy to go the minimum number of steps, whereas if you're going backward, you need to compensate.

spec.bs Outdated Show resolved Hide resolved
@domenic domenic mentioned this pull request May 25, 2021
@domenic domenic force-pushed the navigate-to-spec branch 2 times, most recently from c202c9a to 471dd99 Compare June 30, 2021 21:55
@domenic domenic mentioned this pull request Jul 16, 2021
This commit introduces a much more robust framework for tracking ongoing navigations. This fixes #130, for example, by appropriately keeping the signal around so that we can signal abort when necessary. It also ensures every same-document navigation ends in either navigationsuccess or navigationerror, with a few unified spec paths.

With this framework in hand, we can specify goTo()/back()/forward().
@domenic domenic changed the title Specify goTo()/back()/forward() Jul 23, 2021
@domenic
Copy link
Collaborator Author

domenic commented Jul 23, 2021

This has morphed into a pretty large overhaul of how state is tracked. I think it's almost done except the TODO to not fire navigatesuccess/navigateerror and resolve/reject the promise for canceled navigations. But in general it's much more robust.

@domenic domenic requested a review from natechapin July 26, 2021 23:42
Copy link
Collaborator

@natechapin natechapin left a comment

Choose a reason for hiding this comment

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

Looks good!


Each {{AppHistory}} object has an associated <dfn for="AppHistory">to-be-set serialized state</dfn>, a [=serialized state=] or null, initially null.

Each {{AppHistory}} object has an associated <dfn for="AppHistory">upcoming non-traverse navigation</dfn>, which is an [=app history API navigation=] or null, initially null.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a better name than what I had proposed.

@domenic domenic merged commit 02cb604 into main Jul 27, 2021
@domenic domenic deleted the navigate-to-spec branch July 27, 2021 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants