Navigation API: deferred commit

[noamr]

This adds navigateEvent.prototype.intercept({precommitHandler}).

Instead of always "committing" the navigation immediately, the navigation only commits after all its precommitHandler promises are resolved.

In addition, a precommitHandler receives a NavigationPrecommitController as an argument, and can use it to change the URL of the ongoing navigation before it is commited, using the redirect(newURL) function.

See https://github.com/WICG/navigation-api/blob/main/README.md#precommit-handlers

• [ ] At least two implementers are interested (and none opposed):
  • Chromium: implementing
  • WebKit: https://github.com/WebKit/standards-positions/issues/449
  • Gecko: https://github.com/mozilla/standards-positions/issues/1169#issuecomment-2766983430
• [ ] Tests are written and can be reviewed and commented upon at:
  • …
• [x] Implementation bugs are filed:
  • Chromium: mostly done, opened https://issues.chromium.org/issues/422067785 for the last bit.
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1970123
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=293952
• [x] Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A, no HTML elements involved in this
• [x] MDN issue is filed: Will be done as part of Chromium beta launch
• [x] The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

/browsing-the-web.html ( diff ) /index.html ( diff ) /nav-history-apis.html ( diff )

[noamr]

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.

Fixed

[noamr]

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 :(

Spec-wise, does this mean more than failing when trying to set commit: after-transition on a non-cancelable nav?

[domenic]

Yes. In particular, per my bulleted list above, in the success case currently no traversal ever happens. It should happen.

[noamr]

Yes. In particular, per my bulleted list above, in the success case currently no traversal ever happens. It should happen.

Hmm but if event.intercept({commit: 'after-transition') throws when event.cancelable is false (or ignores after-transition, but that's perhaps too silent), step 32 would commit because the navigation would not have been intercepted, step 37 would return true, and the whole traversal would be committed. I ran through the algorithm steps and your bullet points a few times and it checks out... Not sure what I'm missing.

[domenic]

The case I'm concerned about is a traversal with event.cancelable = true, and the code

event.intercept({
  commit: 'after-transition',
  handler() { return delay(1000); }
})

In this case, step 32 will do nothing, because event's commit behavior is "after-transition".

Step 36 will not execute, since event's interception state is "intercepted".

Step 37 will return false, so we cancel the traversal (in https://html.spec.whatwg.org/#preventing-navigation:fire-a-traverse-navigate-event).

Step 32.4.success should eventually re-do the traverse somehow, but it does not.

---

A similarly broken case is a traversal with event.cancelable = true, and the code

event.intercept({
  commit: 'after-transition',
  async handler() {
    await delay(1000);
    event.commit();
  }
})

This will similarly do nothing in step 32 and step 37, and return false in step 37, sot he traverse is canceled. Then, calling event.commit() will enter the "commit" algorithm, which will do nothing substantial, since the important step 9 is guarded by a "push" or "replace" check. Instead, it needs to re-do the traverse somehow.

[noamr]

I tried a few things in chrome, it seems like:

• we do the right thing, as in update the actual history entry on commit.
• We do fire popstate (it's probably the right thing to do, in case some existing code relies on those classic events).

So I think the right thing to do here is to run the URL and history update steps 7-11, and also call popstate. Coincidentally, this is done for fragment navigation: https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate-fragid steps 12-14.

So I think the plan of action would be to have a few steps equivalent to those fragment-navigation steps, extracting the history length/index from the appropriate places. WDYT @domenic ?

[domenic]

That seems like the right start, but it's not sufficient, right? We also need to update the "browser process" view, and update any iframes. For push/replace, the URL and history update steps 12-13 do that, but the algorithm finalize a same-document navigation strongly assumes that it's only being used for push/replace, so it isn't quite usable for our purposes.

I think this time we need to do at least some of "apply the history step", in order to:

• cancel any ongoing navigations
• set the "current session history entry"
• traverse any subframes (and fire navigate events on them, and update their history index)
• set the "current session history step"

However, it's a bit tricky since we've already done "apply the history step" once; we just bailed out in step 5 ("check if unloading is canceled") after firing the traverse navigate event for the top frame.

I think we can just do this by adding an extra boolean argument to "apply the history step" which causes it to skip step 5?

[noamr]

That seems like the right start, but it's not sufficient, right? We also need to update the "browser process" view, and update any iframes. For push/replace, the URL and history update steps 12-13 do that, but the algorithm finalize a same-document navigation strongly assumes that it's only being used for push/replace, so it isn't quite usable for our purposes.

I think this time we need to do at least some of "apply the history step", in order to:

• cancel any ongoing navigations
• set the "current session history entry"
• traverse any subframes (and fire navigate events on them, and update their history index)
• set the "current session history step"

However, it's a bit tricky since we've already done "apply the history step" once; we just bailed out in step 5 ("check if unloading is canceled") after firing the traverse navigate event for the top frame.

I think we can just do this by adding an extra boolean argument to "apply the history step" which causes it to skip step 5?

Isn't the existing checkForCancelation this boolean already? Can we simply queue apply the history step with that boolean set to false?

[domenic]

Yes, it totally is; not sure how I missed that.

[noamr]

This still seems like it's missing a lot of the hard parts discussed in [#10919 (review)] (https://github.com/whatwg/html/pull/10919#pullrequestreview-2571653909) and following comments:

• No throwing for precommitHandlers on non-cancelable navigations
• For traverses, doesn't properly cancel and then restart the traverse upon commit.

Oops, I had these in a previous revision and forgot to port them when I shuffled things around. I think it's good now.

[domenic]

This is looking very, very close. I pushed some final nits, mostly around aesthetic preferences like putting precommit before commit and committed before finished.

Final things:

• redirect() should probably throw if history is not "push" or "replace". Right now it silently ignores other values. I guess... TypeError? Since that's what we throw for invalid enums generally?
• There's a merge conflict. I tried to fix it locally but it looked nontrivial :-/.

[noamr]

This is looking very, very close. I pushed some final nits, mostly around aesthetic preferences like putting precommit before commit and committed before finished.

Final things:

• redirect() should probably throw if history is not "push" or "replace". Right now it silently ignores other values. I guess... TypeError? Since that's what we throw for invalid enums generally?

Hmm it doesn't? It throws an InvalidStateError in step 4.

• There's a merge conflict. I tried to fix it locally but it looked nontrivial :-/.

Fixed. It was a conflict with https://github.com/whatwg/html/pull/11203.

[atscott]

There's a lot of duplication in the steps for "Let cancel be the following steps given reason:" and "To abort the ongoing navigation given a Navigation navigation and an optional DOMException error:". Is there a way to consolidate those?

In my implementation of the fake, after creating the abort reason in "To abort the ongoing navigation given a Navigation...", I call the cancel steps on the NavigateEvent directly since the steps appear to be the same from that point on. I don't know if that translates to what browsers can do though

[noamr]

There's a lot of duplication in the steps for "Let cancel be the following steps given reason:" and "To abort the ongoing navigation given a Navigation navigation and an optional DOMException error:". Is there a way to consolidate those?

In my implementation of the fake, after creating the abort reason in "To abort the ongoing navigation given a Navigation...", I call the cancel steps on the NavigateEvent directly since the steps appear to be the same from that point on. I don't know if that translates to what browsers can do though

Sorry for the late response on this, I've consolidated the common steps into an "Abort the NavigateEvent" algorithm.

[noamr]

Small nit, and I'm sorry but we need another merge conflict fix with #11512 . (And probably a check that the new stuff or rearranged stuff here does not regress the fix introduced in that PR?)

Github was barfing at my rebasing/merging foo so I created a new PR: https://github.com/whatwg/html/pull/11580