GET Submissions: don't merge into searchParams (#461)

* Dispatch `turbo:before-fetch-request` with URL

Dispatch `turbo:before-fetch-request` with a direct reference to the
`FetchRequest` instance's `url: URL` property as part of the
`event.detail`.

* `GET` Submissions: don't merge into `searchParams`

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

Author: seanpdoyle

src/core/frames/frame_controller.ts

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

src/http/fetch_request.ts

@@ -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
   }
 
@@ -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
     }
@@ -144,7 +142,7 @@ export class FetchRequest {
       cancelable: true,
       detail: {
         fetchOptions,
-        url: this.url.href,
+        url: this.url,
         resume: this.resolveRequestPromise
       },
       target: this.target as EventTarget
@@ -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
@@ -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)
+  }
+}

src/tests/fixtures/form.html

@@ -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="">

src/tests/functional/form_submission_tests.ts

@@ -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")
 
@@ -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")
 

src/tests/functional/visit_tests.ts

@@ -76,7 +76,7 @@ export class VisitTests extends TurboDriveTestCase {
     const { url, fetchOptions } = await this.nextEventNamed("turbo:before-fetch-request")
 
     this.assert.equal(fetchOptions.method, "GET")
-    this.assert.include(url, "/src/tests/fixtures/one.html")
+    this.assert.ok(url.toString().includes("/src/tests/fixtures/one.html"))
   }
 
   async "test turbo:before-fetch-response open new site"() {

src/tests/server.ts

@@ -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) => {