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

Are 204/205 responses, downloads, or external software handoffs "successful" navigations or failures? #137

Closed
domenic opened this issue Jul 28, 2021 · 5 comments · Fixed by #206

Comments

@domenic
Copy link
Collaborator

domenic commented Jul 28, 2021

That is, what should navigation.navigate() return if you navigate to:

  • A 204 or 205 response? In this case the browser will stay on the current page.
  • A response with Content-Disposition: attachment? In this case the browser will start a download.
  • A response with a content-type the browser doesn't know how to handle directly? In this case the browser will either start a download or launch an external app.

I feel like we have a few options:

  1. Count all of these as an aborted navigation error, i.e. the returned promise rejects with an "AbortError" DOMException, navigateerror fires, etc. The mental model here is that navigation.navigate(url) is a request to go to url, and if we don't end up on url, we failed.

  2. Count the 204/205 case as an aborted navigation error, but count the others as successful navigations which fulfill the promise and fire navigatesuccess once the download starts or the response is handed off. The mental model here is in the 204/205 case literally nothing got accomplished, so that's a failure, but in the other cases you succeeded at something, even if the URL didn't update.

  3. Count all of these cases as a success. The mental model here is that we got to the end of the navigation process without errors (204/205 are technically success status codes!) so we should signal as such.

Related:

  • Add the ability to navigate to a download or form submission #82 suggests adding an explicit navigation.navigate(url, { download: true }). We'd certainly want that to treat getting a download response as a success. And probably a 204/205 as a failure?

  • Add the ability to navigate to a download or form submission #82 also allows a different model, where if you don't pass { download: true }, and the server sends Content-Disposition: attachment, we either ignore the server (novel capability; might be dangerous) or we abort the navigation without doing any download (and return failure).

  • These cases are especially strange because so far we only resolve the returned promise/fire the events for same-document navigations, on the reasoning that for a cross-document navigation, the page is going away anyway. But in these cases the page is not going away, so we should really give either a success or failure instead of leaving things hanging. If we do what is suggested in Should appHistory.navigate()'s returned promise settle for cross-document navigations? #95 then this all becomes more uniform and we're always resolving the promise.

@tbondwilkinson
Copy link
Contributor

I lean towards this being a success, as weird as it is.

I feel like a 204/205 should be the same user experience as navigating to a page and then navigating back.

I think without more intentionality with the navigate API (like download: true, where if the browser doesn't download something, it's a failure), it's hard to classify these as "wrong".

@dvoytenko
Copy link

I'd tend to think of this case as an aborted navigation because the stack hasn't changed, no transition took place, etc. In other words, from the navigational point of view these cases do not make sense. Although I could see how a browser that doesn't support downloads would work as: (1) navigate to the resource, (2) save the resource, (3) come back. In which case it's closer to what @tbondwilkinson is saying.

@domenic
Copy link
Collaborator Author

domenic commented Nov 4, 2021

I feel like we have a few options:

I missed another option, which I believe is the current spec/Chrome behavior:

  1. Treat all of these cases as cross-document navigations, and never resolve the returned promises or fire any events.

The downside of this is that most of the time you get a never-returned promise, it's because the page is being unloaded, so any promises/events are kind of useless and inaccurate.

So I think I still lean toward one of (1)-(3).

@annevk
Copy link

annevk commented Mar 9, 2022

What I worry about with 1-3 is that they introduce a new timing channel. I think that means only 4 is reasonable, although I agree that it's far from ideal.

I could see splitting this based on the origin (accounting correctly for redirects will add complexity here) though. If we do that I think success makes the most sense when it's the same origin.

@domenic
Copy link
Collaborator Author

domenic commented Mar 9, 2022

Great catch. I think you are right about a new timing channel... we simply give no information about when the destination server returned a 204/etc. today, as far as I can tell. (E.g., we don't fire unload/navigation timing in such cases, and we fire beforeunload way earlier, before doing the fetch to the destination server.)

Given that, splitting based on origin seems like it probably causes more confusion. The best story I think we could tell developers would be:

  • If you're navigating cross-origin, a 204/etc. will always result in forever-pending promises.
  • But if you're navigating same-origin, you'll get a settled promise.
    • ...unless your same-origin page redirects cross-origin, so, this isn't really a property you can tell by just looking at the call site; you also need to be sure you know what your server is doing.

Given that the main attraction of (1)-(3) would be to make developers never have to deal with forever-pending promises in documents that are sticking around, and we can't guarantee that benefit in cross-origin cases, I'm inclined to go with (4) uniformly.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 17, 2022
See WICG/navigation-api#137.

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 18, 2022
See WICG/navigation-api#137.

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 21, 2022
See WICG/navigation-api#137.

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 21, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

This requires using a different technique for cleaning up ongoing
navigation promises on frame destruction. As https://crbug.com/1219750
discusses, there is no super-clean way to do this, but hooking
LocalDOMWindow::FrameDestroyed() seems to work.

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 22, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

This requires using a different technique for cleaning up ongoing
navigation promises on frame destruction. As https://crbug.com/1219750
discusses, there is no super-clean way to do this, but hooking
LocalDOMWindow::FrameDestroyed() seems to work.

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 22, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 23, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 23, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 23, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}
past pushed a commit to web-platform-tests/wpt that referenced this issue Mar 23, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this issue Mar 25, 2022
…orm-tests#33234)

See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 2, 2022
…loads never settle, a=testonly

Automatic update from web-platform-tests
Navigation API: ensure that 204/205/downloads never settle (#33234)

See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
--

wpt-commits: 2a189356f6c457cd553394c2178b194926960cf0
wpt-pr: 33234
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Apr 5, 2022
…loads never settle, a=testonly

Automatic update from web-platform-tests
Navigation API: ensure that 204/205/downloads never settle (#33234)

See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
--

wpt-commits: 2a189356f6c457cd553394c2178b194926960cf0
wpt-pr: 33234
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}
NOKEYCHECK=True
GitOrigin-RevId: 44593d735dac200a55f9365514a0fa55358f9491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants