Ignore UJS <a> clicks and <form> submissions (#744)

* Prevent double requests from within `turbo-frame`

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

* Revert "Replace LinkInterceptor with LinkClickObserver"

This reverts commit 9bef653.

* adjust after reverting #412

* Restore `<form data-remote="true">`+`<turbo-frame>` behavior from 7.1.0

[comment]: #744 (comment)
[FormInterceptor]: https://github.com/hotwired/turbo/blob/v7.1.0/src/core/frames/form_interceptor.ts#L31

src/core/frames/frame_controller.ts

@@ -23,10 +23,10 @@ import { ViewDelegate, ViewRenderOptions } from "../view"
 import { Locatable, getAction, expandURL, urlsAreEqual, locationIsVisitable } from "../url"
 import { FormSubmitObserver, FormSubmitObserverDelegate } from "../../observers/form_submit_observer"
 import { FrameView } from "./frame_view"
-import { LinkClickObserver, LinkClickObserverDelegate } from "../../observers/link_click_observer"
+import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor"
 import { FormLinkClickObserver, FormLinkClickObserverDelegate } from "../../observers/form_link_click_observer"
 import { FrameRenderer } from "./frame_renderer"
-import { TurboClickEvent, session } from "../index"
+import { session } from "../index"
 import { isAction, Action } from "../types"
 import { VisitOptions } from "../drive/visit"
 import { TurboBeforeFrameRenderEvent } from "../session"
@@ -43,14 +43,14 @@ export class FrameController
     FormSubmissionDelegate,
     FrameElementDelegate,
     FormLinkClickObserverDelegate,
-    LinkClickObserverDelegate,
+    LinkInterceptorDelegate,
     ViewDelegate<FrameElement, Snapshot<FrameElement>>
 {
   readonly element: FrameElement
   readonly view: FrameView
   readonly appearanceObserver: AppearanceObserver
   readonly formLinkClickObserver: FormLinkClickObserver
-  readonly linkClickObserver: LinkClickObserver
+  readonly linkInterceptor: LinkInterceptor
   readonly formSubmitObserver: FormSubmitObserver
   formSubmission?: FormSubmission
   fetchResponseLoaded = (_fetchResponse: FetchResponse) => {}
@@ -70,7 +70,7 @@ export class FrameController
     this.view = new FrameView(this, this.element)
     this.appearanceObserver = new AppearanceObserver(this, this.element)
     this.formLinkClickObserver = new FormLinkClickObserver(this, this.element)
-    this.linkClickObserver = new LinkClickObserver(this, this.element)
+    this.linkInterceptor = new LinkInterceptor(this, this.element)
     this.restorationIdentifier = uuid()
     this.formSubmitObserver = new FormSubmitObserver(this, this.element)
   }
@@ -84,7 +84,7 @@ export class FrameController
         this.loadSourceURL()
       }
       this.formLinkClickObserver.start()
-      this.linkClickObserver.start()
+      this.linkInterceptor.start()
       this.formSubmitObserver.start()
     }
   }
@@ -94,7 +94,7 @@ export class FrameController
       this.connected = false
       this.appearanceObserver.stop()
       this.formLinkClickObserver.stop()
-      this.linkClickObserver.stop()
+      this.linkInterceptor.stop()
       this.formSubmitObserver.stop()
     }
   }
@@ -204,22 +204,22 @@ export class FrameController
   // Form link click observer delegate
 
   willSubmitFormLinkToLocation(link: Element): boolean {
-    return link.closest("turbo-frame") == this.element && this.shouldInterceptNavigation(link)
+    return this.shouldInterceptNavigation(link)
   }
 
   submittedFormLinkToLocation(link: Element, _location: URL, form: HTMLFormElement): void {
     const frame = this.findFrameElement(link)
     if (frame) form.setAttribute("data-turbo-frame", frame.id)
   }
 
-  // Link click observer delegate
+  // Link interceptor delegate
 
