Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: FetchEvent.navigationLoadType #1167

Closed
mfalken opened this issue Jul 13, 2017 · 45 comments
Closed

Proposal: FetchEvent.navigationLoadType #1167

mfalken opened this issue Jul 13, 2017 · 45 comments

Comments

@mfalken
Copy link
Member

mfalken commented Jul 13, 2017

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 whatwg/fetch#524

Developers want to simply know if a navigation was due to reload or back/forward (see whatwg/fetch#524 (comment)). 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
Copy link
Contributor

Thanks to @mattto, @wanderview, @toyoshim and everyone in whatwg/fetch#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:

  • "navigate"
  • "reload"
  • "back"
  • "forward"
  • Empty string for non-navigations

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
Copy link
Member

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

  • evt.request.mode === 'navigate' for these cases
  • evt.request.type and evt.request.destination have also cover aspects of how/why the browser initiated the load

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
Copy link
Contributor

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
Copy link
Member

Why was .isReload put on FetchEvent before?

@wanderview
Copy link
Member

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
Copy link

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
Copy link

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
Copy link
Collaborator

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

  • A back signal could be used to provide alternative (the user quickly went back) or related (the user spent some time before going back) suggestions. However, you might not want to do so for a forward navigation.

@toyoshim
Copy link

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
Copy link
Contributor

kinu commented Jul 20, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

kinu commented Jul 20, 2017

/cc @natechapin

@wanderview
Copy link
Member

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link
Member

request.navigationType with values like #1167 (comment) works for me.

@jakearchibald
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

annevk commented Nov 8, 2017

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
Copy link

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
Copy link
Contributor

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

@jakearchibald
Copy link
Contributor

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

@jakearchibald
Copy link
Contributor

@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
Copy link

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
Copy link
Member

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 pushed a commit to web-platform-tests/wpt that referenced this issue Apr 27, 2018
annevk pushed a commit to whatwg/fetch that referenced this issue Apr 27, 2018
And also a member on the request concept (reload-navigation flag) to support this API. See w3c/ServiceWorker#1167 for the discussion that led to this.

Tests: web-platform-tests/wpt#10192.

Corresponding HTML change: whatwg/html#3592.
annevk pushed a commit to whatwg/html that referenced this issue Apr 27, 2018
See w3c/ServiceWorker#1167 for the discussion that led to this change and whatwg/fetch#685 for the infrastructure in Fetch this builds upon.

Tests: web-platform-tests/wpt#10192.
@annevk
Copy link
Member

annevk commented Apr 27, 2018

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

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 2, 2018
…, a=testonly

Automatic update from web-platform-testsAdd tests for Request.isReloadNavigation

See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 58ee169367245c6fe5edc01177eac68f76c12f4a
wpt-pr: 10192
domenic pushed a commit to whatwg/html that referenced this issue May 25, 2018
See w3c/ServiceWorker#1167 for the discussion that led to this change and
whatwg/fetch#718 for the infrastructure in Fetch this builds upon.

Also includes minor editorial changes to the "traverse the history"
algorithm.

Tests: web-platform-tests/wpt#10909
annevk pushed a commit to web-platform-tests/wpt that referenced this issue May 29, 2018
annevk pushed a commit to whatwg/fetch that referenced this issue May 29, 2018
And also a member on the request concept (history-navigation flag) to support this API. See w3c/ServiceWorker#1167 for the discussion that led to this.

Tests: web-platform-tests/wpt#10909.

Corresponding HTML change: whatwg/html#3674.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 10, 2018
…n, a=testonly

Automatic update from web-platform-testsFetch: tests for Request's isHistoryNavigation

See whatwg/fetch#718, whatwg/html#3674, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 8f805ef0b6a3ddb06c0266cc56557d05e36f3a3d
wpt-pr: 10909
xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this issue Jun 12, 2018
…n, a=testonly

Automatic update from web-platform-testsFetch: tests for Request's isHistoryNavigation

See whatwg/fetch#718, whatwg/html#3674, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 8f805ef0b6a3ddb06c0266cc56557d05e36f3a3d
wpt-pr: 10909
@jakearchibald
Copy link
Contributor

AI: @jakearchibald check if we can close this.

@mfalken mfalken closed this as completed Oct 26, 2018
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
See w3c/ServiceWorker#1167 for the discussion that led to this change and whatwg/fetch#685 for the infrastructure in Fetch this builds upon.

Tests: web-platform-tests/wpt#10192.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
See w3c/ServiceWorker#1167 for the discussion that led to this change and
whatwg/fetch#718 for the infrastructure in Fetch this builds upon.

Also includes minor editorial changes to the "traverse the history"
algorithm.

Tests: web-platform-tests/wpt#10909
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
…, a=testonly

Automatic update from web-platform-testsAdd tests for Request.isReloadNavigation

See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 58ee169367245c6fe5edc01177eac68f76c12f4a
wpt-pr: 10192

UltraBlame original commit: 96c5380b8719433164c1c86dac5b4c8db50c1966
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…, a=testonly

Automatic update from web-platform-testsAdd tests for Request.isReloadNavigation

See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 58ee169367245c6fe5edc01177eac68f76c12f4a
wpt-pr: 10192

UltraBlame original commit: 96c5380b8719433164c1c86dac5b4c8db50c1966
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…, a=testonly

Automatic update from web-platform-testsAdd tests for Request.isReloadNavigation

See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 58ee169367245c6fe5edc01177eac68f76c12f4a
wpt-pr: 10192

UltraBlame original commit: 96c5380b8719433164c1c86dac5b4c8db50c1966
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…n, a=testonly

Automatic update from web-platform-testsFetch: tests for Request's isHistoryNavigation

See whatwg/fetch#718, whatwg/html#3674, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 8f805ef0b6a3ddb06c0266cc56557d05e36f3a3d
wpt-pr: 10909

UltraBlame original commit: 28a4916ff9d5655c7b816da2a9d4c7746f664304
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…n, a=testonly

Automatic update from web-platform-testsFetch: tests for Request's isHistoryNavigation

See whatwg/fetch#718, whatwg/html#3674, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 8f805ef0b6a3ddb06c0266cc56557d05e36f3a3d
wpt-pr: 10909

UltraBlame original commit: 28a4916ff9d5655c7b816da2a9d4c7746f664304
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…n, a=testonly

Automatic update from web-platform-testsFetch: tests for Request's isHistoryNavigation

See whatwg/fetch#718, whatwg/html#3674, and discussion in w3c/ServiceWorker#1167.
--

wpt-commits: 8f805ef0b6a3ddb06c0266cc56557d05e36f3a3d
wpt-pr: 10909

UltraBlame original commit: 28a4916ff9d5655c7b816da2a9d4c7746f664304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.