Prevent double requests from within turbo-frame

seanpdoyle · seanpdoyle · commit 3db3efd9c20e · 2022-09-30T14:43:37.000-04:00
Closes #743 The change in behavior can be traced back to [#412][]. When the overlap between the `LinkInterceptor` and the `LinkClickObserver` were unified, the technique used by the `LinkInterceptor` to prevent duplicate event handlers from responding to the same `click` event was changed in a subtle way. In its place, this commit changes the `LinkClickObserver` and `FormSubmitObserver` to ignore `<a>` clicks and `<form>` submissions if they're annotated with `[data-remote="true"]`. To exercise these edge cases, this commit also adds a `ujs.html` fixture file along with a `ujs_tests.ts` module to cover situations when `@rails/ujs` and `@hotwired/turbo` co-exist. [#412]: #412
diff --git a/src/observers/form_submit_observer.ts b/src/observers/form_submit_observer.ts
@@ -1,3 +1,5 @@
+import { getAttribute } from "../util"
+
 export interface FormSubmitObserverDelegate {
   willSubmitForm(form: HTMLFormElement, submitter?: HTMLElement): boolean
   formSubmitted(form: HTMLFormElement, submitter?: HTMLElement): void
@@ -41,6 +43,7 @@ export class FormSubmitObserver {
         form &&
         submissionDoesNotDismissDialog(form, submitter) &&
         submissionDoesNotTargetIFrame(form, submitter) &&
+        submissionDoesNotIntegrateWithUJS(form, submitter) &&
         this.delegate.willSubmitForm(form, submitter)
       ) {
         event.preventDefault()
@@ -65,3 +68,10 @@ function submissionDoesNotTargetIFrame(form: HTMLFormElement, submitter?: HTMLEl
 
   return true
 }
+
+function submissionDoesNotIntegrateWithUJS(form: HTMLFormElement, submitter?: HTMLElement): boolean {
+  const value = getAttribute("data-remote", submitter, form)
+  const remote = /true/i.test(value || "")
+
+  return !remote
+}
diff --git a/src/observers/link_click_observer.ts b/src/observers/link_click_observer.ts
@@ -38,7 +38,7 @@ export class LinkClickObserver {
     if (event instanceof MouseEvent && this.clickEventIsSignificant(event)) {
       const target = (event.composedPath && event.composedPath()[0]) || event.target
       const link = this.findLinkFromClickTarget(target)
-      if (link && doesNotTargetIFrame(link)) {
+      if (link && doesNotTargetIFrame(link) && doesNotIntegrateWithUJS(link)) {
         const location = this.getLocationForLink(link)
         if (this.delegate.willFollowLinkToLocation(link, location, event)) {
           event.preventDefault()
@@ -78,3 +78,10 @@ function doesNotTargetIFrame(anchor: HTMLAnchorElement): boolean {
 
   return true
 }
+
+function doesNotIntegrateWithUJS(anchor: HTMLAnchorElement): boolean {
+  const value = anchor.getAttribute("data-remote")
+  const remote = /true/i.test(value || "")
+
+  return !remote
+}
diff --git a/src/tests/fixtures/ujs.html b/src/tests/fixtures/ujs.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Frame</title>
+    <script src="/src/tests/fixtures/test.js"></script>
+    <script type="module">
+      import Rails from "https://ga.jspm.io/npm:@rails/ujs@7.0.1/lib/assets/compiled/rails-ujs.js"
+
+      Rails.start()
+    </script>
+    <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
+  </head>
+  <body>
+    <turbo-frame id="frame">
+      <h2>Frames: #frame</h2>
+
+      <a data-remote="true" href="/src/tests/fixtures/frames/frame.html">navigate #frame to /src/tests/fixtures/frames/frame.html</a>
+      <form data-remote="true" action="/src/tests/fixtures/frames/frame.html">
+        <button>navigate #frame to /src/tests/fixtures/frames/frame.html</button>
+      </form>
+    </turbo-frame>
+  </body>
+</html>
diff --git a/src/tests/functional/ujs_tests.ts b/src/tests/functional/ujs_tests.ts
@@ -0,0 +1,33 @@
+import { Page, test } from "@playwright/test"
+import { assert } from "chai"
+import { noNextEventOnTarget } from "../helpers/page"
+
+test.beforeEach(async ({ page }) => {
+  await page.goto("/src/tests/fixtures/ujs.html")
+})
+
+test("test clicking a [data-remote=true] anchor within a turbo-frame", async ({ page }) => {
+  await assertRequestLimit(page, 1, async () => {
+    await page.click("#frame a[data-remote=true]")
+    await noNextEventOnTarget(page, "frame", "turbo:frame-load")
+
+    assert.equal(await page.textContent("#frame h2"), "Frames: #frame", "does not navigate the target frame")
+  })
+})
+
+test("test submitting a [data-remote=true] form within a turbo-frame", async ({ page }) => {
+  await assertRequestLimit(page, 1, async () => {
+    await page.click("#frame form[data-remote=true] button")
+    await noNextEventOnTarget(page, "frame", "turbo:frame-load")
+
+    assert.equal(await page.textContent("#frame h2"), "Frames: #frame", "does not navigate the target frame")
+  })
+})
+
+async function assertRequestLimit(page: Page, count: number, callback: () => Promise<void>) {
+  let requestsStarted = 0
+  await page.on("request", () => requestsStarted++)
+  await callback()
+
+  assert.equal(requestsStarted, 1, `only submits ${count} requests`)
+}
