Restore searchParams to Visit and Frame Navigation (hotwired#466)

* Restore searchParams to Visit and Frame Navigation

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

* Attempt to fix flaky Scroll Restoration test

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.

src/core/drive/form_submission.ts

@@ -44,6 +44,7 @@ export class FormSubmission {
   readonly formElement: HTMLFormElement
   readonly submitter?: HTMLElement
   readonly formData: FormData
+  readonly location: URL
   readonly fetchRequest: FetchRequest
   readonly mustRedirect: boolean
   state = FormSubmissionState.initialized
@@ -58,6 +59,10 @@ export class FormSubmission {
     this.formElement = formElement
     this.submitter = submitter
     this.formData = buildFormData(formElement, submitter)
+    this.location = expandURL(this.action)
+    if (this.method == FetchMethod.get) {
+      mergeFormDataEntries(this.location, [ ...this.body.entries() ])
+    }
     this.fetchRequest = new FetchRequest(this, this.method, this.location, this.body, this.formElement)
     this.mustRedirect = mustRedirect
   }
@@ -72,10 +77,6 @@ export class FormSubmission {
     return this.submitter?.getAttribute("formaction") || this.formElement.getAttribute("action") || formElementAction || ""
   }
 
-  get location(): URL {
-    return expandURL(this.action)
-  }
-
   get body() {
     if (this.enctype == FormEnctype.urlEncoded || this.method == FetchMethod.get) {
       return new URLSearchParams(this.stringFormData)
@@ -220,3 +221,17 @@ function getMetaContent(name: string) {
 function responseSucceededWithoutRedirect(response: FetchResponse) {
   return response.statusCode == 200 && !response.redirected
 }
+
+function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL {
+  const searchParams = new URLSearchParams
+
+  for (const [ name, value ] of entries) {
+    if (value instanceof File) continue
+
+    searchParams.append(name, value)
+  }
+
+  url.search = searchParams.toString()
+
+  return url
+}

src/core/frames/frame_controller.ts

@@ -236,7 +236,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   // Private
 
   private async visit(url: URL) {
-    const request = new FetchRequest(this, FetchMethod.get, url, url.searchParams, this.element)
+    const request = new FetchRequest(this, FetchMethod.get, url, new URLSearchParams, this.element)
 
     this.currentFetchRequest?.cancel()
     this.currentFetchRequest = request

src/http/fetch_request.ts

@@ -57,9 +57,7 @@ export class FetchRequest {
     this.method = method
     this.headers = this.defaultHeaders
     this.body = body
-    this.url = this.isIdempotent ?
-      mergeFormDataEntries(new URL(location.href), this.entries) :
-      location
+    this.url = location
     this.target = target
   }
 
@@ -150,17 +148,3 @@ export class FetchRequest {
     if (event.defaultPrevented) await requestInterception
   }
 }
-
-function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL {
-  const searchParams = new URLSearchParams
-
-  for (const [ name, value ] of entries) {
-    if (value instanceof File) continue
-
-    searchParams.append(name, value)
-  }
-
-  url.search = searchParams.toString()
-
-  return url
-}

src/tests/fixtures/frames.html

@@ -29,6 +29,7 @@ <h2>Frames: #frame</h2>
       </form>
       <button id="add-turbo-action-to-frame" type="button">Add [data-turbo-action="advance"] to frame</button>
       <a id="link-frame" href="/src/tests/fixtures/frames/frame.html">Navigate #frame from within</a>
+      <a id="link-frame-with-search-params" href="/src/tests/fixtures/frames/frame.html?key=value">Navigate #frame with ?key=value</a>
       <a id="link-nested-frame-action-advance" href="/src/tests/fixtures/frames/frame.html" data-turbo-action="advance">Navigate #frame from within with a[data-turbo-action="advance"]</a>
     </turbo-frame>
     <a id="outside-frame-form" href="/src/tests/fixtures/frames/form.html" data-turbo-frame="frame">Navigate #frame to /frames/form.html</a>
@@ -84,7 +85,7 @@ <h2>Frames: #nested-child</h2>
     </turbo-frame>
 
     <turbo-frame id="navigate-top" target="_top">
-      <a href="/src/tests/fixtures/one.html">Visit one.html</a>
+      <a href="/src/tests/fixtures/one.html?key=value">Visit one.html?key=value</a>
       <a href="/src/tests/fixtures/one.html" data-turbo-frame="_self">Visit self</a>
     </turbo-frame>
 

src/tests/fixtures/navigation.html

@@ -33,6 +33,7 @@
     <section id="main" style="height: 200vh">
       <h1>Navigation</h1>
       <p><a id="same-origin-unannotated-link" href="/src/tests/fixtures/one.html">Same-origin unannotated link</a></p>
+      <p><a id="same-origin-unannotated-link-search-params" href="/src/tests/fixtures/one.html?key=value">Same-origin unannotated link ?key=value</a></p>
       <p><form id="same-origin-unannotated-form" method="get" action="/src/tests/fixtures/one.html"><button>Same-origin unannotated form</button></form></p>
       <p><a id="same-origin-replace-link" href="/src/tests/fixtures/one.html" data-turbo-action="replace">Same-origin data-turbo-action=replace link</a></p>
       <p><form id="same-origin-replace-form-get" action="/src/tests/fixtures/one.html" data-turbo-action="replace"><button>Same-origin data-turbo-action=replace form</button></form></p>

src/tests/fixtures/scroll_restoration.html

@@ -4,6 +4,7 @@
     <meta charset="utf-8">
     <title>Scroll Restoration</title>
     <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
+    <script src="/src/tests/fixtures/test.js"></script>
     <style>
       li {
         min-height: 50vh;

src/tests/fixtures/visit.html

@@ -10,6 +10,7 @@
     <section>
       <h1>Visit</h1>
       <p><a id="same-origin-link" href="/src/tests/fixtures/one.html">Same-origin link</a></p>
+      <p><a id="same-origin-link-search-params" href="/src/tests/fixtures/one.html?key=value">Same-origin link with ?key=value</a></p>
       <p><a id="sample-response" href="/src/tests/fixtures/one.html">Sample response</a></p>
     </section>
   </body>

src/tests/functional/frame_tests.ts

@@ -27,6 +27,22 @@ export class FrameTests extends TurboDriveTestCase {
     this.assert.equal(await frame.getAttribute("src"), await this.propertyForSelector("#hello a", "href"))
   }
 
+  async "test following a link sets the frame element's [src]"() {
+    await this.clickSelector("#link-frame-with-search-params")
+
+    const { url } = await this.nextEventOnTarget("frame", "turbo:before-fetch-request")
+    const fetchRequestUrl = new URL(url)
+
+    this.assert.equal(fetchRequestUrl.pathname, "/src/tests/fixtures/frames/frame.html")
+    this.assert.equal(fetchRequestUrl.searchParams.get("key"), "value", "fetch request encodes query parameters")
+
+    await this.nextBeat
+    const src = new URL(await this.attributeForSelector("#frame", "src") || "")
+
+    this.assert.equal(src.pathname, "/src/tests/fixtures/frames/frame.html")
+    this.assert.equal(src.searchParams.get("key"), "value", "[src] attribute encodes query parameters")
+  }
+
   async "test a frame whose src references itself does not infinitely loop"() {
     await this.clickSelector("#frame-self")
 
@@ -129,6 +145,8 @@ export class FrameTests extends TurboDriveTestCase {
     const frameText = await this.querySelector("body > h1")
     this.assert.equal(await frameText.getVisibleText(), "One")
     this.assert.notOk(await this.hasSelector("#navigate-top a"))
+    this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html")
+    this.assert.equal(await this.getSearchParam("key"), "value")
   }
 
   async "test following a link that declares data-turbo-frame='_self' within a frame with target=_top navigates the frame itself"() {

src/tests/functional/navigation_tests.ts

@@ -48,6 +48,15 @@ export class NavigationTests extends TurboDriveTestCase {
     })
     await this.nextBody
     this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html")
+    this.assert.equal(await this.search, "")
+    this.assert.equal(await this.visitAction, "advance")
+  }
+
+  async "test following a same-origin unannotated link with search params"() {
+    this.clickSelector("#same-origin-unannotated-link-search-params")
+    await this.nextBody
+    this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html")
+    this.assert.equal(await this.search, "?key=value")
     this.assert.equal(await this.visitAction, "advance")
   }
 

src/tests/functional/scroll_restoration_tests.ts

@@ -3,9 +3,8 @@ import { TurboDriveTestCase } from "../helpers/turbo_drive_test_case"
 export class ScrollRestorationTests extends TurboDriveTestCase {
   async "test landing on an anchor"() {
     await this.goToLocation("/src/tests/fixtures/scroll_restoration.html#three")
-    await this.nextBeat
+    await this.nextBody
     const { y: yAfterLoading } = await this.scrollPosition
-    await this.nextBeat
     this.assert.notEqual(yAfterLoading, 0)
   }
 

src/tests/functional/visit_tests.ts

@@ -79,6 +79,13 @@ export class VisitTests extends TurboDriveTestCase {
     this.assert.ok(url.toString().includes("/src/tests/fixtures/one.html"))
   }
 
+  async "test turbo:before-fetch-request event.detail encodes searchParams"() {
+    await this.clickSelector("#same-origin-link-search-params")
+    const { url } = await this.nextEventNamed("turbo:before-fetch-request")
+
+    this.assert.ok(url.includes("/src/tests/fixtures/one.html?key=value"))
+  }
+
   async "test turbo:before-fetch-response open new site"() {
     this.remote.execute(() => addEventListener("turbo:before-fetch-response", async function eventListener(event: any) {
       removeEventListener("turbo:before-fetch-response", eventListener, false);