-
Notifications
You must be signed in to change notification settings - Fork 26
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
Conversation
Per #110 I don't think Edit: per further discussions there I now realize it will. |
I'll block this on whatwg/html#5666 |
What would you expect
How I see traversal working with whatwg/html#6315:
There are a few ways this can 'fail', and I'm not sure what you'd like to do for each of these cases:
Getting the previous step given the current 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 Getting the next step given the current navigable:
Getting the
|
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.
I think we should do both. If the synchronously-accessible
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.
Reject promise with "SecurityError" DOMException.
Reject promise with an "AbortError" DOMException.
Reject promise with an "AbortError" DOMException.
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?
Definitely a success. |
That makes as much sense as anything. Ugh, I just gave some of this a quick test:
I'm sure there are more subtleties, and I only tested Chrome. Pretty sure this won't be interoperable.
I can't tempt you to make But yeah, this won't be hard to spec, as the hooks are already there for history length and index.
It was a good test of the central "go here" algorithm!
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/ |
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. |
There was a problem hiding this comment.
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".
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. |
Some easy questions:
Is this pointer defined anywhere in the draft?
By this you mean navigable, not traversable? Trickier question:
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,
I'm not sure the filtering is necessary since we already filtered the list of possible keys? |
Should this be current or active? I think current? |
I'm realizing that I'm going to need an updated version of "traverse the history by a delta" in order to specify Would you be able to prioritize
? I gave it a shot myself, but I'm unsure about the details of translating a delta into a step. |
There's
Yes. It has to be the navigable, since app history back/forward is specific to the navigable.
I think it might be more complicated. For example:
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
Hmm, I'm not sure. The
I'm still not 100% sure what to do here. But yeah, let's go for current for now.
Yeah, shall do |
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: If we're on step 4, and want to go However, when we go For example: If we're on step 2, and go 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 Let's say we're on step 3, but we're still showing
This isn't so clear with
I think the latter is more consistent with what app-history is trying to do. Maybe this also means that |
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:
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.
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:
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
This would be ideal, I agree. |
Taking another swing at it: Getting the
I might be getting confused. Here's where my mind's at: 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
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 When going from step 6 to key
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.
Yeah, that looks right to me. I'd need to do some tests of reload to be sure. |
Agreed. However, We do need to check that the key we're given is in (the spec equivalent of) 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. |
I don't see a definition for navigable's BCG in https://whatpr.org/html/6315/history.html#navigables ... should there be one? |
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:
I think you're assuming this always exists because you're handling three cases:
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. |
c7c8beb
to
723d6fd
Compare
c202c9a
to
471dd99
Compare
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().
471dd99
to
6ee58b9
Compare
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. |
This reverts commit 70b2fe0.
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
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