-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Navigation API: deferred commit #10919
base: main
Are you sure you want to change the base?
Conversation
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 so much for working on this!!!
This is missing the behavior of auto-committing when all finished promises fulfill. See https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/navigation_api/navigate_event.cc;l=338;drc=3b5e9130048e66ce39338d34f99751d0a2346e9c for the counterpart Chromium code.
This adds `navigateEvent.prototype.intercept({commit: "after-transition"})`, as well as `NavigateEvent.prototype.commit()`. Instead of always "committing" the navigation immediately, we refer to this interception option. If it's the default ("immediate"), the existing behavior is invoked. The new "after-transition" behavior keeps the navigation un-committed, until `NavigateEvent.prototype.commit()` is called. This follows roughly the same semantics of `scroll`. See https://github.com/WICG/navigation-api/blob/main/README.md#deferred-commit
Fixed |
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.
LGTM with nits. So exciting!!
data-x="dom-Navigation-currentEntry">navigation.currentEntry</code> property, and update the | ||
browser UI, such as the location bar.</p> | ||
|
||
<p>The <code data-x="dom-NavigationInterceptOptions-commit">commit</code> option can be set 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.
Nit: merge this with the previous paragraph.
<code>DOMException</code> if called on a <code>NavigateEvent</code> that has already been | ||
committed due to the default "<code | ||
data-x="dom-NavigationCommitBehavior-immediate">immediate</code>" commit behavior, if called | ||
more than once for the same navigation, or if called synchronously, during event dispatch, or |
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.
more than once for the same navigation, or if called synchronously, during event dispatch, or | |
more than once for the same navigation, if called synchronously during event dispatch, or if called |
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, but am not sure, that delayed commit is broken for traverses. So marking as "request changes" until we figure that out.
The intention is to allow delaying traverses whenever the event is cancelable (which, for traverses, means same-document traverses of top-level traversables with some anti-abuse protection from history-action activation).
However, from what I can tell, the proposed spec is broken in this regard:
- They allow delaying traverses at all times. That is, there is nothing that gives an error when attempting to set
commit: "after-transition"
when the event is non-cancelable. - The "inner navigate event firing algorithm" will return false if you call
navigateEvent.intercept()
on a traverse, due to falling back to the final step 37. - This causes "check if unloading is canceled" to prevent the unloading, and thus the traversal to never happen.
- Later, when the developer calls
commit()
, nothing happens.
In Chromium, I'm not sure if it's working or not. The flow is similar to the spec, except calling commit()
ends up performing the URL and history update steps, because of this line.
We have tests for this. https://wpt.live/navigation-api/commit-behavior/after-transition-traverse.html in particular. It passes, in Chromium, but note that it's failing to test the actual position of navigation.currentEntry
in the entry index. It only tests the URL (via location.hash
). So it might be doing a push (or replace?) navigation instead of a traverse.
Or, it's possible that Chromium's RunURLAndHistoryUpdateSteps()
is doing a sort of traverse. Note that it does take a destination_item
.
This is related to #10621, which claims that Chromium is also using the RunURLAndHistoryUpdateSteps()
to do observable work for the reload case, despite the spec omitting them.
If Chromium is behaving correctly, then the right path to fixing both this issue and #10621 might be adding something to "commit" which handles the reload and traverse cases. It seems like it would be some subset of "apply the history step".
Sorry this got complicated :(
This adds
navigateEvent.prototype.intercept({commit: "after-transition"})
, as well asNavigateEvent.prototype.commit()
.Instead of always "committing" the navigation immediately, we refer to this interception option. If it's the default ("immediate"), the existing behavior is invoked.
The new "after-transition" behavior keeps the navigation un-committed, until
NavigateEvent.prototype.commit()
is called.This follows roughly the same semantics of
scroll
.See https://github.com/WICG/navigation-api/blob/main/README.md#deferred-commit
(See WHATWG Working Mode: Changes for more details.)
/nav-history-apis.html ( diff )