Navigables: fireNavigateEventOnCommit is unused in apply the history step

[ADKaster]

In https://html.spec.whatwg.org/multipage/browsing-the-web.html#apply-the-history-step, the argument fireNavigateEventOnCommit is not used.

Instead, it appears that the logic for whether to fire a traverse navigation event on commit happens in step 14.11.2, inside a queued global task:

If navigable is not traversable, and targetEntry is not navigable's current session history entry, and targetEntry's document state's origin is the same as navigable's current session history entry's document state's origin, then fire a traverse navigate event given targetEntry and userInvolvementForNavigateEvents.

---

A traverse navigation event is also fired from step 5 of apply the history step under a similar check in step 4.3 of the algorithm for checking if unloading is canceled. https://html.spec.whatwg.org/multipage/browsing-the-web.html#checking-if-unloading-is-canceled:

If targetEntry is not traversable's current session history entry, and targetEntry's document state's origin is the same as traversable's current session history entry's document state's origin, then:

... and step 4.3.5.4 of checking if unloading is canceled fires the actual event:

Let navigateEventResult be the result of firing a traverse navigate event at a Navigation navigation given targetEntry and userInvolvementForNavigateEvent.

[ADKaster]

Step 14.11.2 of apply the history step also doesn't say where to get the navigation API from, whereas 4.3.5.3 of check if unloading is canceled says to get it from the traversible's active window.

[domenic]

I believe the intent here is to only fire the "early" version of the navigate event (during "checking if unloading is canceled") under certain circumstances. However, I've lost track a bit of what those circumstances are supposed to be.

The current spec says those circumstances are:

• not a reload (targetEntry is not traversable's current session history entry) (this is kind of a weird thing to check?? Why disallow reloads but let through push/replace??)
• same origin (targetEntry's document state's origin is the same as traversable's current session history entry's document state's origin)

I think the intention of the spec was to add the condition

• is definitely a traverse, not a reload, push, or replace

by using a boolean passed in from the wrapper algorithms. Which would obviate the first condition above, which is why I'm a bit suspicious of it.

But then I'm wondering what the connection is between these conditions under which navigate is fired "early", and the conditions under which a traversal navigate is cancelable. Which have the additional conditions

• is same-document
• either userInvolvement is not "browser UI", or navigation's relevant global object has transient activation

@natechapin, can you help me remember which condition we implemented in Chromium for when the event fires "early"? I tried poking through the code but much of the context has fallen out of my head...

[natechapin]

I believe the intent here is to only fire the "early" version of the navigate event (during "checking if unloading is canceled") under certain circumstances. However, I've lost track a bit of what those circumstances are supposed to be.

The current spec says those circumstances are:

• not a reload (targetEntry is not traversable's current session history entry) (this is kind of a weird thing to check?? Why disallow reloads but let through push/replace??)
• same origin (targetEntry's document state's origin is the same as traversable's current session history entry's document state's origin)

I think the intention of the spec was to add the condition

• is definitely a traverse, not a reload, push, or replace

by using a boolean passed in from the wrapper algorithms. Which would obviate the first condition above, which is why I'm a bit suspicious of it.

But then I'm wondering what the connection is between these conditions under which navigate is fired "early", and the conditions under which a traversal navigate is cancelable. Which have the additional conditions

• is same-document
• either userInvolvement is not "browser UI", or navigation's relevant global object has transient activation

@natechapin, can you help me remember which condition we implemented in Chromium for when the event fires "early"? I tried poking through the code but much of the context has fallen out of my head...

We should only fire "early" for traversal afaik.

If I'm reading the spec correctly, there's some subtlety here. We only "check if unloading is canceled" in the step 5 of "apply the history step" if checkForCancelation is true, and it will only be true for reloads and traversals. Given that constraint plus the steps in "check if unloading is canceled", I think we get the correct behavior of only firing early for traversals?

[ADKaster]

In #9826 I walked through what happens for navigate to a fragment (including an assertion failure in the navigation API), which eventually ends up in apply the history step with that argument set to "false", because we already fired a navigate event in step 3 of that algorithm. I'm not sure if that's completely correct? It's hard to trace how fragment navigations flow through apply the history step for me.

[domenic]

Thanks @natechapin for confirming. So the desired state is:

• Only fire if same-origin
  • For top-level traversals, fire early
  • For non-top-level traversals, fire late
  • For reload/push/replace, don't fire in this algorithm at all; navigate should be fired earlier

What the spec currently implements:

• Top-level traversals are correct. checkForCancelation is only true for traversals and reloads, and the "targetEntry is not traversable's current session history entry" condition excludes reloads.
• Non-top-level traversals are correct. The only time we reach "If navigable is not traversable" is for reloads or traversals; push/replace will blow away all descendants. Reloads are excluded by "targetEntry is not navigable's current session history entry".

So I think the only issue is the leftover fireNavigateEventOnCommit. I will remove it with a clean conscience, and then move on to @ADKaster's other spec issues.