GET Forms: fire submit-start and submit-end (#424)

Closes #421 Closes #122 --- Fire the same sequence of events for `<form method="get">` submissions as for `<form method="post">` submissions. The [FormSubmission.prepareHeadersForRequest][] already accounts for `[method="get"]` forms by omitting the `Accept:` header with the custom `turbo-stream` MIME type. Visit actions --- Prior to this change, a `<form method="get">` would _always_ result in two Visits: the first with `{ action: "advance" }`, then a second with `{ action: "replace" }`. Since the response to a `<form method="get">` has the potential to be a [200 OK][] or a [201 Created][], this commit also modifies the `Visit` class to skip the `followRedirect()` call when the request is idempotent and the response is not a redirect. Test changes --- The `eventLogs` mechanisms we have in place declared in `src/tests/fixutres/test.js` cannot properly serialize `Element` instances, so adding `turbo:submit-start` and `turbo:submit-end` listeners to serialize events for our test suite isn't possible in the current configuration. To that end, this commit adds assertions for `<form>` submit event sequences for all events except those two. In their place, the suite adds listeners to set `[data-form-submit-start]` and `[data-form-submit-end]` to the `<html>` element when they fire. [FormSubmission.prepareHeadersForRequest]: https://github.com/hotwired/turbo/blob/58d2261274533a80a2c5efda7da211b3f20efcbb/src/core/drive/form_submission.ts#L136-L144 [200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200 [200 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201
hotwired · Oct 18, 2021 · 9fcb68d · 9fcb68d
1 parent 58d2261
commit 9fcb68d
Show file tree

Hide file tree

Showing 6 changed files with 130 additions and 38 deletions.
diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts
@@ -44,11 +44,7 @@ export class Navigator {
     this.stop()
     this.formSubmission = new FormSubmission(this, form, submitter, true)
 
-    if (this.formSubmission.isIdempotent) {
-      this.proposeVisit(this.formSubmission.fetchRequest.url, { action: this.getActionForFormSubmission(this.formSubmission) })
-    } else {
-      this.formSubmission.start()
-    }
+    this.formSubmission.start()
   }
 
   stop() {
@@ -92,8 +88,9 @@ export class Navigator {
           this.view.clearSnapshotCache()
         }
 
-        const { statusCode } = fetchResponse
-        const visitOptions = { response: { statusCode, responseHTML } }
+        const { statusCode, redirected } = fetchResponse
+        const action = this.getActionForFormSubmission(formSubmission)
+        const visitOptions = { action, response: { statusCode, responseHTML, redirected } }
         this.proposeVisit(fetchResponse.location, visitOptions)
       }
     }

diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -51,6 +51,7 @@ const defaultOptions: VisitOptions = {
 
 export type VisitResponse = {
   statusCode: number,
+  redirected: boolean,
   responseHTML?: string
 }
 
@@ -259,7 +260,7 @@ export class Visit implements FetchRequestDelegate {
   }
 
   followRedirect() {
-    if (this.redirectedToLocation && !this.followedRedirect) {
+    if (this.redirectedToLocation && !this.followedRedirect && this.response?.redirected) {
       this.adapter.visitProposedToLocation(this.redirectedToLocation, {
         action: 'replace',
         response: this.response
@@ -289,25 +290,27 @@ export class Visit implements FetchRequestDelegate {
 
   async requestSucceededWithResponse(request: FetchRequest, response: FetchResponse) {
     const responseHTML = await response.responseHTML
+    const { redirected, statusCode } = response
     if (responseHTML == undefined) {
-      this.recordResponse({ statusCode: SystemStatusCode.contentTypeMismatch })
+      this.recordResponse({ statusCode: SystemStatusCode.contentTypeMismatch, redirected })
     } else {
       this.redirectedToLocation = response.redirected ? response.location : undefined
-      this.recordResponse({ statusCode: response.statusCode, responseHTML })
+      this.recordResponse({ statusCode: statusCode, responseHTML, redirected })
     }
   }
 
   async requestFailedWithResponse(request: FetchRequest, response: FetchResponse) {
     const responseHTML = await response.responseHTML
+    const { redirected, statusCode } = response
     if (responseHTML == undefined) {
-      this.recordResponse({ statusCode: SystemStatusCode.contentTypeMismatch })
+      this.recordResponse({ statusCode: SystemStatusCode.contentTypeMismatch, redirected })
     } else {
-      this.recordResponse({ statusCode: response.statusCode, responseHTML })
+      this.recordResponse({ statusCode: statusCode, responseHTML, redirected })
     }
   }
 
   requestErrored(request: FetchRequest, error: Error) {
-    this.recordResponse({ statusCode: SystemStatusCode.networkFailure })
+    this.recordResponse({ statusCode: SystemStatusCode.networkFailure, redirected: false })
   }
 
   requestFinished() {

diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts
@@ -152,13 +152,9 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
 
     this.reloadable = false
     this.formSubmission = new FormSubmission(this, element, submitter)
-    if (this.formSubmission.fetchRequest.isIdempotent) {
-      this.navigateFrame(element, this.formSubmission.fetchRequest.url.href, submitter)
-    } else {
-      const { fetchRequest } = this.formSubmission
-      this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest)
-      this.formSubmission.start()
-    }
+    const { fetchRequest } = this.formSubmission
+    this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest)
+    this.formSubmission.start()
   }
 
   // Fetch request delegate

diff --git a/src/observers/form_submit_observer.ts b/src/observers/form_submit_observer.ts
@@ -36,7 +36,7 @@ export class FormSubmitObserver {
       const submitter = event.submitter || undefined
 
       if (form) {
-        const method = submitter?.getAttribute("formmethod") || form.method
+        const method = submitter?.getAttribute("formmethod") || form.getAttribute("method")
 
         if (method != "dialog" && this.delegate.willSubmitForm(form, submitter)) {
           event.preventDefault()

diff --git a/src/tests/fixtures/form.html b/src/tests/fixtures/form.html
@@ -18,7 +18,12 @@ <h1>Form</h1>
       <form action="/__turbo/redirect" method="post" class="redirect">
         <input type="hidden" name="path" value="/src/tests/fixtures/form.html">
         <input type="hidden" name="greeting" value="Hello from a redirect">
-        <input type="submit">
+        <input id="standard-post-form-submit" type="submit" value="form[method=post]">
+      </form>
+      <form action="/__turbo/redirect" method="get" class="redirect">
+        <input type="hidden" name="path" value="/src/tests/fixtures/form.html">
+        <input type="hidden" name="greeting" value="Hello from a redirect">
+        <input id="standard-get-form-submit" type="submit" value="form[method=get]">
       </form>
       <form action="/__turbo/messages" method="post" class="created">
         <input type="hidden" name="content" value="Hello!">
@@ -155,6 +160,16 @@ <h1>Form</h1>
         <button type="submit">Submit</button>
       </form>
 
+      <form action="/__turbo/redirect" method="post" data-turbo-frame="frame">
+        <input type="hidden" name="path" value="/src/tests/fixtures/frames/frame.html">
+        <button id="targets-frame-post-form-submit" type="submit">targets-frame form[method=post]</button>
+      </form>
+
+      <form action="/__turbo/redirect" method="get" data-turbo-frame="frame">
+        <input type="hidden" name="path" value="/src/tests/fixtures/frames/frame.html">
+        <button id="targets-frame-get-form-submit" type="submit">targets-frame form[method=get]</button>
+      </form>
+
       <form action="/__turbo/redirect" method="post" data-turbo-frame="frame" class="frame">
         <input type="hidden" name="path" value="/src/tests/fixtures/frames/frame.html">
         <button type="submit">Submit</button>

diff --git a/src/tests/functional/form_submission_tests.ts b/src/tests/functional/form_submission_tests.ts
@@ -4,7 +4,8 @@ export class FormSubmissionTests extends TurboDriveTestCase {
   async setup() {
     await this.goToLocation("/src/tests/fixtures/form.html")
     await this.remote.execute(() => {
-      addEventListener("turbo:submit-start", () => document.documentElement.setAttribute("data-form-submitted", ""), { once: true })
+      addEventListener("turbo:submit-start", () => document.documentElement.setAttribute("data-form-submit-start", ""), { once: true })
+      addEventListener("turbo:submit-end", () => document.documentElement.setAttribute("data-form-submit-end", ""), { once: true })
     })
   }
 
@@ -26,15 +27,15 @@ export class FormSubmissionTests extends TurboDriveTestCase {
 
     this.assert.equal(await this.getAlertText(), "Are you sure?")
     await this.acceptAlert()
-    this.assert.ok(await this.formSubmitted)
+    this.assert.ok(await this.formSubmitStarted)
   }
 
   async "test form submission with confirmation cancelled"() {
     await this.clickSelector("#standard form.confirm input[type=submit]")
 
     this.assert.equal(await this.getAlertText(), "Are you sure?")
     await this.dismissAlert()
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.notOk(await this.formSubmitStarted)
   }
 
   async "test from submission with confirmation overriden"() {
@@ -44,7 +45,7 @@ export class FormSubmissionTests extends TurboDriveTestCase {
 
     this.assert.equal(await this.getAlertText(), "Overriden message")
     await this.acceptAlert()
-    this.assert.ok(await this.formSubmitted)
+    this.assert.ok(await this.formSubmitStarted)
   }
 
   async "test standard form submission does not render a progress bar before expiring the delay"() {
@@ -58,22 +59,64 @@ export class FormSubmissionTests extends TurboDriveTestCase {
     await this.clickSelector("#standard form.redirect input[type=submit]")
     await this.nextBody
 
-    this.assert.ok(await this.formSubmitted)
+    this.assert.ok(await this.formSubmitStarted)
     this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html")
     this.assert.equal(await this.visitAction, "advance")
     this.assert.equal(await this.getSearchParam("greeting"), "Hello from a redirect")
   }
 
+  async "test standard POST form submission events"() {
+    await this.clickSelector("#standard-post-form-submit")
+
+    this.assert.ok(await this.formSubmitStarted, "fires turbo:submit-start")
+
+    const { fetchOptions } = await this.nextEventNamed("turbo:before-fetch-request")
+
+    this.assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html"))
+
+    await this.nextEventNamed("turbo:before-fetch-response")
+
+    this.assert.ok(await this.formSubmitEnded, "fires turbo:submit-end")
+
+    await this.nextEventNamed("turbo:before-visit")
+    await this.nextEventNamed("turbo:visit")
+    await this.nextEventNamed("turbo:before-cache")
+    await this.nextEventNamed("turbo:before-render")
+    await this.nextEventNamed("turbo:render")
+    await this.nextEventNamed("turbo:load")
+  }
+
   async "test standard GET form submission"() {
     await this.clickSelector("#standard form.greeting input[type=submit]")
     await this.nextBody
 
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.ok(await this.formSubmitStarted)
     this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html")
-    this.assert.equal(await this.visitAction, "replace")
+    this.assert.equal(await this.visitAction, "advance")
     this.assert.equal(await this.getSearchParam("greeting"), "Hello from a form")
   }
 
+  async "test standard GET form submission events"() {
+    await this.clickSelector("#standard-get-form-submit")
+
+    this.assert.ok(await this.formSubmitStarted, "fires turbo:submit-start")
+
+    const { fetchOptions } = await this.nextEventNamed("turbo:before-fetch-request")
+
+    this.assert.notOk(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html"))
+
+    await this.nextEventNamed("turbo:before-fetch-response")
+
+    this.assert.ok(await this.formSubmitEnded, "fires turbo:submit-end")
+
+    await this.nextEventNamed("turbo:before-visit")
+    await this.nextEventNamed("turbo:visit")
+    await this.nextEventNamed("turbo:before-cache")
+    await this.nextEventNamed("turbo:before-render")
+    await this.nextEventNamed("turbo:render")
+    await this.nextEventNamed("turbo:load")
+  }
+
   async "test standard GET form submission appending keys"() {
     await this.goToLocation("/src/tests/fixtures/form.html?query=1")
     await this.clickSelector("#standard form.conflicting-values input[type=submit]")
@@ -281,6 +324,40 @@ export class FormSubmissionTests extends TurboDriveTestCase {
     this.assert.equal(await title.getVisibleText(), "One")
   }
 
+  async "test frame POST form targetting frame submission"() {
+    await this.clickSelector("#targets-frame-post-form-submit")
+
+    this.assert.ok(await this.formSubmitStarted, "fires turbo:submit-start")
+
+    const { fetchOptions } = await this.nextEventNamed("turbo:before-fetch-request")
+
+    this.assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html"))
+    this.assert.equal("frame", fetchOptions.headers["Turbo-Frame"])
+
+    await this.nextEventNamed("turbo:before-fetch-response")
+
+    this.assert.ok(await this.formSubmitEnded, "fires turbo:submit-end")
+
+    await this.nextEventNamed("turbo:frame-render")
+  }
+
+  async "test frame GET form targetting frame submission"() {
+    await this.clickSelector("#targets-frame-get-form-submit")
+
+    this.assert.ok(await this.formSubmitStarted, "fires turbo:submit-start")
+
+    const { fetchOptions } = await this.nextEventNamed("turbo:before-fetch-request")
+
+    this.assert.notOk(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html"))
+    this.assert.equal("frame", fetchOptions.headers["Turbo-Frame"])
+
+    await this.nextEventNamed("turbo:before-fetch-response")
+
+    this.assert.ok(await this.formSubmitEnded, "fires turbo:submit-end")
+
+    await this.nextEventNamed("turbo:frame-render")
+  }
+
   async "test frame form GET submission from submitter referencing another frame"() {
     await this.clickSelector("#frame form[method=get] [type=submit][data-turbo-frame=hello]")
     await this.nextBeat
@@ -409,49 +486,49 @@ export class FormSubmissionTests extends TurboDriveTestCase {
     await this.nextBody
     await this.querySelector("#element-id")
 
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.notOk(await this.formSubmitStarted)
   }
 
   async "test frame form submission with [data-turbo=false] on the submitter"() {
     await this.clickSelector('#frame form:not([data-turbo]) input[data-turbo="false"]')
     await this.nextBody
     await this.querySelector("#element-id")
 
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.notOk(await this.formSubmitStarted)
   }
 
   async "test form submission with [data-turbo=false] on the form"() {
     await this.clickSelector('#turbo-false form[data-turbo="false"] input[type=submit]')
     await this.nextBody
     await this.querySelector("#element-id")
 
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.notOk(await this.formSubmitStarted)
   }
 
   async "test form submission with [data-turbo=false] on the submitter"() {
     await this.clickSelector('#turbo-false form:not([data-turbo]) input[data-turbo="false"]')
     await this.nextBody
     await this.querySelector("#element-id")
 
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.notOk(await this.formSubmitStarted)
   }
 
   async "test form submission skipped within method=dialog"() {
     await this.clickSelector('#dialog-method [type="submit"]')
     await this.nextBeat
 
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.notOk(await this.formSubmitStarted)
   }
 
   async "test form submission skipped with submitter formmethod=dialog"() {
     await this.clickSelector('#dialog-formmethod [formmethod="dialog"]')
     await this.nextBeat
 
-    this.assert.notOk(await this.formSubmitted)
+    this.assert.notOk(await this.formSubmitStarted)
   }
 
   async "test form submission targets disabled frame"() {
-    this.remote.execute(() => document.getElementById("frame")?.setAttribute("disabled", ""))
+    await this.remote.execute(() => document.getElementById("frame")?.setAttribute("disabled", ""))
     await this.clickSelector('#targets-frame form.one [type="submit"]')
     await this.nextBody
 
@@ -574,8 +651,12 @@ export class FormSubmissionTests extends TurboDriveTestCase {
     this.assert.ok(await this.nextEventOnTarget("form_one", "turbo:before-fetch-response"))
   }
 
-  get formSubmitted(): Promise<boolean> {
-    return this.hasSelector("html[data-form-submitted]")
+  get formSubmitStarted(): Promise<boolean> {
+    return this.hasSelector("html[data-form-submit-start]")
+  }
+
+  get formSubmitEnded(): Promise<boolean> {
+    return this.hasSelector("html[data-form-submit-end]")
   }
 }