From 732db00a3a50a449854c00a2132ef27dc8d24e43 Mon Sep 17 00:00:00 2001
From: Sean Doyle <seanpdoyle@users.noreply.github.com>
Date: Tue, 27 Sep 2022 10:27:37 -0400
Subject: [PATCH] Fix double `before-fetch-request` dispatch during reload
 (#740)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes https://github.com/hotwired/turbo/issues/739

Issue #739 mentions the issue at the root of the unexpected behavior:

> The reason is that `reload()` calls `this.removeAttribute("complete")`
> and the frame watches attribute changes and calls
> `this.loadSourceURL()` when the “complete” attribute is removed.

This commit resolves the issue in several steps.

First, add the `sourceURLReloaded()` callback to the
`FrameElementDelegate` interface.

Next, move the `FrameElement.reload()` implementation from
`FrameElement.reload()` to the `FrameController.sourceURLReloaded()`.

Finally, wrap the implementation's call to
`this.element.removeAttribute("complete")` within a
`ignoringChangesToAttribute("complete", () => ...)` block so that the
change to the attribute is temporarily ignored.
---
 src/core/frames/frame_controller.ts | 10 ++++++++++
 src/elements/frame_element.ts       |  7 ++-----
 src/tests/functional/frame_tests.ts | 19 +++++++++++++++++++
 3 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts
index 036df862b..d94c5c005 100644
--- a/src/core/frames/frame_controller.ts
+++ b/src/core/frames/frame_controller.ts
@@ -117,6 +117,16 @@ export class FrameController
     }
   }
 
+  sourceURLReloaded() {
+    const { src } = this.element
+    this.ignoringChangesToAttribute("complete", () => {
+      this.element.removeAttribute("complete")
+    })
+    this.element.src = null
+    this.element.src = src
+    return this.element.loaded
+  }
+
   completeChanged() {
     if (this.isIgnoringChangesTo("complete")) return
 
diff --git a/src/elements/frame_element.ts b/src/elements/frame_element.ts
index a995da374..82a4b113a 100644
--- a/src/elements/frame_element.ts
+++ b/src/elements/frame_element.ts
@@ -16,6 +16,7 @@ export interface FrameElementDelegate extends LinkClickObserverDelegate, FormSub
   completeChanged(): void
   loadingStyleChanged(): void
   sourceURLChanged(): void
+  sourceURLReloaded(): Promise<void>
   disabledChanged(): void
   loadResponse(response: FetchResponse): void
   fetchResponseLoaded: (fetchResponse: FetchResponse) => void
@@ -63,11 +64,7 @@ export class FrameElement extends HTMLElement {
   }
 
   reload(): Promise<void> {
-    const { src } = this
-    this.removeAttribute("complete")
-    this.src = null
-    this.src = src
-    return this.loaded
+    return this.delegate.sourceURLReloaded()
   }
 
   attributeChangedCallback(name: string) {
diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts
index 6b675aef4..8560a1560 100644
--- a/src/tests/functional/frame_tests.ts
+++ b/src/tests/functional/frame_tests.ts
@@ -492,6 +492,25 @@ test("test navigating a frame targeting _top from an outer link fires events", a
   assert.equal(otherEvents.length, 0, "no more events")
 })
 
+test("test invoking .reload() re-fetches the frame's content", async ({ page }) => {
+  await page.click("#link-frame")
+  await nextEventOnTarget(page, "frame", "turbo:frame-load")
+  await page.evaluate(() => (document.getElementById("frame") as any).reload())
+
+  const dispatchedEvents = await readEventLogs(page)
+
+  assert.deepEqual(
+    dispatchedEvents.map(([name, _, id]) => [id, name]),
+    [
+      ["frame", "turbo:before-fetch-request"],
+      ["frame", "turbo:before-fetch-response"],
+      ["frame", "turbo:before-frame-render"],
+      ["frame", "turbo:frame-render"],
+      ["frame", "turbo:frame-load"],
+    ]
+  )
+})
+
 test("test following inner link reloads frame on every click", async ({ page }) => {
   await page.click("#hello a")
   await nextEventNamed(page, "turbo:before-fetch-request")