Navigation API: consolidate aborting code paths #11579
Description
What is the issue with the HTML Standard?
After #11409, we've end up with the following spec:
-
Navigate step 19 calls set the ongoing navigation which calls inform the navigation API about aborting navigation which calls abort the ongoing navigation.
-
Fire a push/replace/reload
navigateevent will, in the same-document case only, repeatedly loop and call abort the ongoing navigation.
Call sites:
- Fragment navigations only call "fire a push/replace/reload
navigateevent" pushState()only calls "fire a push/replace/reloadnavigateevent"- Cross-document navigations, including cross-document navigations which get intercepted and turned into same-document navigations by the navigation API, call "set the ongoing navigation". And then call "fire a push/replace/reload
navigateevent", but the relevant loop is skipped. - Removing an iframe directly calls "inform the navigation API about aborting navigation"
javascript:URL navigations only call "set the ongoing navigation". (I have no idea if this is good behavior, to be honest...)
Note on the Chromium implementation
We don't seem to have a direct counterpart of "set the ongoing navigation"; instead, we locate the relevant part of it inside a cross-document branch in "fire a push/replace/reload navigate event". For many cases this is equivalent to the current spec, but I'm not sure about in edge cases where no navigate event is fired. I suspect we properly inform the navigation API via some code path, just not necessarily an obvious one.
I think this all works, but it's a bit inelegant. In particular, although it might be the case that the repeated-aborting loop is only necessary for the fragment navigation + pushState() case, I think it'd be nicer to just consolidate it so that all call sites do the repetition.
My proposal is:
- We change "inform the navigation API about aborting navigation" to do the loop: "while ongoing
navigateevent is not null, abort the ongoing navigation". - We change "fire a push/replace/reload navigate event" to call "inform the navigation API about aborting navigation", without the isSameDocument check. If we're in the cross-document navigation case, this will mean "inform the navigation API about aborting navigation" is called twice (once via "set the ongoing navigation", and once via "fire a push/replace/reload
navigateevent"), but the second time will be a no-op.
This change should be just editorial, and @natechapin's work in https://chromium-review.googlesource.com/c/chromium/src/+/6622087 seems to be verifying this.
Thoughts? @natechapin @farre
(Note: don't send a PR for this until #10919 merges, so that @noamr doesn't have any more merge conflcits!!)