-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add the navigation API #8502
Add the navigation API #8502
Conversation
99f4e40
to
4dbf993
Compare
68f4303
to
836044a
Compare
This imports much of the specification from https://wicg.github.io/navigation-api/. It includes some minor fixes and renamings, as well as a lot of rearranging to flow better, but the feature set is the same.
836044a
to
fe5bbc4
Compare
Aligns with latest work in https://chromium-review.googlesource.com/c/chromium/src/+/4262131/10
The specification work has moved to whatwg/html#8502. The TypeScript definitions have moved to DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/dom-navigation/index.d.ts.
The specification work has moved to whatwg/html#8502. The TypeScript definitions have moved to DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/dom-navigation/index.d.ts.
The specification work has moved to whatwg/html#8502. The TypeScript definitions have moved to DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/dom-navigation/index.d.ts.
Just want to leave a comment that the timing of |
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.
Just nits really. This is a massive rewrite, really impressive seeing it all laid out!
source
Outdated
@@ -88583,8 +88705,3221 @@ interface <dfn interface>History</dfn> { | |||
</div> | |||
|
|||
|
|||
<h4 id="navigation-api">The navigation API</h4> |
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 other section titles in this spec page are written like "The History
interface". Any reason this title deviates?
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 section contains more than just the Navigation
interface, as it's the section header for all of the navigation API. IN that way it's more like the section titles elsewhere of "Dynamic markup insertion", "User prompts", "Server-sent events", "Cross-document messaging", etc. The addition of "API" is a bit unusual, but it's necessary because of the generic term we've chosen: throughout the spec we need to distinguish "navigation" and "the navigation API", whereas we don't need to distinguish "WebSockets" and "the WebSockets API".
<p>If <span>this</span>'s <span>upcoming non-traverse API method tracker</span> is | ||
<var>apiMethodTracker</var>, then:</p> | ||
|
||
<p class="note">This means the <span>navigate</span> algorithm bailed out before ever getting 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.
I think this note could be just a little clearer.
"If upcoming non-traverse API method tracker is still apiMethodTracker after the the navigate algorithm, it means that the navigate algorithm did not successfully promote the upcoming non-traverse API method tracker to the ongoing method tracker."
I also wonder why the navigate algorithm does not return an error code which would make the algorithm clearer, I think?
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.
Thanks; I tweaked the note similar to what you suggested.
As for why not modifying the navigate algorithm, it seems cleaner to keep the navigation API-specific stuff a bit separated, especially since it's important only for this one call site. Navigate is called by many different parts of the spec, and having a return value that 90% of call sites ignore seems a bit unfortunate.
<dt><code data-x=""><var>event</var>.<span subdfn data-x="dom-NavigationCurrentEntryChangeEvent-navigationType">navigationType</span></code></dt> | ||
<dd><p>Returns the type of navigation which caused the current entry to change, or null if the | ||
change is due to <code | ||
data-x="dom-Navigation-updateCurrentEntry">navigation.updateCurrentEntry()</code>.</p></dd> |
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.
It's a bit odd that the navigationType is null for updateState(). Are we sure we don't want to throw in an extra navigation type for that?
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 since it's very much not a navigation, it's best to have it be null, to signal the exceptional nature of what's going on.
Found while doing a code refactoring in https://chromium-review.googlesource.com/c/chromium/src/+/4369772 #2621 is related, but since we don't have an overall fix yet, let's patch it here
document">focused area</span> to <var>document</var>'s <span>viewport</span>, and set | ||
<var>document</var>'s <span>relevant global object</span>'s <span | ||
data-x="window-navigation-api">navigation API</span>'s <span>focus changed during ongoing | ||
navigation</span> to false.</p> |
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 surprising. Why would removing a node clear such flag.
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.
So how focusReset: "after-transition"
is supposed to work is as follows:
- If nothing relevant changes in the document, focus gets reset after the transition
- If the user (or developer, but the user is more important) changes focus during the transition, we don't reset the focus: that would be too disruptive.
- But, if the user or developer changes focus during the transition, and then the developer removes the currently-focused DOM node from the document, we do reset the focus.
Without this step you're commenting on, (3) will be missing.
Note here "reset the focus" means the slightly-special behavior in https://whatpr.org/html/8502/nav-history-apis.html#potentially-reset-the-focus , of using the autofocus behavior and also resetting the sequential focus navigation starting point. So it's more than just the focus fixup steps.
What do you think?
source
Outdated
<dt><dfn export data-x="history-action activation-consuming API" data-lt="history-action activation-consuming API">History-action activation-consuming APIs</dfn></dt> | ||
<dd><p>These APIs require the <span>history-action activation</span> state to be true, and they | ||
<span>consume history-action user activation</span> in each call to prevent multiple calls per | ||
user activation.</p></dd> |
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.
It is unclear why this is needed. (but I'm still reading all this, so perhaps something somewhere clarifies the issue this is trying to solve.)
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 added to prevent back-trapping. See "traversals have special restrictions on canceling the navigation" in https://github.com/wicg/navigation-api#restrictions-on-firing-canceling-and-responding . It is also envisioned to be used by https://github.com/WICG/close-watcher .
source
Outdated
return true.</p></li> | ||
|
||
<li><p>If <var>document</var>'s <span data-x="concept-document-origin">origin</span> is <span | ||
data-x="concept-origin-opaque">opaque</span>, then return true.</p></li> |
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 looks surprising. One couldn't use the API for top level opaque documents for example.
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. We haven't run into people using top-level opaque documents who complained, but it's something we could loosen up in the future if you are in contact with some developers who would like to use this API, and can't.
<li><p>If <var>navigation</var>'s <span>upcoming traverse API method | ||
trackers</span>[<var>key</var>] <span data-x="map exists">exists</span>, then return a | ||
<span>navigation API method tracker-derived result</span> for <var>navigation</var>'s | ||
<span>upcoming traverse API method trackers</span>[<var>key</var>].</p></li> |
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.
Isn't this surpising? if the key is already there, reuse that even if the map might have gotten other entries after the key was added.
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 don't understand your second sentence, or what's surprising about this behavior. Could you say more?
To be clear, this is about cases like:
const key = navigation.entries()[navigation.currentEntry.index - 1];
const result1 = navigation.traverseTo(key);
// ... probably in some other part of the app ...
const result2 = navigation.traverseTo(key);
and says that result2
should just reflect what's going on with result1
(i.e. have the same promises). I'm not sure what other behavior you'd envision here...
Closes #254. This is already reflected in the specification at whatwg/html#8502.
Mozilla is positive. |
Thanks so much Simon! Given that we've gotten review on this from multiple sources already, and all the checkboxes in the OP are filled out, I'll plan to merge this Monday (Japan time). Of course, if more reviews come in, I'm happy to delay. |
This imports much of the specification from https://github.com/WICG/navigation-api/blob/91c2e7f959418d71e26e5b9a6723fed2944001d9/spec.bs. It includes some fixes and renamings, as well as a lot of rearranging to flow better, but the feature set is the same.
In addition to the new "navigation API" section under "APIs related to navigation and session history", this makes the following changes to integrate the navigation API:
Introduces the NavigationHistoryBehavior enum, as a superset of the "history handling" specification type. This includes an "auto" value to reflect that existing entry points will sometimes default to "replace" instead of "push". The navigation API allows overriding that auto behavior in some circumstances by explicitly supplying "push". (But in some cases, i.e., the initial
about:blank
orjavascript:
URLs, this is still not allowed.)Introduces the concept of "user navigation involvement". This will be helpful for solving issues such as Sec-Fetch-User and Sec-Fetch-Site for user-initiated UI navigations w3c/webappsec-fetch-metadata#71.
Introduces the concept of "history-action user activation", which is a separate type of user activation meant to be used specifically to prevent back button tracking. It has no associated timeout (unlike transient activation), but can be flipped to false (unlike sticky activation). It is used by the navigation API to prevent calling
navigateEvent.preventDefault()
on two traversals in a row without intervening user activation. It is likely also to be useful for issues such as Specify back button history-entry skipping? #7832.The activation behavior for
<a>
and<area>
elements is made more rigorous and consolidated into one place, so that we can hook into it appropriately to firenavigate
events.Some surgery was needed on "apply the history step". The result is that it now has slightly more parameters, but also several wrapper algorithms which take care of setting up those parameters correctly for the cases of: navigable creation/destruction, push/replace, reload, and traverse. It also returns a value indicating if the application of the history step was canceled, and if so, how; this is used to give informative errors as return values of
navigation.traverseTo()
.The "check if unloading is user-canceled" algorithm has become "check if unloading is canceled", as it now handles firing a possibly-cancelable
navigate
event at the top-level traversable.Other changes scattered throughout are mostly integrating appropriate calls to the navigation API as necessary, especially to fire
navigate
events.(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/browsing-the-web.html ( diff )
/document-lifecycle.html ( diff )
/document-sequences.html ( diff )
/form-control-infrastructure.html ( diff )
/form-elements.html ( diff )
/forms.html ( diff )
/iframe-embed-object.html ( diff )
/image-maps.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/interaction.html ( diff )
/links.html ( diff )
/nav-history-apis.html ( diff )
/parsing.html ( diff )
/popover.html ( diff )
/semantics.html ( diff )
/server-sent-events.html ( diff )
/text-level-semantics.html ( diff )
/webappapis.html ( diff )