w3c / ServiceWorker

Service Workers
https://w3c.github.io/ServiceWorker/
Other
3.63k stars 312 forks source link

Proposal: FetchEvent.navigationLoadType #1167

Closed mfalken closed 6 years ago

mfalken commented 7 years ago

A while ago we removed FetchEvent.isReload in favor of Request.cache, reasoning that cache mode can tell you whether a navigation is a reload and also back/forward etc.

However it's quite complicated how to map Request.cache to navigation type. Even implementors don't really understand it and it's inconsistent across browsers. See https://github.com/whatwg/fetch/issues/524

Developers want to simply know if a navigation was due to reload or back/forward (see https://github.com/whatwg/fetch/issues/524#issuecomment-314870317). The proposal is to add something like FetchEvent.navigationLoadType. It could be consistent with window.performance.navigation.type, i.e., four types: 'navigate', 'reload', 'back_forward', and 'undefined'. But we may possibly want to differentiate back vs forward navigation.

jakearchibald commented 7 years ago

Thanks to @mattto, @wanderview, @toyoshim and everyone in https://github.com/whatwg/fetch/issues/524 for investigating this, I know I was a sticking point here, but I now agree that cache mode isn't enough. Sorry!

How about fetchEvent.navigationType:

navigationType is shorter than navigationLoadType and seems to convey the same thing.

Splitting out back_forward seems useful, also I hate enums with underscores 😄 .

I'm not sure what "undefined" means (it isn't in the spec), but an empty string seems more useful for non-navigations, eg if (event.navigationType).

@rsamuelklatchko does this work for you?

@hober @aliams @jatindersmann Any objections? I can organise a call if this is contentious.

wanderview commented 7 years ago

I guess my only concern here is that we already have some overlapping members on Request:

Should navigationType be part of the Request object instead of FetchEvent?

Is this something browsers might usefuly do so something with in the pass through case e.respondWith(fetch(e.request))? If we put this info on the request it would get passed through, but if its on FetchEvent it is not passed through.

jakearchibald commented 7 years ago

It would be consistent with .destination to add this to the request object rather than the event. I can't think of any pass-through benefits though.

wanderview commented 7 years ago

Why was .isReload put on FetchEvent before?

wanderview commented 7 years ago

Just bikeshedding here (sorry), but I wonder if something like this would be any better:

interface Request {
  readonly attribute RequestTrigger trigger;
};

enum RequestTrigger {
  "default",
  "reload",
  "back",
  "forward"
};

Then it can be combined with the other fields to determine what happened:

r.mode === 'navigate' && r.trigger === 'reload'

Non-subresource requests would always have a default trigger for now.

If chrome/firefox actually implemented .type and .destination then script could also distinguish between what kind of subresource load it is.

Or since no one has implemented .type and .destination we could try to fit the triggering information into there as well.

rsamuelklatchko commented 7 years ago

