w3c / navigation-timing

Navigation Timing
https://w3c.github.io/navigation-timing/
Other
116 stars 30 forks source link

Definition of "back_forward" navigation type does not make sense #115

Closed bzbarsky closed 4 years ago

bzbarsky commented 5 years ago

https://w3c.github.io/navigation-timing/#dom-navigationtype says:

back_forward Navigation through a history traversal operation.

where "history traversal" links to https://html.spec.whatwg.org/multipage/browsing-the-web.html#traverse-the-history

But every navigation passes through "traverse the history". For example, a link click goes through https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate which lands in https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch which lands in https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-response which lands in https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate-html (say this is an HTML document) which then ends up in https://html.spec.whatwg.org/multipage/browsing-the-web.html#update-the-session-history-with-the-new-page which lands in the "Otherwise" case of step 2 and does "traverse the history" in substep 3.

What you probably want to say here is that the type should be "back_forward" if https://html.spec.whatwg.org/multipage/history.html#traverse-the-history-by-a-delta was involved. Maybe. What happens if the history load ends up in a 3xx redirect? Should that still count as a "history navigation" at that point?

It's probably worth clearly spelling out the desired behavior here and then discussing with the HTML spec editors how to best spec it.

bzbarsky commented 5 years ago

Note that if instead of a 3xx HTTP redirect there is a location.href set early in the document that came from history that does a replace load, that would presumably not count as a "back_forward" navigation type. Or should it?

yoavweiss commented 5 years ago

From a use case perspective, it feels like redirects to new pages should not be considered "back_forward". At the same time, we probably want to align with ServiceWorkers on that front.

yoavweiss commented 5 years ago

Discussing this some more at the F2F, it actually doesn't really make sense to have a navigation type that's "back_forward" which only represents history navigations that did not go through the BFCache. Seems like we can remove this type altogether.

bzbarsky commented 5 years ago

Why would it necessarily be only ones "that did not go through the bfcache"?

nicjansma commented 5 years ago

We use the classification of back_forward for a couple reasons in our analytics scripts. In some cases, we don't send beacons at all (i.e. we only beacon for navigation and reload).

In other cases, we will avoid incrementing a metric/counter for conversion pages. One example of that is if a site designates a Checkout page as a "conversion", we don't want to double-count the conversion if they hit Back.

igrigorik commented 5 years ago

@nicjansma why would the page double count the conversion? The script is not re-executed, conceptually it's the same as freezing and then resuming the tab, or perhaps a better analogy, backgrounding (sans any execution) and bringing it back.

nicjansma commented 5 years ago

Maybe I've misunderstood, but performance.navigation.TYPE_BACK_FORWARD are for cases that don't go through BFCache, and thus are "full" navigations (that rebuild the HTML and execute all script), right?

igrigorik commented 5 years ago

Ah, therein is the confusion.. I believe it's the opposite.

In the current spec, we just define it as tied to history traversal, but that's not terribly helpful — e.g. SPA nav can trigger a history traversal and we don't re-trigger nav timing, reset time origin, etc. I think the conclusion we came to in the room was that it just makes sense to drop this value from the enum entirely, for these reasons. That said, paging @yoavweiss for sanity check.

nicjansma commented 5 years ago

Hm, that's not what I see in practice.

philipwalton commented 5 years ago

@nicjansma is correct, pages that come out of the BFCache do not update their navigation timing entries or emit new ones. (I tested this in Firefox during the meeting.)

My opinion is I think it's fine to keep these entries, since there may be cases where analytics vendors want to know if the navigation was the result of a user hitting the back button (for similar reasons that they might want to know if the navigation was a reload).

For back_forward "loads" that come out of the BFCache, analytics vendors can already detect that via the pageshow event (if event.persisted is true).

philipwalton commented 5 years ago

To address @bzbarsky's concern specifically:

What you probably want to say here is that the type should be "back_forward" if https://html.spec.whatwg.org/multipage/history.html#traverse-the-history-by-a-delta was involved.

That sounds right to me. I don't think it should matter how the page was actually loaded, I think what matters (at least from a measurement perspective) is the user interaction that triggered the page load.

What happens if the history load ends up in a 3xx redirect? Should that still count as a "history navigation" at that point?

In my opinion, yes. Because it was triggered by a user clicking the back/forward button.

Note that if instead of a 3xx HTTP redirect there is a location.href set early in the document that came from history that does a replace load, that would presumably not count as a "back_forward" navigation type. Or should it?

I don't think script redirects should retain their original navigation type.

bzbarsky commented 5 years ago

pages that come out of the BFCache do not update their navigation timing entries or emit new ones. (I tested this in Firefox during the meeting.)

Which Firefox version did you test? The behavior here changed a few months ago; see https://bugzilla.mozilla.org/show_bug.cgi?id=1459711. Release Firefox has the old behavior, but beta and nightly have the new one, which does change the navigation.type when coming out of bfcache. That new behavior will ship to release in mid-October.

For the rest, sounds like we are on the same page, and I think the new Firefox behavior implements more or less what https://github.com/w3c/navigation-timing/issues/115#issuecomment-532603678 suggests...

philipwalton commented 5 years ago

Hmmm, that's interesting. I would not have expected that behavior (e.g. existing entries being modified and the value of performance.timeOrigin being updated).

I do see value in exposing BFCache load timing data, but I wonder if it makes more sense to do so in a separate performance entry?

One concern I have with this new behavior in Firefox is what effect it might have on scripts that have a local reference to performance.timeOrigin. For example, my personal website includes this small polyfill for performance.now() and performance.timeOrigin, which I guess will now be buggy in future versions of Firefox (since the polyfill stores a local reference to performance.timeOrigin at page load time).

rniwa commented 5 years ago

Safari's behavior appears to come from this code change which resets navigation timing upon page cache navigation: https://trac.webkit.org/changeset/64939

npm1 commented 4 years ago

This was discussed in a call (minutes: https://docs.google.com/document/d/1fzGwfa39HRyKUGIpz5TqbbwQ5CfbILRpzsxj622JGk8/edit#). @yoavweiss you had an AI from that I believe?

yoavweiss commented 4 years ago

Indeed. It's in my queue, hoping to get to it in the next few weeks.

paulirish commented 4 years ago

This recent chromium doc covers a lot of details regarding bfcache and web-exposed behavior: https://docs.google.com/document/d/1JtDCN9A_1UBlDuwkjn1HWxdhQ1H2un9K4kyPLgBqJUc/edit

npm1 commented 4 years ago

I wonder if this can be closed given the recent change to use history handling behavior?

yoavweiss commented 4 years ago

Indeed. Closed by https://github.com/w3c/navigation-timing/pull/129