Skip to content

Commit

Permalink
GET Submissions: don't merge into searchParams
Browse files Browse the repository at this point in the history
The background
---

According to the HTML Specification's [§ 4.10.21.3 Form submission
algorithm][] section, submissions transmitted as `GET` requests [mutate
the `[action]` URL][mutate], overriding any search parameters already
encoded into the `[action]` value:

> [Mutate action URL][algorithm]
> ---
>
> Let <var>pairs</var> be the result of converting to a list of
> name-value pairs with <var>entry list</var>.
>
> Let <var>query</var> be the result of running the
> `application/x-www-form-urlencoded` serializer with <var>pairs</var>
> and <var>encoding</var>.
>
> Set <Var>parsed action</var>'s query component to <var>query</var>.
>
> Plan to navigate to <var>parsed action</var>.

[§ 4.10.21.3 Form submission algorithm]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm
[algorithm]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#submit-mutate-action
[mutate]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm:submit-mutate-action

Form submissions made with `POST` requests, on the other hand, encode
_both_ the `[action]` value's query parameters and any additionally
encoded body data:

> [Submit as entity body][post-submit]
> ---
>
> …
>
> Plan to navigate to a new request whose URL is <var>parsed
> action</var>, method is <var>method</var>, header list is
> « (`Content-Type`, mimeType) », and body is <var>body</var>.

[post-submit]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#submit-body

The problem
---