Our use case is we need to be able to be able to know when a user has requested a reload (our goal is that for any other navigation we'll serve results from the SW cache but for reload we want to ignore the cache and always fetch fresh results).

I'm open to most proposals where there is a field that explicitly lets us know there's a reload.

toyoshim commented 7 years ago

Just a question, but is splitting back_forward really useful? It did not mean moving to the previous page or the next page from, but moving to any point in the current navigation history.

I'm just thinking about how we deal with History.go(N) of N != 0 (and user actions from pull-down menu of back and forward button). If we want to split it, N < 0 is mapped to back and N > 0 is mapped to forward.

But I'm not sure if we can have a reasonable use case scenario to utilize such detailed separation of back and forward here.

KenjiBaheux commented 7 years ago

@toyoshim I think it's generally better to start with a non-committal design (i.e. keep your options open by splitting back and forward)

One example use case that comes to mind:

toyoshim commented 7 years ago

I am not proposing something new, but just have a concern about having a different design from another existing navigation type that didn't split back and forward navigation type. As far as I know, we do not have a spec that differentiate back and forward navigation types, and Chrome internal implementation does not differentiate them too. So, splitting back and forward navigation may need some additional code to plumb how the navigation was initiated. E.g., https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoaderTypes.h

kinu commented 7 years ago

I've chatted a bit about other engs in Chrome, and I share the concern with @toyoshim -- naively it feels a bit overkill but I could be wrong. Say, if a user back twice and then forward once the final navigation would be mapped to 'forward', but should it be really distinguished from 'back'? ...but I could be of course convinced if there really is a good, legitimate use case for that.

jakearchibald commented 7 years ago

I don't have a solid use-case for separating them, I was just thinking more detail is better, especially as breaking these things out later would be tough.

That said, if browsers don't distinguish internally, I'll get over back_forward as a value.

@wanderview does Gecko behave like this internally too?

jakearchibald commented 7 years ago

In terms of naming, the closest thing that fetch has is initiator, but I don't think it really fits there.

I think I still prefer navigationType, especially as it fits in with navigationLoadType.

If the default for non-navigations is an empty string, request.navigationType is truthy for navigations, which is kinda nice.

kinu commented 7 years ago

/cc @natechapin

wanderview commented 7 years ago

@wanderview does Gecko behave like this internally too?

I mean, we have the ability to distinguish forward/back because it effects the order of things loaded, but they seem to do similar operations:

http://searchfox.org/mozilla-central/source/docshell/base/nsIWebNavigation.idl#36 http://searchfox.org/mozilla-central/source/docshell/base/nsIWebNavigation.idl#48

The only use case I could think of for separating them was possibly triggering different transition animations or something for forward vs back. I know transitions are hard for other reasons, though.

jungkees commented 7 years ago

Both proposals look fine to me. I think I prefer fetchEvent.navigationType though considering: the values it serves are all types of "navigation", and we haven't come up with any solid use case using it outside of fetch event handlers.

jungkees commented 7 years ago

if browsers don't distinguish internally, I'll get over back_forward as a value.

I think conforming to the existing semantic is a starting point here. If we meet compelling use cases, a platform-wide extension would make more sense.

jakearchibald commented 7 years ago

@wanderview are you happy with that?

I agree back vs forward is useful for transitions, but I think we need another API for that. Popstate doesn't even distinguish between back vs forward.

wanderview commented 7 years ago

request.navigationType with values like https://github.com/w3c/ServiceWorker/issues/1167#issuecomment-315080941 works for me.

jakearchibald commented 7 years ago

F2F:

Agree on request.navigationInfo:

{
  reload: bool,
  history: bool,
  submit: bool
}

If the browser doesn't support a property, eg 'submit' in request.navigationInfo would be false.

camillelamy commented 7 years ago

Can we clarify that reload and history can't be true at the same time? At least, Chrome navigation expects it to be so.

annevk commented 7 years ago

This would be an extension to the Request object? I don't particularly like tear-off objects. Is this better than navigationReload, navigationHistory, and navigationSubmit? Who is taking responsibility for integrating this with HTML and writing all the tests?

davidcblack commented 7 years ago

I'm a bit confused by how exactly the proposed fields in navigationInfo actually map to real-world events, especially "submit". I'd like to verify my assumptions about the values for various use cases:

User hits the reload button in the browser: {reload:true, history:false, submit:false} User focuses the omnibox and immediately presses enter: {reload:true(??), history:false, submit:true} User types a new URL into the omnibox and presses enter: {reload:false, history:false, submit:true} User hits back button to get to a page: {reload:false, history:true, submit:false} Forward button: {reload:false, history:true, submit:false} User clicks on a link to get to page: {reload:false, history:false, submit:false} (??) Page is reloaded because user switched to that tab but Chrome had thrown it out of memory: {reload:true, history:true, submit:false} (??)

Honestly I think this API would be a lot simpler and easier to understand as something like an enum of actual user events rather than these three vague bools, but I'll take whatever I can get.

jakearchibald commented 7 years ago

@camillelamy confirmed. reload and history can't be true at the same time.

jakearchibald commented 7 years ago

@annevk grouping these navigation properties together seems nice, but maybe I don't understand the issues with tear off objects.

jakearchibald commented 7 years ago

@davidcblack reload would only be true in cases we'd change the cache mode. So pressing the refresh button, cmd-r etc etc.

Honestly I think this API would be a lot simpler and easier to understand as something like an enum of actual user events

This makes it difficult to express form submissions that are also a reload. Also, a single enum isn't extensible. With the proposed design we could introduce back and forward which provide specifics to the type of history navigation. Changing an enum in this way would break existing usage.

davidcblack commented 7 years ago

Sure, I didn't actually expect you to switch it to an enum (I agree with the limitations thereof) but was in general trying to make the observation that it felt more like the API was exposing some arcana of how Chrome internally thinks about navigations instead of how users (and presumably most developers) think about navigations. As such, I think the proposal is sufficiently unclear as to how the various values would be set in various cases that I'd request that it be published along with documentation containing a fairly comprehensive mapping of user actions to values, much as I attempted to do earlier.

I assume your comment about reload was referring to the "focuses the omnibox and hits enter" use case? Are my other interpretations of how the values would be set correct?

camillelamy commented 7 years ago

In Chrome navigation, "User focuses the omnibox and immediately presses enter" is a regular navigation. "Page is reloaded because user switched to that tab but Chrome had thrown it out of memory" is a restore navigation: it uses cache semantics which are different both from reloads and history navigations. I imagine this would translate as history: false and reload: false in this API?

annevk commented 7 years ago

This makes it difficult to express form submissions that are also a reload.

It would help if the use cases are flushed out a bit, since this would argue for exposing GET vs POST, not necessarily <form> vs <a> vs <area> vs ...

I'm also generally uncomfortable that we keep exposing more and more without having refactored the underlying algorithms. It's rather unclear we're doing the right thing.

NekR commented 7 years ago

I assume your comment about reload was referring to the "focuses the omnibox and hits enter" use case? Are my other interpretations of how the values would be set correct?

I think the comment was about interrupted submission (user offline) and reloading the same page when network restores, this would ask a user to "re-submit" the form. Hence both reload and submission. But I'm not 100% sure "reload" information is useful in such situation. Also it might be an implementation detail of Chrome, but not other browser, e.g. one may treat it as just a submission.

jakearchibald commented 6 years ago

@davidcblack I think there's some crossed wires around "submit". To clarify:

reload

Reload navigation. This includes:

Does not include:

The above are standard navigations.

Clicking the omnibox and hitting enter may be different between implementations. In Chrome it triggers a reload navigation.

history

A back or forward navigation. This includes:

Does not include:

submit

Navigation is part of a form submission. The details around this aren't super clear, as in should it include GET submissions? The most important thing at this state is we design with additional properties in mind, which is why we aren't going for an enum.

jakearchibald commented 6 years ago

I'm swayed by @annevk's arguments in https://github.com/w3c/ServiceWorker/issues/1167#issuecomment-342845250, we don't need a tear-off object for this. Separate properties fit the extensibility use-case fine.

I'm going to start writing tests for request.isReloadNavigation and request.isHistoryNavigation. Happy for any bikeshedding on the names.

request.isSubmitNavigation seems more complicated, but I'm happy that we can add it later, along with request.isBackNavigation should we need to break out the history type.

toyoshim commented 6 years ago

In Chrome, if the URL is not changed, clicking the omnibox and hitting enter does not results in a regular navigation, but behaves as a reload, it actually validates disk cache.

jakearchibald commented 6 years ago

@toyoshim ah, sorry. I assumed https://github.com/w3c/ServiceWorker/issues/1167#issuecomment-343237486 was correct. I'll update my summary.

NekR commented 6 years ago

What about location.href = location.href?

On Dec 11, 2017 18:13, "Jake Archibald" notifications@github.com wrote:

@toyoshim https://github.com/toyoshim ah, sorry. I assumed #1167 (comment) https://github.com/w3c/ServiceWorker/issues/1167#issuecomment-343237486 was correct. I'll update my summary.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/w3c/ServiceWorker/issues/1167#issuecomment-350752697, or mute the thread https://github.com/notifications/unsubscribe-auth/ABIlkVQwOxWjgEuM3RdMAjfh2zR9C3AUks5s_UaNgaJpZM4OWa45 .

toyoshim commented 6 years ago

location.href = location.href behaves as a normal navigation in Chrome.

Also, I'm not sure if we should explicitly define in this spec, but history.go(0) behaves as a reload. This is clearly defined in the History's spec, and even history.go() is per-frame operation, history.go(0) behaves as a per-page operation.

camillelamy commented 6 years ago

@toyoshim: location.href = location.href is a normal navigation in Chrome only if your URL doesn't have a fragment. If it has a fragment, it is a same-document navigation. We might want to fix this eventually so that it behaves similarly with and without the fragment.

wanderview commented 6 years ago

FYI it looks like @cdumez removed FetchEvent.isReload from WPT in https://github.com/w3c/web-platform-tests/commit/0f71c3b681c0606ab38dce76dfc5502275fb5554. I don't think we plan to remove it from our implementation until the alternative feature is spec'd.

cdumez commented 6 years ago

I'll note that I do not have strong feelings about what gets standardized here. However, I feel strongly that WPT should match the latest version of the specification.

jungkees commented 6 years ago

So, we agreed on https://github.com/w3c/ServiceWorker/issues/1167#issuecomment-350251122, right?

For request.isReloadNavigation, it seems we can reference reload-triggered navigation from the navigate algorithm, and set the corresponding boolean state (which needs to be added) of the request to relevant value. Maybe it'd be nice if we could define the reload-triggered navigation concept a little more concretely.

For request.isHistoryNavigation, we don't have any definition that collectively refers to history navigations yet. It seems we can add the history-triggerd navigation or something like that in traverse the history algorithm.

If we want to specify it by introducing some concrete states, we will need to pass some additional arguments or flags to the navigate algorithm as the request is created inside of the navigate.

@annevk, do you have any other ideas?

slightlyoff commented 6 years ago

In conversation with @davidcblack et. al., there are big users who continue to want back-navigation information. It sounds like we're generally agreed on approach. What's left here? Tests? Spec?

mfalken commented 6 years ago

I think we have enough for Chrome to start implementing request.isReloadNavigation and request.isHistoryNavigation. I suspect it's just a matter of exposing WebKit/Blink's FrameLoadType enum in FrameLoaderTypes.h.

It would be useful to have spec text and tests, but we're not really blocked on it (we just are working on other things at the moment).

yutakahirano commented 6 years ago

I'll define the flags on the fetch spec and have some features in HTML set them.

annevk commented 6 years ago

FYI: @yutakahirano did a lot of work and made Request object's isReloadNavigation happen in terms of standard and test updates. 🎉

jakearchibald commented 6 years ago

AI: @jakearchibald check if we can close this.

mfalken commented 6 years ago

Yep, this was speced with tests by @yutakahirano including at: