-
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
Tweak and expand navigation interception #26
Conversation
README.md
Outdated
[@slightlyoff](https://github.com/slightlyoff) | ||
for their help in exploring this space and providing feedback. | ||
|
||
## Appendix: types of navigations | ||
|
||
The web platform has many ways of initiating a navigation. For the purposes of this API, and in particular the [`navigate` event](#navigation-monitoring-and-interception), the following is intended to be a comprehensive list: |
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.
Could you clarify "this API" so that we know what "this" refers to? For example, I think this would read better: Below is a comprehensive list of ways that a navigation can be initiated, many of which are hooked into the appHistory
API and the navigate
event in particular.
README.md
Outdated
|
||
The web platform has many ways of initiating a navigation. For the purposes of this API, and in particular the [`navigate` event](#navigation-monitoring-and-interception), the following is intended to be a comprehensive list: | ||
|
||
- Users triggering navigations via browser UI, including (but not necessarily limited to): |
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.
Below are mechanisms, rather than actions. A user uses one of these to initiate a navigation. To maintain the current description, I'd expect the list to contain items like "change the text in the URL bar" (to initiate a navigation).
Or, describe these as "mechanisms that can be used to trigger a navigation" (no longer using active voice)
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.
The below are specific pieces of browser UI, so I think the phrasing is appropriate here? i.e. "... browser UI, including (but not necessarily limited to): 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.
Yeah, I see. Maybe just change "triggering" to "can trigger" then?
|
||
**Same-document** navigations are ones where, after the navigation completes, you stay on the same `Document`, with the same JavaScript environment. | ||
|
||
Most navigations are cross-document navigations. Same-document navigations can happen due to: |
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 can be confusing for many users is that these mechanisms assume they are cross-document, even if they wind up fetching the same document at runtime. Using the wrong mechanism leads to lower page performance and excessive refreshes because they are not integrated into the SPA. I think this section does a good job of describing the expectations, and the real goal for SPAs is to get as many clicks as possible into that beloved 3rd column: navigate
event.
README.md
Outdated
|Trigger|Cross- vs. same-document|Fires `navigate`?|`event.userInitiated`| | ||
|-------|------------------------|-----------------|---------------------| | ||
|Browser UI (fragment change only)|Always same|Yes|Yes| | ||
|Browser UI (other)|Always cross|No|—| |
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.
Does this include browser back? For entries that were previously pushed, I'd expect browser back to be equivalent to history.back()
. So long as the SPA is pushing correctly, back & forward inside the app's history entries should fire navigate
. (until the user goes back/forward out of the app's history space).
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.
That doesn't work though, because if you allow intercepting backs, even same-app-history ones, then the page can just trap the user forever on their origin, never letting them actually get back to where they were.
If you want to see backs, you can use currententrychange
. But since we cannot allow them to be interceptable for anti-abuse reasons, we don't fire navigate
for them.
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.
Is it worth adding another column for currententrychange
? I don't want this nuance to be lost
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 should try to incorporate it somehow (beyond what's already mentioned in the main section: "These restrictions are designed so that canceling..."). But a dedicated column isn't great, because its values are basically the same as the same- vs. cross-document column. That is, cross-document navigations don't fire currententrychange
(since by the time the entry has changed, the document is gone, and no event listeners fire on the old document). And same-document navigations always do.
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, the back/capture part is clear in the other section, I just rabbit-holed on what this table says on its own.
I think https://github.com/WICG/app-history#current-entry-change-monitoring is also descriptive enough to understand when I'd expect currententrychange
, even if it's not listed in table format. But just to make sure I do understand... If I do a cross-document navigation from A.foo/ to B.foo/ via a URL-bar edit, then the A document does not hear navigate
nor currententrychange
, but the B document does hear a currententrychange
. Is that correct?
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 discussion made me realize that we can't intercept the back button even for fragment changes, so I pushed some more changes in that direction...
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.
yes, that's a good clarification. I had assumed you were only describing editing the fragment in the URL-bar (a forward fragment change)
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.
But just to make sure I do understand... If I do a cross-document navigation from A.foo/ to B.foo/ via a URL-bar edit, then the A document does not hear navigate nor currententrychange,
Correct
but the B document does hear a currententrychange. Is that correct?
Well... yes, as currently written, but I'm not sure that's a good idea. See the TODO in #27. In particular, this would be the only time currententrychange
fires for a cross-document navigation. And it'd be basically redundant with the load
event. It's a weird special case, and I suspect firing it might hurt people more than it helps. Probably this should get its own dedicated issue...
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.
Started an issue thread: #30 .
The normative change here is to resolve "TODO: should the event even fire at all, for these latter two cases?" in the negative: i.e., the navigate event will not fire at all for non-interceptable cases. Additionally, this makes same-document fragment navigations, even initiated via browser UI or window.open(), interceptable. Beyond that, this commit: * Expands the discussion of navigation interception to be clearer about what "user-initiated" means * Adds an appendix detailing all the navigations on the platform, the distinction between cross-document and same-document (closes #3), and a summary table listing how this spec works for each type. * Discusses the connection between the navigations we're considering, and the HTML Standard algorithms.
4d2cf53
to
91be8da
Compare
@natechapin I'm now wondering about the case of
Let's say the code in Preventing such navigation is not important for anti-abuse reasons (since it's not the user initiating the back). However, I don't believe it's something you could do today. So adding such interception would be expanding capabilities. Do you have any thoughts? In general (see #32), web developers are pretty sad about any restrictions we place on interception. A natural followup is that |
chromium doesn't know which frame(s) will be navigated by
Agreed that this is similar. |
After some offline discussion with @natechapin, I've updated this PR to allow navigation interception for cases where the navigation is initiated by code in another frame. This goes beyond what is possible to intercept today, but does not have abuse implications, so in theory it should be OK. We may need to scale back this ambition if we find it too hard to implement. |
The normative change here is to resolve "TODO: should the event even fire at all, for these latter two cases?" in the negative: i.e., the navigate event will not fire at all for non-interceptable cases. Additionally, this makes same-document fragment navigations, even initiated via browser UI or window.open(), interceptable.
Beyond that, this commit:
/cc @matt-buland-sfdc and @natechapin; I'll probably let this sit for a day or two before merging in case you have any comments.