Ever since [18585fc][] (and subsequently [#108][]),
Turbo's `FetchRequest` has merged submission values from `<form>`
elements with `[method="get"]` and without `[action]` attributes _into_
the current URL's query parameters.

For example, consider the following forms rendered on the `/articles`
page:

```html
<form>
  <label for="q">Term</label>
  <input id="q" name="q">
  <button>Search</button>
</form>

<!-- elsewhere in the page -->
<form>
  <button name="order" value="asc">Sort ascending</button>
  <button name="order" value="desc">Sort ascending</button>
</form>
```

Without Turbo, entering "Hotwire" as the search term into
`input[name="q"]` and clicking the "Search" button would navigate the
browser to `/articles?q=Hotwire`. Then, clicking the "Sort ascending"
button would navigate the page to `/articles?order=asc`.

Without Turbo, entering "Hotwire" as the search term into
`input[name="q"]` and clicking the "Search" button navigates the
browser to `/articles?q=Hotwire`. Then, clicking the "Sort ascending"
button **navigates the page to `/articles?q=Hotwire&order=asc`**,
effectively _merging_ values from the page's URL and the `<form>`
element's fields.

[18585fc]: 18585fc#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R135-R142
[#108]: #108

The solution
---

This commit modifies the way that `FetchRequest` constructs its `url:
URL` and `body: FormData | URLSearchParams` properties.

First, it _always_ assigns a `body` property, but _conditionally_
encodes that value into the `fetchOptions: RequestInit` property based
on whether or not the request is an idempotent `GET`.

Next, if constructed with a `body: URLSearchParams` that has entries,
**replace** the `url: URL` instance's search params _entirely_ with
those values, like the HTML Specification algorithm entails.

If constructed with a `body: URLSearchParams` that is empty, pass the
`url: URL` through and assign the property **without modifying it**.

Additionally, this commit adds test cases to ensure that `POST` requests
transmit data in both the body and the URL.

While the previous multi-form merging behavior can be desirable, it is
not the behavior outlined by the HTML Specification, so Turbo should not
provide it out-of-the-box.

Having said that, there are two ways for applications to restore that
behavior:

1. declare a [turbo:before-fetch-request][] event listener to merge
   values _into_ the event's `detail.url` instance:

```js
addEventListener("turbo:before-fetch-request", ({ target, detail: { url, fetchOptions: { method } } }) => {
  if (target instanceof HTMLFormElement && method == "GET") {
    for (const [ name, value ] of new URLSearchParams(window.location.search)) {
      // conditionally call `set` or `append`,
      // depending on your application's needs
      if (url.searchParams.has(name)) continue
      else url.searchParams.set(name, value)
    }
  }
})
```

2. declare a [formdata][] event listener to merge values _into_ the
   submitted form's [FormData][] instance prior to entering the Turbo
   request pipeline:

```js
addEventListener("submit", (event) => {
  if (event.defaultPrevented) return

  const { target, submitter } = event
  const action = submitter?.getAttribute("formaction") || target.getAttribute("action")

  if (target.method == "get" && !action) {
    target.addEventListener("formdata", ({ formData }) => {
      for (const [ name, value ] of new URLSearchParams(window.location.search)) {
        // conditionally call `set` or `append`,
        // depending on your application's needs
        if (formData.has(name)) continue
        else formData.set(name, value)
      }
    }, { once: true })
  }
})
```

The conditionals in both cases could be omitted if applications
controlled the behavior more directly, like by declaring a Stimulus
controller and action (e.g. `<form data-controller="form"
data-action="formdata->form#mergeSearchParams">`).

[turbo:before-fetch-request]: https://turbo.hotwired.dev/reference/events
[formdata]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/formdata_event
[FormData]: https://developer.mozilla.org/en-US/docs/Web/API/FormData
  • Loading branch information
seanpdoyle committed Nov 22, 2021
1 parent af46cc1 commit 6906e57
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 13 deletions.
8 changes: 4 additions & 4 deletions src/core/frames/frame_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { clearBusyState, getAttribute, parseHTMLDocument, markAsBusy } from "../
import { FormSubmission, FormSubmissionDelegate } from "../drive/form_submission"
import { Snapshot } from "../snapshot"
import { ViewDelegate } from "../view"
import { getAction, expandURL, urlsAreEqual, locationIsVisitable, Locatable } from "../url"
import { getAction, expandURL, urlsAreEqual, locationIsVisitable } from "../url"
import { FormInterceptor, FormInterceptorDelegate } from "./form_interceptor"
import { FrameView } from "./frame_view"
import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor"
Expand Down Expand Up @@ -86,7 +86,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
this.currentURL = this.sourceURL
if (this.sourceURL) {
try {
this.element.loaded = this.visit(this.sourceURL)
this.element.loaded = this.visit(expandURL(this.sourceURL))
this.appearanceObserver.stop()
await this.element.loaded
this.hasBeenLoaded = true
Expand Down Expand Up @@ -235,8 +235,8 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest

// Private

private async visit(url: Locatable) {
const request = new FetchRequest(this, FetchMethod.get, expandURL(url), new URLSearchParams, this.element)
private async visit(url: URL) {
const request = new FetchRequest(this, FetchMethod.get, url, url.searchParams, this.element)

this.currentFetchRequest?.cancel()
this.currentFetchRequest = request
Expand Down
19 changes: 12 additions & 7 deletions src/http/fetch_request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,10 @@ export class FetchRequest {
this.delegate = delegate
this.method = method
this.headers = this.defaultHeaders
if (this.isIdempotent) {
this.url = mergeFormDataEntries(location, [ ...body.entries() ])
} else {
this.body = body
this.url = location
}
this.body = body
this.url = this.isIdempotent && this.entries.length ?
mergeFormDataEntries(new URL(location.href), this.entries) :
location
this.target = target
}

Expand Down Expand Up @@ -118,7 +116,7 @@ export class FetchRequest {
credentials: "same-origin",
headers: this.headers,
redirect: "follow",
body: this.body,
body: this.isIdempotent ? null : this.body,
signal: this.abortSignal,
referrer: this.delegate.referrer?.href
}
Expand Down Expand Up @@ -155,6 +153,7 @@ export class FetchRequest {

function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL {
const currentSearchParams = new URLSearchParams(url.search)
deleteAll(url.searchParams)

for (const [ name, value ] of entries) {
if (value instanceof File) continue
Expand All @@ -169,3 +168,9 @@ function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][])

return url
}

function deleteAll(searchParams: URLSearchParams) {
for (const name of searchParams.keys()) {
searchParams.delete(name)
}
}
12 changes: 12 additions & 0 deletions src/tests/fixtures/form.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,18 @@ <h1>Form</h1>
<input type="hidden" name="greeting" value="Hello from a redirect">
<input id="standard-get-form-submit" type="submit" value="form[method=get]">
</form>
<hr>
<form>
<button id="form-action-none-q-a" name="q" value="a">Submit ?q=a to form:not([action])</button>
</form>
<form action="/src/tests/fixtures/form.html?sort=asc">
<button id="form-action-self-submit">Submit form[action]</button>
<button id="form-action-self-q-b" name="q" value="b">Submit ?q=b to form[action]</button>
</form>
<form action="/__turbo/redirect?path=%2Fsrc%2Ftests%2Ffixtures%2Fform.html%3Fsort%3Dasc" method="post">
<button id="form-action-post-redirect-self-q-b" name="q" value="b">POST q=b to form[action]</button>
</form>
<hr>
<form action="/__turbo/messages" method="post" class="created">
<input type="hidden" name="content" value="Hello!">
<input type="submit" style="">
Expand Down
37 changes: 37 additions & 0 deletions src/tests/functional/form_submission_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ export class FormSubmissionTests extends TurboDriveTestCase {
await this.nextEventNamed("turbo:load")
}

async "test standard POST form submission merges values from both searchParams and body"() {
await this.clickSelector("#form-action-post-redirect-self-q-b")
await this.nextBody

this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html")
this.assert.equal(await this.getSearchParam("q"), "b")
this.assert.equal(await this.getSearchParam("sort"), "asc")
}

async "test standard POST form submission toggles submitter [disabled] attribute"() {
await this.clickSelector("#standard-post-form-submit")

Expand Down Expand Up @@ -126,6 +135,34 @@ export class FormSubmissionTests extends TurboDriveTestCase {
await this.nextEventNamed("turbo:load")
}

async "test standard GET form submission does not incorporate the current page's URLSearchParams values into the submission"() {
await this.clickSelector("#form-action-self-submit")
await this.nextBody

this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html")
this.assert.equal(await this.search, "?sort=asc")

await this.clickSelector("#form-action-none-q-a")
await this.nextBody

this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html")
this.assert.equal(await this.search, "?q=a", "navigates without omitted keys")
}

async "test standard GET form submission does not merge values into the [action] attribute"() {
await this.clickSelector("#form-action-self-submit")
await this.nextBody

this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html")
this.assert.equal(await this.search, "?sort=asc")

await this.clickSelector("#form-action-self-q-b")
await this.nextBody

this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html")
this.assert.equal(await this.search, "?q=b", "navigates without omitted keys")
}

async "test standard GET form submission toggles submitter [disabled] attribute"() {
await this.clickSelector("#standard-get-form-submit")

Expand Down
4 changes: 2 additions & 2 deletions src/tests/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ router.use((request, response, next) => {

router.post("/redirect", (request, response) => {
const { path, sleep, ...query } = request.body
const pathname = path ?? "/src/tests/fixtures/one.html"
const { pathname, query: searchParams } = url.parse(path ?? request.query.path ?? "/src/tests/fixtures/one.html", true)
const enctype = request.get("Content-Type")
if (enctype) {
query.enctype = enctype
}
setTimeout(() => response.redirect(303, url.format({ pathname, query })), parseInt(sleep || "0", 10))
setTimeout(() => response.redirect(303, url.format({ pathname, query: { ...query, ...searchParams } })), parseInt(sleep || "0", 10))
})

router.get("/redirect", (request, response) => {
Expand Down

0 comments on commit 6906e57

Please sign in to comment.