Skip to content

Align redirect fragment handling with HTTP #696

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

Merged
merged 1 commit into from
Feb 25, 2021
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 12, 2018

This clarifies how fragment conflicts between the request's current URL
and the Location header are resolved (the latter wins if non-null).
This is the same for documents and subresources.

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

HTML change: whatwg/html#6391.

Follow-ups: #1167 and #1175.

Fixes #505.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@annevk annevk requested a review from wanderview April 12, 2018 08:02
@annevk

This comment has been minimized.

annevk added a commit to whatwg/xhr that referenced this pull request Apr 12, 2018
Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If fetch() is going to return fragments on the Response.url, should the fetch spec also handle fragment propagation on redirects? I don't think it currently does because html spec handles navigation redirects.

Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need service worker fixes before this can be implemented. Consider:

  1. If a FetchEvent.request.url has a fragment, but Response.url does not, then we want to propagate the request fragment to the new response URL.
  2. We need to do something if the FetchEvent request and response URLs have different fragments. I guess Response fragment wins?

Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume fragments will need to be stored in Cache API. This should be fine in the spec since I believe it just holds Response objects. In practice, though, you may run into situations where a site has old Response values without a fragment in Cache API and new Response objects with a fragment. Not sure there is anything we can do to fix that, but its something to consider.

Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem fine, but see my other comments about other things that need to be addressed before implementation. Thanks!

@annevk annevk force-pushed the annevk/response-fragments branch from ee53836 to 53de36a Compare August 23, 2018 12:09
@annevk

This comment has been minimized.

@annevk

This comment has been minimized.

@wanderview
Copy link
Member

FWIW, the bit I implemented for propagating the request fragment to the final response in firefox is here:

https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/dom/serviceworkers/ServiceWorkerEvents.cpp#720-731

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Aug 24, 2018
@annevk

This comment has been minimized.

@annevk

This comment has been minimized.

@annevk

This comment has been minimized.

@annevk

This comment has been minimized.

@jungkees

This comment has been minimized.

@annevk

This comment has been minimized.

@wanderview

This comment has been minimized.

@jungkees

This comment has been minimized.

@annevk

This comment has been minimized.

@jungkees

This comment has been minimized.

Base automatically changed from master to main January 15, 2021 07:38
@annevk annevk force-pushed the annevk/response-fragments branch from d50c213 to bb1c332 Compare February 10, 2021 17:30
This clarifies how fragment conflicts between the request's current URL
and the Location header are resolved (the latter wins if non-null).
This is the same for documents and subresources.

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

HTML change: whatwg/html#6391.

Follow-ups: #1167 and #1175.

Fixes #505.
@annevk annevk force-pushed the annevk/response-fragments branch from 0d779b6 to 99e0110 Compare February 19, 2021 10:03
@annevk annevk requested a review from davidben February 19, 2021 10:05
@annevk annevk changed the title Propagate URL fragments Align redirect fragment handling with HTTP Feb 19, 2021
@annevk
Copy link
Member Author

annevk commented Feb 19, 2021

I've reduced the scope of this PR (for the second time) to be about redirects only and filed a second follow-up issue, this time for service workers. Who knew fragments would be this much fun.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 24, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575
@annevk
Copy link
Member Author

annevk commented Feb 24, 2021

@davidben @wanderview any concerns with merging this (and the HTML PR) now that the service worker bits have been moved to #1175?

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Feb 25, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 429a832dd2419ede43885279864503338753fd61
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 25, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 429a832dd2419ede43885279864503338753fd61
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 25, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 429a832dd2419ede43885279864503338753fd61
@annevk annevk merged commit a876e5d into main Feb 25, 2021
@annevk annevk deleted the annevk/response-fragments branch February 25, 2021 11:34
annevk added a commit to whatwg/html that referenced this pull request Feb 25, 2021
Complements whatwg/fetch#696. See that pull request for context and tests.
@triple-underscore
Copy link

In the location URL algorithm:

If locationURL’s fragment is null, then set locationURL’s fragment to requestFragment.

The variable locationURL is not declared; it may be location.
Assuming location, it can be failure. Thus, the if clause needs to include for checking it.

@annevk
Copy link
Member Author

annevk commented Feb 26, 2021

Thanks, I created #1180 to address that.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 5, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 8, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Mar 8, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 10, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 2cdf82ffca4a04f470664a28dc850457a5ebe88a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 10, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 6364dbe87f3fc92171b65a6884434b80904e2b29
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 10, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 2cdf82ffca4a04f470664a28dc850457a5ebe88a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 10, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 6364dbe87f3fc92171b65a6884434b80904e2b29
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Mar 10, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 2cdf82ffca4a04f470664a28dc850457a5ebe88a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Mar 10, 2021
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575

UltraBlame original commit: 6364dbe87f3fc92171b65a6884434b80904e2b29
bhearsum pushed a commit to mozilla-releng/staging-firefox that referenced this pull request May 1, 2025
Automatic update from web-platform-tests
Fetch: fragments

Tests for whatwg/fetch#696.
--

wpt-commits: e8181c07ac768f4fc5c7b74016a8e51226717400
wpt-pr: 27575
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Browsers preserve fragments on redirects
6 participants