Update the history before rendering promoted frames (#618)

* Add a updateHistory check to visit

* make frame change history before rendering

* add frame fixture

* extract history logic to History class

* maintain the same restorationIdentifier

* remove guard that was always true

* lint

* make history receive a method

* lint

* reset frame and action at each nav

* udpate ids to reflect each link

* extract getVisitAction and fix compiler issues

* lint

* add before-frame-render event and make the render pausable

* Add test showcasing the URL being updated first

* add pause/abort tests for frames

* update ids to match initial page

* remove before-frame-render event code since it was introduced in #431

* yarn.lock

* yarn.lock

src/core/drive/visit.ts

@@ -6,7 +6,7 @@ import { getAnchor } from "../url"
 import { Snapshot } from "../snapshot"
 import { PageSnapshot } from "./page_snapshot"
 import { Action } from "../types"
-import { uuid } from "../../util"
+import { getHistoryMethodForAction, uuid } from "../../util"
 import { PageView } from "./page_view"
 
 export interface VisitDelegate {
@@ -45,6 +45,8 @@ export type VisitOptions = {
   response?: VisitResponse
   visitCachedSnapshot(snapshot: Snapshot): void
   willRender: boolean
+  updateHistory: boolean
+  restorationIdentifier?: string
   shouldCacheSnapshot: boolean
 }
 
@@ -53,6 +55,7 @@ const defaultOptions: VisitOptions = {
   historyChanged: false,
   visitCachedSnapshot: () => {},
   willRender: true,
+  updateHistory: true,
   shouldCacheSnapshot: true,
 }
 
@@ -77,6 +80,7 @@ export class Visit implements FetchRequestDelegate {
   readonly timingMetrics: TimingMetrics = {}
   readonly visitCachedSnapshot: (snapshot: Snapshot) => void
   readonly willRender: boolean
+  readonly updateHistory: boolean
 
   followedRedirect = false
   frame?: number
@@ -110,6 +114,7 @@ export class Visit implements FetchRequestDelegate {
       response,
       visitCachedSnapshot,
       willRender,
+      updateHistory,
       shouldCacheSnapshot,
     } = {
       ...defaultOptions,
@@ -123,6 +128,7 @@ export class Visit implements FetchRequestDelegate {
     this.isSamePage = this.delegate.locationWithActionIsSamePage(this.location, this.action)
     this.visitCachedSnapshot = visitCachedSnapshot
     this.willRender = willRender
+    this.updateHistory = updateHistory
     this.scrolled = !willRender
     this.shouldCacheSnapshot = shouldCacheSnapshot
   }
@@ -187,9 +193,9 @@ export class Visit implements FetchRequestDelegate {
   }
 
   changeHistory() {
-    if (!this.historyChanged) {
+    if (!this.historyChanged && this.updateHistory) {
       const actionForHistory = this.location.href === this.referrer?.href ? "replace" : this.action
-      const method = this.getHistoryMethodForAction(actionForHistory)
+      const method = getHistoryMethodForAction(actionForHistory)
       this.history.update(method, this.location, this.restorationIdentifier)
       this.historyChanged = true
     }

src/core/frames/frame_controller.ts

@@ -7,7 +7,16 @@ import {
 import { FetchMethod, FetchRequest, FetchRequestDelegate, FetchRequestHeaders } from "../../http/fetch_request"
 import { FetchResponse } from "../../http/fetch_response"
 import { AppearanceObserver, AppearanceObserverDelegate } from "../../observers/appearance_observer"
-import { clearBusyState, dispatch, getAttribute, parseHTMLDocument, markAsBusy } from "../../util"
+import {
+  clearBusyState,
+  dispatch,
+  getAttribute,
+  parseHTMLDocument,
+  markAsBusy,
+  uuid,
+  getHistoryMethodForAction,
+  getVisitAction,
+} from "../../util"
 import { FormSubmission, FormSubmissionDelegate } from "../drive/form_submission"
 import { Snapshot } from "../snapshot"
 import { ViewDelegate, ViewRenderOptions } from "../view"
@@ -18,7 +27,8 @@ import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor"
 import { FormLinkInterceptor, FormLinkInterceptorDelegate } from "../../observers/form_link_interceptor"
 import { FrameRenderer } from "./frame_renderer"
 import { session } from "../index"
-import { isAction } from "../types"
+import { isAction, Action } from "../types"
+import { VisitOptions } from "../drive/visit"
 import { TurboBeforeFrameRenderEvent } from "../session"
 
 export class FrameController
@@ -45,6 +55,9 @@ export class FrameController
   private connected = false
   private hasBeenLoaded = false
   private ignoredAttributes: Set<FrameElementObservedAttribute> = new Set()
+  private action: Action | null = null
+  private frame?: FrameElement
+  readonly restorationIdentifier: string
   private previousFrameElement?: FrameElement
 
   constructor(element: FrameElement) {
@@ -53,6 +66,7 @@ export class FrameController
     this.appearanceObserver = new AppearanceObserver(this, this.element)
     this.formLinkInterceptor = new FormLinkInterceptor(this, this.element)
     this.linkInterceptor = new LinkInterceptor(this, this.element)
+    this.restorationIdentifier = uuid()
     this.formSubmitObserver = new FormSubmitObserver(this, this.element)
   }
 
@@ -141,6 +155,8 @@ export class FrameController
           false
         )
         if (this.view.renderPromise) await this.view.renderPromise
+        this.changeHistory()
+
         await this.view.render(renderer)
         this.complete = true
         session.frameRendered(fetchResponse, this.element)
@@ -328,28 +344,40 @@ export class FrameController
   }
 
   private proposeVisitIfNavigatedWithAction(frame: FrameElement, element: Element, submitter?: HTMLElement) {
-    const action = getAttribute("data-turbo-action", submitter, element, frame)
+    this.action = getVisitAction(submitter, element, frame)
+    this.frame = frame
 
-    if (isAction(action)) {
+    if (isAction(this.action)) {
       const { visitCachedSnapshot } = frame.delegate
 
       frame.delegate.fetchResponseLoaded = (fetchResponse: FetchResponse) => {
         if (frame.src) {
           const { statusCode, redirected } = fetchResponse
           const responseHTML = frame.ownerDocument.documentElement.outerHTML
           const response = { statusCode, redirected, responseHTML }
-
-          session.visit(frame.src, {
-            action,
+          const options: Partial<VisitOptions> = {
             response,
             visitCachedSnapshot,
             willRender: false,
-          })
+            updateHistory: false,
+            restorationIdentifier: this.restorationIdentifier,
+          }
+
+          if (this.action) options.action = this.action
+
+          session.visit(frame.src, options)
         }
       }
     }
   }
 
+  changeHistory() {
+    if (this.action && this.frame) {
+      const method = getHistoryMethodForAction(this.action)
+      session.history.update(method, expandURL(this.frame.src || ""), this.restorationIdentifier)
+    }
+  }
+
   private findFrameElement(element: Element, submitter?: HTMLElement) {
     const id = getAttribute("data-turbo-frame", submitter, element) || this.element.getAttribute("target")
     return getFrameElementById(id) ?? this.element

src/core/native/browser_adapter.ts

@@ -24,7 +24,7 @@ export class BrowserAdapter implements Adapter {
   }
 
   visitProposedToLocation(location: URL, options?: Partial<VisitOptions>) {
-    this.navigator.startVisit(location, uuid(), options)
+    this.navigator.startVisit(location, options?.restorationIdentifier || uuid(), options)
   }
 
   visitStarted(visit: Visit) {

src/tests/fixtures/tabs.html

@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Tabs</title>
+    <script src="/dist/turbo.es2017-umd.js"></script>
+    <script src="/src/tests/fixtures/test.js"></script>
+  </head>
+  <body>
+    <h1>Tabs</h1>
+
+    <turbo-frame id="tab-frame" data-turbo-action="advance">
+      <div>
+        <a id="tab-1" href="/src/tests/fixtures/tabs.html">Tab 1</a>
+        <a id="tab-2" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
+        <a id="tab-3" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
+      </div>
+
+      <div id="tab-content">One</div>
+    </turbo-frame>
+  </body>
+</html>

src/tests/fixtures/tabs/three.html

@@ -0,0 +1,9 @@
+<turbo-frame id="tab-frame" data-turbo-action="advance">
+  <div>
+    <a id="tabs-1" href="/src/tests/fixtures/tabs.html">Tab 1</a>
+    <a id="tabs-2" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
+    <a id="tabs-3" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
+  </div>
+
+  <div id="tab-content">Three</div>
+</turbo-frame>

src/tests/fixtures/tabs/two.html

@@ -0,0 +1,9 @@
+<turbo-frame id="tab-frame" data-turbo-action="advance">
+  <div>
+    <a id="tab-1" href="/src/tests/fixtures/tabs.html">Tab 1</a>
+    <a id="tab-2" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
+    <a id="tab-3" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
+  </div>
+
+  <div id="tab-content">Two</div>
+</turbo-frame>

src/tests/fixtures/test.js

@@ -47,6 +47,7 @@
   "turbo:before-fetch-request",
   "turbo:before-fetch-response",
   "turbo:visit",
+  "turbo:before-frame-render",
   "turbo:frame-load",
   "turbo:frame-render",
   "turbo:reload"

src/tests/functional/frame_navigation_tests.ts

@@ -1,5 +1,6 @@
 import { test } from "@playwright/test"
-import { nextEventOnTarget } from "../helpers/page"
+import { getFromLocalStorage, nextEventNamed, nextEventOnTarget, pathname } from "../helpers/page"
+import { assert } from "chai"
 
 test.beforeEach(async ({ page }) => {
   await page.goto("/src/tests/fixtures/frame_navigation.html")
@@ -22,3 +23,25 @@ test("test frame navigation with exterior link", async ({ page }) => {
 
   await nextEventOnTarget(page, "frame", "turbo:frame-load")
 })
+
+test("test promoted frame navigation updates the URL before rendering", async ({ page }) => {
+  await page.goto("/src/tests/fixtures/tabs.html")
+
+  page.evaluate(() => {
+    addEventListener("turbo:before-frame-render", () => {
+      localStorage.setItem("beforeRenderUrl", window.location.pathname)
+      localStorage.setItem("beforeRenderContent", document.querySelector("#tab-content")?.textContent || "")
+    })
+  })
+
+  await page.click("#tab-2")
+  await nextEventNamed(page, "turbo:before-frame-render")
+
+  assert.equal(await getFromLocalStorage(page, "beforeRenderUrl"), "/src/tests/fixtures/tabs/two.html")
+  assert.equal(await getFromLocalStorage(page, "beforeRenderContent"), "One")
+
+  await nextEventNamed(page, "turbo:frame-render")
+
+  assert.equal(await pathname(page.url()), "/src/tests/fixtures/tabs/two.html")
+  assert.equal(await page.textContent("#tab-content"), "Two")
+})

src/util.ts

@@ -1,3 +1,5 @@
+import { Action, isAction } from "./core/types"
+
 export type DispatchOptions<T extends CustomEvent> = {
   target: EventTarget
   cancelable: boolean
@@ -96,6 +98,22 @@ export function clearBusyState(...elements: Element[]) {
   }
 }
 
+export function getHistoryMethodForAction(action: Action) {
+  switch (action) {
+    case "replace":
+      return history.replaceState
+    case "advance":
+    case "restore":
+      return history.pushState
+  }
+}
+
+export function getVisitAction(...elements: (Element | undefined)[]): Action | null {
+  const action = getAttribute("data-turbo-action", ...elements)
+
+  return isAction(action) ? action : null
+}
+
 export function getMetaElement(name: string): HTMLMetaElement | null {
   return document.querySelector(`meta[name="${name}"]`)
 }