Resolve Frame-to-Page Visit event ordering

The problem --- By attempting to avoid unnecessary renders and events by introducing the `willRender:` Visit option, the initial `<turbo-frame data-turbo-action="...">` implementation was skipping several crucial lifecycle hooks. For example, the resulting Visit would fire a `turbo:render`, but would not fire a `turbo:load`. This left the `<html>` element in an inconsistent state without cleaning up any `[data-turbo-preview]` or `[aria-busy]` attribute modifications. The solution --- Forego the `willRender:` option, and instead propose a visit with a pre-populated `statusCode`, `redirected`, and `responseHTML` value so that the `Session` (including all of its hooks) can handle transparently the same as other `Visit` instances. The result is much simpler than the original implementation: a promoted Visit doesn't receive any specialized treatment, so it stands to benefits from all the existing plumbing.
hotwired · Nov 13, 2021 · c5479d4 · c5479d4
1 parent 33507e5
commit c5479d4
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 13 deletions.
diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -41,7 +41,6 @@ export type VisitOptions = {
   action: Action,
   delegate: Partial<VisitDelegate>
   historyChanged: boolean,
-  willRender: boolean
   referrer?: URL,
   snapshotHTML?: string,
   response?: VisitResponse
@@ -51,7 +50,6 @@ const defaultOptions: VisitOptions = {
   action: "advance",
   delegate: {},
   historyChanged: false,
-  willRender: true
 }
 
 export type VisitResponse = {
@@ -75,7 +73,6 @@ export class Visit implements FetchRequestDelegate {
   readonly timingMetrics: TimingMetrics = {}
   readonly optionalDelegate: Partial<VisitDelegate>
 
-  willRender: boolean
   followedRedirect = false
   frame?: number
   historyChanged = false
@@ -94,8 +91,7 @@ export class Visit implements FetchRequestDelegate {
     this.location = location
     this.restorationIdentifier = restorationIdentifier || uuid()
 
-    const { action, historyChanged, referrer, snapshotHTML, response, willRender, delegate: optionalDelegate } = { ...defaultOptions, ...options }
-    this.willRender = willRender
+    const { action, historyChanged, referrer, snapshotHTML, response, delegate: optionalDelegate } = { ...defaultOptions, ...options }
     this.action = action
     this.historyChanged = historyChanged
     this.referrer = referrer
@@ -211,7 +207,7 @@ export class Visit implements FetchRequestDelegate {
   }
 
   loadResponse() {
-    if (this.response && this.willRender) {
+    if (this.response) {
       const { statusCode, responseHTML } = this.response
       this.render(async () => {
         this.cacheSnapshot()

diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts
@@ -262,11 +262,14 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
 
     if (isAction(action)) {
       const delegate = new SnapshotSubstitution(frame)
-      const proposeVisit = () => {
-        if (frame.src) {
-          const snapshotHTML = frame.ownerDocument.documentElement.outerHTML
-
-          session.visit(frame.src, { willRender: false, action, snapshotHTML, delegate })
+      const proposeVisit = (event: Event) => {
+        const { target, detail: { fetchResponse } } = event as CustomEvent
+        if (target instanceof FrameElement && target.src) {
+          const { statusCode, redirected } = fetchResponse
+          const responseHTML = target.ownerDocument.documentElement.outerHTML
+          const response = { statusCode, redirected, responseHTML }
+
+          session.visit(target.src, { action, response, delegate })
         }
       }
 

diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts
@@ -282,6 +282,26 @@ export class FrameTests extends TurboDriveTestCase {
     this.assert.equal(requestLogs.length, 0)
   }
 
+  async "test navigating pushing URL state from a frame navigation fires events"() {
+    await this.clickSelector("#link-outside-frame-action-advance")
+
+    this.assert.equal(await this.nextAttributeMutationNamed("frame", "aria-busy"), "true", "sets aria-busy on the <turbo-frame>")
+    await this.nextEventOnTarget("frame", "turbo:before-fetch-request")
+    await this.nextEventOnTarget("frame", "turbo:before-fetch-response")
+    await this.nextEventOnTarget("html", "turbo:before-visit")
+    await this.nextEventOnTarget("html", "turbo:visit")
+    await this.nextEventOnTarget("frame", "turbo:frame-render")
+    await this.nextEventOnTarget("frame", "turbo:frame-load")
+    this.assert.notOk(await this.nextAttributeMutationNamed("frame", "aria-busy"), "removes aria-busy from the <turbo-frame>")
+
+    this.assert.equal(await this.nextAttributeMutationNamed("html", "aria-busy"), "true", "sets aria-busy on the <html>")
+    await this.nextEventOnTarget("html", "turbo:before-cache")
+    await this.nextEventOnTarget("html", "turbo:before-render")
+    await this.nextEventOnTarget("html", "turbo:render")
+    await this.nextEventOnTarget("html", "turbo:load")
+    this.assert.notOk(await this.nextAttributeMutationNamed("html", "aria-busy"), "removes aria-busy from the <html>")
+  }
+
   async "test navigating turbo-frame[data-turbo-action=advance] from within pushes URL state"() {
     await this.clickSelector("#add-turbo-action-to-frame")
     await this.clickSelector("#link-frame")
@@ -358,7 +378,7 @@ export class FrameTests extends TurboDriveTestCase {
   async "test navigating back after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames previous contents"() {
     await this.clickSelector("#add-turbo-action-to-frame")
     await this.clickSelector("#link-frame")
-    await this.nextBody
+    await this.nextEventNamed("turbo:load")
     await this.goBack()
     await this.nextBody
 
@@ -373,7 +393,7 @@ export class FrameTests extends TurboDriveTestCase {
   async "test navigating back then forward after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames next contents"() {
     await this.clickSelector("#add-turbo-action-to-frame")
     await this.clickSelector("#link-frame")
-    await this.nextBody
+    await this.nextEventNamed("turbo:load")
     await this.goBack()
     await this.nextBody
     await this.goForward()