-  willFollowLinkToLocation(element: Element, location: URL, event: MouseEvent) {
-    return this.shouldInterceptNavigation(element) && this.frameAllowsVisitingLocation(element, location, event)
+  shouldInterceptLinkClick(element: Element, _location: string, _event: MouseEvent) {
+    return this.shouldInterceptNavigation(element)
   }
 
-  followedLinkToLocation(element: Element, location: URL) {
-    this.navigateFrame(element, location.href)
+  linkClickIntercepted(element: Element, location: string) {
+    this.navigateFrame(element, location)
   }
 
   // Form submit observer delegate
@@ -555,16 +555,6 @@ export class FrameController
     return expandURL(root)
   }
 
-  private frameAllowsVisitingLocation(target: Element, { href: url }: URL, originalEvent: MouseEvent): boolean {
-    const event = dispatch<TurboClickEvent>("turbo:click", {
-      target,
-      detail: { url, originalEvent },
-      cancelable: true,
-    })
-
-    return !event.defaultPrevented
-  }
-
   private isIgnoringChangesTo(attributeName: FrameElementObservedAttribute): boolean {
     return this.ignoredAttributes.has(attributeName)
   }

src/core/frames/frame_redirector.ts

@@ -1,41 +1,39 @@
 import { FormSubmitObserver, FormSubmitObserverDelegate } from "../../observers/form_submit_observer"
 import { FrameElement } from "../../elements/frame_element"
+import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor"
 import { expandURL, getAction, locationIsVisitable } from "../url"
-import { LinkClickObserver, LinkClickObserverDelegate } from "../../observers/link_click_observer"
-import { Session, TurboClickEvent } from "../session"
-import { dispatch } from "../../util"
-
-export class FrameRedirector implements LinkClickObserverDelegate, FormSubmitObserverDelegate {
+import { Session } from "../session"
+export class FrameRedirector implements LinkInterceptorDelegate, FormSubmitObserverDelegate {
   readonly session: Session
   readonly element: Element
-  readonly linkClickObserver: LinkClickObserver
+  readonly linkInterceptor: LinkInterceptor
   readonly formSubmitObserver: FormSubmitObserver
 
   constructor(session: Session, element: Element) {
     this.session = session
     this.element = element
-    this.linkClickObserver = new LinkClickObserver(this, element)
+    this.linkInterceptor = new LinkInterceptor(this, element)
     this.formSubmitObserver = new FormSubmitObserver(this, element)
   }
 
   start() {
-    this.linkClickObserver.start()
+    this.linkInterceptor.start()
     this.formSubmitObserver.start()
   }
 
   stop() {
-    this.linkClickObserver.stop()
+    this.linkInterceptor.stop()
     this.formSubmitObserver.stop()
   }
 
-  willFollowLinkToLocation(element: Element, location: URL, event: MouseEvent) {
-    return this.shouldRedirect(element) && this.frameAllowsVisitingLocation(element, location, event)
+  shouldInterceptLinkClick(element: Element, _location: string, _event: MouseEvent) {
+    return this.shouldRedirect(element)
   }
 
-  followedLinkToLocation(element: Element, url: URL) {
+  linkClickIntercepted(element: Element, url: string, event: MouseEvent) {
     const frame = this.findFrameElement(element)
     if (frame) {
-      frame.delegate.followedLinkToLocation(element, url)
+      frame.delegate.linkClickIntercepted(element, url, event)
     }
   }
 
@@ -54,16 +52,6 @@ export class FrameRedirector implements LinkClickObserverDelegate, FormSubmitObs
     }
   }
 
-  private frameAllowsVisitingLocation(target: Element, { href: url }: URL, originalEvent: MouseEvent): boolean {
-    const event = dispatch<TurboClickEvent>("turbo:click", {
-      target,
-      detail: { url, originalEvent },
-      cancelable: true,
-    })
-
-    return !event.defaultPrevented
-  }
-
   private shouldSubmit(form: HTMLFormElement, submitter?: HTMLElement) {
     const action = getAction(form, submitter)
     const meta = this.element.ownerDocument.querySelector<HTMLMetaElement>(`meta[name="turbo-root"]`)

src/core/frames/link_interceptor.ts

@@ -0,0 +1,57 @@
+import { TurboClickEvent, TurboBeforeVisitEvent } from "../session"
+
+export interface LinkInterceptorDelegate {
+  shouldInterceptLinkClick(element: Element, url: string, originalEvent: MouseEvent): boolean
+  linkClickIntercepted(element: Element, url: string, originalEvent: MouseEvent): void
+}
+
+export class LinkInterceptor {
+  readonly delegate: LinkInterceptorDelegate
+  readonly element: Element
+  private clickEvent?: Event
+
+  constructor(delegate: LinkInterceptorDelegate, element: Element) {
+    this.delegate = delegate
+    this.element = element
+  }
+
+  start() {
+    this.element.addEventListener("click", this.clickBubbled)
+    document.addEventListener("turbo:click", this.linkClicked)
+    document.addEventListener("turbo:before-visit", this.willVisit)
+  }
+
+  stop() {
+    this.element.removeEventListener("click", this.clickBubbled)
+    document.removeEventListener("turbo:click", this.linkClicked)
+    document.removeEventListener("turbo:before-visit", this.willVisit)
+  }
+
+  clickBubbled = (event: Event) => {
+    if (this.respondsToEventTarget(event.target)) {
+      this.clickEvent = event
+    } else {
+      delete this.clickEvent
+    }
+  }
+
+  linkClicked = <EventListener>((event: TurboClickEvent) => {
+    if (this.clickEvent && this.respondsToEventTarget(event.target) && event.target instanceof Element) {
+      if (this.delegate.shouldInterceptLinkClick(event.target, event.detail.url, event.detail.originalEvent)) {
+        this.clickEvent.preventDefault()
+        event.preventDefault()
+        this.delegate.linkClickIntercepted(event.target, event.detail.url, event.detail.originalEvent)
+      }
+    }
+    delete this.clickEvent
+  })
+
+  willVisit = <EventListener>((_event: TurboBeforeVisitEvent) => {
+    delete this.clickEvent
+  })
+
+  respondsToEventTarget(target: EventTarget | null) {
+    const element = target instanceof Element ? target : target instanceof Node ? target.parentElement : null
+    return element && element.closest("turbo-frame, html") == this.element
+  }
+}

src/elements/frame_element.ts

@@ -1,6 +1,6 @@
 import { FetchResponse } from "../http/fetch_response"
 import { Snapshot } from "../core/snapshot"
-import { LinkClickObserverDelegate } from "../observers/link_click_observer"
+import { LinkInterceptorDelegate } from "../core/frames/link_interceptor"
 import { FormSubmitObserverDelegate } from "../observers/form_submit_observer"
 
 export enum FrameLoadingStyle {
@@ -10,7 +10,7 @@ export enum FrameLoadingStyle {
 
 export type FrameElementObservedAttribute = keyof FrameElement & ("disabled" | "complete" | "loading" | "src")
 
-export interface FrameElementDelegate extends LinkClickObserverDelegate, FormSubmitObserverDelegate {
+export interface FrameElementDelegate extends LinkInterceptorDelegate, FormSubmitObserverDelegate {
   connect(): void
   disconnect(): void
   completeChanged(): void

src/observers/form_link_click_observer.ts

@@ -6,20 +6,20 @@ export type FormLinkClickObserverDelegate = {
 }
 
 export class FormLinkClickObserver implements LinkClickObserverDelegate {
-  readonly linkClickObserver: LinkClickObserver
+  readonly linkInterceptor: LinkClickObserver
   readonly delegate: FormLinkClickObserverDelegate
 
   constructor(delegate: FormLinkClickObserverDelegate, element: HTMLElement) {
     this.delegate = delegate
-    this.linkClickObserver = new LinkClickObserver(this, element)
+    this.linkInterceptor = new LinkClickObserver(this, element)
   }
 
   start() {
-    this.linkClickObserver.start()
+    this.linkInterceptor.start()
   }
 
   stop() {
-    this.linkClickObserver.stop()
+    this.linkInterceptor.stop()
   }
 
   willFollowLinkToLocation(link: Element, location: URL, originalEvent: MouseEvent): boolean {

src/observers/form_submit_observer.ts

@@ -44,6 +44,7 @@ export class FormSubmitObserver {
         this.delegate.willSubmitForm(form, submitter)
       ) {
         event.preventDefault()
+        event.stopImmediatePropagation()
         this.delegate.formSubmitted(form, submitter)
       }
     }

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>

src/tests/functional/frame_tests.ts

@@ -2,9 +2,9 @@ import { Page, test } from "@playwright/test"
 import { assert, Assertion } from "chai"
 import {
   attributeForSelector,
-  cancelNextEvent,
   hasSelector,
   innerHTMLForSelector,
+  listenForEventOnTarget,
   nextAttributeMutationNamed,
   noNextAttributeMutationNamed,
   nextBeat,
@@ -442,6 +442,7 @@ test("test navigating a frame from an outer form fires events", async ({ page })
 })
 
 test("test navigating a frame from an outer link fires events", async ({ page }) => {
+  await listenForEventOnTarget(page, "outside-frame-form", "turbo:click")
   await page.click("#outside-frame-form")
 
   await nextEventOnTarget(page, "outside-frame-form", "turbo:click")
@@ -456,14 +457,8 @@ test("test navigating a frame from an outer link fires events", async ({ page })
   assert.equal(otherEvents.length, 0, "no more events")
 })
 
-test("test canceling a turbo:cilck event falls back to built-in browser navigation", async ({ page }) => {
-  await cancelNextEvent(page, "turbo:click")
-  await Promise.all([page.waitForNavigation(), page.click("#link-frame")])
-
-  assert.equal(pathname(page.url()), "/src/tests/fixtures/frames/frame.html")
-})
-
 test("test navigating a frame from an inner link fires events", async ({ page }) => {
+  await listenForEventOnTarget(page, "link-frame", "turbo:click")
   await page.click("#link-frame")
 
   await nextEventOnTarget(page, "link-frame", "turbo:click")
@@ -479,6 +474,7 @@ test("test navigating a frame from an inner link fires events", async ({ page })
 })
 
 test("test navigating a frame targeting _top from an outer link fires events", async ({ page }) => {
+  await listenForEventOnTarget(page, "outside-navigate-top-link", "turbo:click")
   await page.click("#outside-navigate-top-link")
 
   await nextEventOnTarget(page, "outside-navigate-top-link", "turbo:click")

src/tests/functional/ujs_tests.ts

@@ -0,0 +1,37 @@
+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 () => {
+    assert.equal(await page.textContent("#frame h2"), "Frames: #frame")
+
+    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 () => {
+    assert.equal(await page.textContent("#frame h2"), "Frames: #frame")
+
+    await page.click("#frame form[data-remote=true] button")
+    await noNextEventOnTarget(page, "frame", "turbo:frame-load")
+
+    assert.equal(await page.textContent("#frame h2"), "Frame: Loaded", "navigates 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, count, `only submits ${count} requests`)
+}