Restore searchParams to Visit and Frame Navigation#466
Conversation
Closes [hotwired#456][] The changes made in [hotwired#464][] resolved an issue in how `GET` requests merged query parameters into their `URL` instance. Unfortunately, the changes were too loosely scoped, and they affected _all_ `GET` requests, including those initiated by `<a>` elements with `[href]` attributes that declare search parameters directly (that *do not* need to be "merged" into anything). The [change][] made in [hotwired#464][] to conditionally include the `FetchRequest.body` based on the request's verb enables the changes made in this commit. In response to that change, this commit moves the `mergeFormDataEntries()` function out of the `FetchMethod` module and into the `FormSubmission` class. Since merging _from_ `FormData` instances _into_ a `URL` is a concern of `GET`-powered Form Submissions, moving the function out of the `FetchRequest` further simplifies that class. Move the `mergeFormDataEntries()` to the `FormSubmission` constructor to re-write its `location` property's [searchParams][] prior to constructing the instance's `FetchRequest`. [hotwired#456]: hotwired#465 [hotwired#464]: hotwired#464 [change]: hotwired@6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119 [searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams
718a876 to
5e68bd8
Compare
There is a common test failure in CI:
```
× firefox on linux 5.11.0-1020-azure - ScrollRestorationTests - landing on an anchor (0.02s)
444
TypeError: Cannot read property 'slice' of null
445
at RemoteChannel.read @ src/tests/helpers/remote_channel.ts:14:44
```
This commit addresses it in two ways. First, ensures that the
`scroll_restoration.html` page loads the
`src/tests/fixtures/test.js` script to wire-up the remote channels (like
`window.eventLogs` and `window.mutationLogs`).
Next, it modifies the test itself to await the `this.nextBody` hook, to
ensure that the page load completes before checking the scroll.
|
Seems like this is still broken if there's a get-form with params in the URL. Like: |
|
#461 and #464 aimed to cover They was guided by the to the HTML Specification's § 4.10.21.3 Form submission
Are the query parameters included, or ignored? |
|
The query parameters are ignored. We just end up doing a visit to |
|
If Turbo were removed or JavaScript were disabled, does the browser include or omit the |
|
It's included. And it used to be with Turbo as well. I found this as a regression in a system test that verified that this used to work. |
I can confirm this. The motivation behind #461, #464, and #466 (this PR) was to make Turbo's In an environment without Turbo, the encoded values are ignored. Consider this code sample: <!-- index.html -->
<html>
<head></head>
<body>
<form action="/destination.html">
<button>form[action="destination.html"]</button>
</form>
<form action="/destination.html?from=index">
<button>form[action="destination.html?from=index"]</button>
</form>
<form action="/destination.html?from=index">
<button name="foo" value="bar">form[action="destination.html?from=index"] button[name="foo"][value="bar"]</button>
</form>
<form action="/destination.html">
<input type="hidden" name="foo" value="bar">
<button>form[action="destination.html"] input[type="hidden"][name="foo"][value="bar"]</button>
</form>
<script>document.body.insertAdjacentText("beforeend", window.location.toString())</script>
</body>
</html>
<!-- destination.html -->
<html>
<head></head>
<body>
<form action="/index.html">
<button>form[action="index.html"]</button>
</form>
<form action="/index.html?from=destination">
<button>form[action="index.html?from=destination"]</button>
</form>
<form action="/index.html?from=destination">
<button name="foo" value="bar">form[action="index.html?from=destination"] button[name="foo"][value="bar"]</button>
</form>
<form action="/index.html">
<input type="hidden" name="foo" value="bar">
<button>form[action="index.html"] input[type="hidden"][name="foo"][value="bar"]</button>
</form>
<script>document.body.insertAdjacentText("beforeend", window.location.toString())</script>
</body>
</html>When navigating the From what I'm experiencing, values are only included in the next page's URL when encoded onto the submitter as You can experiment with the sandbox by visiting https://fuschia-abyssinian-planarian.glitch.me (or view the code at https://glitch.com/edit/#!/fuschia-abyssinian-planarian). Is that a proper recreation of your circumstances? |
|
Oh wow. Yeah, you're right. This was just a quirk of Turbo's. I guess it's a bit of regression on the Turbo side of things, but agree that it seems reasonable to match the native HTML experience. Thanks for the very thorough illumination ✌️. All good letting things stay as they are. |
Closes hotwired/turbo#456
The changes made in hotwired/turbo#464 resolved an issue in how
GETrequests merged query parameters into theirURLinstance.Unfortunately, the changes were too loosely scoped, and they affected
all
GETrequests, including those initiated by<a>elements with[href]attributes that declare search parameters directly (that donot need to be "merged" into anything).
The change made in hotwired/turbo#464 to conditionally include
the
FetchRequest.bodybased on the request's verb enables the changesmade in this commit.
In response to that change, this commit moves the
mergeFormDataEntries()function out of theFetchMethodmodule andinto the
FormSubmissionclass. Since merging fromFormDatainstances into a
URLis a concern ofGET-powered Form Submissions,moving the function out of the
FetchRequestfurther simplifies thatclass. Move the
mergeFormDataEntries()to theFormSubmissionconstructor to re-write its
locationproperty's searchParams priorto constructing the instance's
FetchRequest.