-
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
Revamp when navigate fires, is cancelable, and is respond-able #56
Conversation
* 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.
// Don't intercept cross-origin navigations; let the browser handle those normally. | ||
if (!e.sameOrigin) { | ||
// Some navigations, e.g. cross-origin navigations, we cannot intercept. Let the browser handle those normally. | ||
if (!e.canRespond) { |
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.
I had to think for a while before remembering that you can still cancel these navigations, even if you can't respond to them. Maybe an example? Otherwise some people may wonder why this is even needed
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.
https://github.com/WICG/app-history#example-single-page-app-redirects is kind of that example. I split the examples so the most central ones were first and the less central ones were after the explanatory subsections, but maybe that was mistake.
README.md
Outdated
} | ||
|
||
const isBackForward = appHistory.entries.includes(event.destination); | ||
let { key } = event.destination; |
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.
hypernit, let {key}
is more common in the JS code I see, but maybe this is more common in specs?
README.md
Outdated
- User-initiated [cross-document](#appendix-types-of-navigations) navigations via browser UI, such as the URL bar, back/forward button, or bookmarks. | ||
- [`document.open()`](https://developer.mozilla.org/en-US/docs/Web/API/Document/open), which can strip off the fragment from the current document's URL. | ||
|
||
User-initiated navigations of this sort are outside the scope of the webpage, and can never be intercepted or prevented, even if they are to same-origin documents. On the other hand, we do allow the page to intercept user-initiated _same_-document navigations via browser UI, e.g. if the user changes the fragment component in the URL bar. |
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 "even if they are to same-origin documents" only refers to document.open()
right? If so, move it into that bullet, otherwise it's confusing because it looks like it's also possible in bullet 1
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.
No, this is meant to cover cases like:
- You are on
https://example.com/foo
- The user types
https://example.com/bar
in the URL bar
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.
ah okay I got confused by same-origin same-document again :)
README.md
Outdated
- User-initiated [cross-document](#appendix-types-of-navigations) navigations via browser UI, such as the URL bar, back/forward button, or bookmarks. | ||
- [`document.open()`](https://developer.mozilla.org/en-US/docs/Web/API/Document/open), which can strip off the fragment from the current document's URL. | ||
|
||
User-initiated navigations of this sort are outside the scope of the webpage, and can never be intercepted or prevented, even if they are to same-origin documents. On the other hand, we do allow the page to intercept user-initiated _same_-document navigations via browser UI, e.g. if the user changes the fragment component in the URL bar. |
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.
What about clicking a cross-document link?
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.
Yeah, that's fine and interceptable. I guess I messed up by abbreviating "User-initiated cross-document navigations via browser UI" to "User-initiated navigations of this sort". This is meant to be only about the first bullet.
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.
small clarification
|
||
First, the following navigations **will not fire `navigate`** at all: | ||
|
||
- User-initiated [cross-document](#appendix-types-of-navigations) navigations via browser UI, such as the URL bar, back/forward button, or bookmarks. |
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.
I may be reading this wrong, but you list user-initiated cross-document navigations via... back/forward button
as something that will never fire navigate
here.
yet below on line 372 it says user-initiated navigations (cross- or same-document) via browser's back/foward buttons
which implies that navigate is fired (else there would be no need to mention event.preventDefault()
?)
also the table appears to list browser back/forward as firing navigate
.
am I confused or are these at odds?
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.
Good catch. Listing cross-document browser back/forward navigations as non-cancelable is redundant since they won't fire navigate at all. Fixed.
As for the table, it says "Yes †*" where - † = No if cross-document (and * = No if the URL differs in components besides path/fragment/query). I struggled with these "Yes" table entries; most of them are "Yes except in specific rarer circumstances", but that cell has more serious exceptions. Any suggestions are welcome.
README.md
Outdated
|Browser UI (non-back/forward other)|Always cross|No|—|—|—| | ||
|`<a>`/`<area>`|Either|Yes|Yes ‡|Yes|Yes *| | ||
|`<form>`|Either|Yes|Yes ‡|Yes|Yes *| | ||
|`<meta http-equiv="refresh">`|Either|Yes|No|Yes|Yes *| |
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.
Drive-by nitpick: meta Refresh and the Refresh header will always be cross-document, since they're effectively a reload. Same with form submission.
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.
Not true in Firefox, it turns out! https://boom-bath.glitch.me/meta-refresh.html fun times.
Sometimes it's desirable to handle back/forward navigations specially, e.g. reusing cached views by transitioning them onto the screen. This can be done by branching as follows: | ||
|
||
```js | ||
appHistory.addEventListener("navigate", e => { |
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.
I think you need async here?
appHistory.addEventListener("navigate", e => { | |
appHistory.addEventListener("navigate", async (e) => { |
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.
Oh, the problem here is I need to wrap this all in respondWith.
@natechapin and @tbondwilkinson, I'd love your review of these changes.