Preserve page state while promoting Frame-to-Visit

The problem --- The changes made in [444][] removed the `willRender:` Visit option in favor of allowing Frame-to-Visit navigations to participate in the entire Visit Rendering, Snapshot Caching, and History navigating pipeline. The way that the `willRender:` guard clause was removed caused new issues in how Frame-to-Visit navigations were treated. Removing the outer conditional without replacing it with matching checks elsewhere has caused Frame-to-Visit navigations to re-render the entire page, and losing the current contextual state like scroll, focus or anything else that exists outside the `<turbo-frame>` element. Similarly, the nature of the `FrameController.proposeVisitIfNavigatedWithAction()` helper resulted in an out-of-order dispatching of `turbo:` and `turbo:frame-` events, and resulted in `turbo:before-visit and `turbo:visit` events firing before `turbo:frame-render` and `turbo:frame-load` events. The solution --- To resolve the rendering issues, this commit re-introduces the `willRender:` option (originally introduced in [398][] and removed in [444][]). The option is captured in the `Visit` constructor and passed along the constructed `PageRenderer`. This commit adds the `willRender:` property to the `PageRenderer` class, which defaults to `true` unless specified as an argument. During `PageRenderer.render()` calls, the `replaceBody()` call is only made if `willRender == true`. To integrate with caching, this commit invokes the `VisitDelegate.visitCachedSnapshot()` callback with the `Snapshot` instance that is written to the `PageView.snapshotCache` so that the `FrameController` can manage the before- and after-navigation HTML to enable integration with navigating back and forward through the browser's history. To re-order the events, this commit replaces the `frame.addEventListener("turbo:frame-render")` attachment with a one-off `fetchResponseLoaded(FetchResponse)` callback that is assigned and reset during the frame navigation. When present, that callback is invoked _after_ the `turbo:load` event fires, which results in a much more expected event order: `turbo:before-fetch-request`, `turbo:before-fetch-response`, and `turbo:frame-` events fire first, then the rest of the Visit's events fire. The `fetchResponseLoaded(FetchResponse)` callback is an improvement, but is still an awkward way to coordinate between the `formSubmissionIntercepted()` and `linkClickIntercepted()` delegate methods, the `FrameController` instance, and the `Session` instance. It's functional for now, and we'll likely have a change to improve it with work like what's proposed in [430][] (which we can take on while developing `7.2.0`). To ensure this behavior, this commit adds several new types of tests, including coverage to make sure that the frame navigations can be transformed into page Visits without lasting consequences to the `<turbo-frame>` element. Similarly, another test ensures the preservation of scroll state and input text state after a Frame-to-Visit navigation. There is one quirk worth highlighting: the `FrameTests` seem incapable of using Selenium to serialize the `{ detail: { newBody: <body> } }` value out of the driven Browser's environment and into the Test harness environment. The event itself fires, but references a detached element or instance that results in a [Stale Element Reference][]. To work around that issue while delivering the bug fixes, this commit alters the `frame.html` page's `<html>` to opt-out of serializing those events' `event.detail` object (handled in [src/tests/fixtures/test.js](./src/tests/fixtures/test.js)). All other tests that assert about `turbo:` events (with `this.nextEventNamed` or `this.nextEventOnTarget`) will continue to behave as normal, the `FrameTests` is the sole exception. [398]: hotwired#398 [430]: hotwired#430 [441]: hotwired#441 [444]: hotwired#444 [Stale Element Reference]: https://developer.mozilla.org/en-US/docs/Web/WebDriver/Errors/StaleElementReference
seanpdoyle · Nov 16, 2021 · 394f202 · 394f202
1 parent ca1117b
commit 394f202
Show file tree

Hide file tree

Showing 11 changed files with 158 additions and 58 deletions.
diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts
@@ -133,10 +133,6 @@ export class Navigator {
     this.delegate.visitCompleted(visit)
   }
 
-  visitCachedSnapshot(visit: Visit) {
-    this.delegate.visitCachedSnapshot(visit)
-  }
-
   locationWithActionIsSamePage(location: URL, action?: Action): boolean {
     const anchor = getAnchor(location)
     const currentAnchor = getAnchor(this.view.lastRenderedLocation)

diff --git a/src/core/drive/page_renderer.ts b/src/core/drive/page_renderer.ts
@@ -11,7 +11,9 @@ export class PageRenderer extends Renderer<HTMLBodyElement, PageSnapshot> {
   }
 
   async render() {
-    this.replaceBody()
+    if (this.willRender) {
+      this.replaceBody()
+    }
   }
 
   finishRendering() {

diff --git a/src/core/drive/page_view.ts b/src/core/drive/page_view.ts
@@ -15,8 +15,8 @@ export class PageView extends View<Element, PageSnapshot, PageViewRenderer, Page
   readonly snapshotCache = new SnapshotCache(10)
   lastRenderedLocation = new URL(location.href)
 
-  renderPage(snapshot: PageSnapshot, isPreview = false) {
-    const renderer = new PageRenderer(this.snapshot, snapshot, isPreview)
+  renderPage(snapshot: PageSnapshot, isPreview = true, willRender = true) {
+    const renderer = new PageRenderer(this.snapshot, snapshot, isPreview, willRender)
     return this.render(renderer)
   }
 
@@ -34,7 +34,9 @@ export class PageView extends View<Element, PageSnapshot, PageViewRenderer, Page
       this.delegate.viewWillCacheSnapshot()
       const { snapshot, lastRenderedLocation: location } = this
       await nextEventLoopTick()
-      this.snapshotCache.put(location, snapshot.clone())
+      const cachedSnapshot = snapshot.clone()
+      this.snapshotCache.put(location, cachedSnapshot)
+      return cachedSnapshot
     }
   }
 

diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -15,7 +15,6 @@ export interface VisitDelegate {
 
   visitStarted(visit: Visit): void
   visitCompleted(visit: Visit): void
-  visitCachedSnapshot(visit: Visit): void
   locationWithActionIsSamePage(location: URL, action: Action): boolean
   visitScrolledToSamePageLocation(oldURL: URL, newURL: URL): void
 }
@@ -39,17 +38,19 @@ export enum VisitState {
 
 export type VisitOptions = {
   action: Action,
-  delegate: Partial<VisitDelegate>
   historyChanged: boolean,
   referrer?: URL,
   snapshotHTML?: string,
   response?: VisitResponse
+  visitCachedSnapshot(snapshot: PageSnapshot): void
+  willRender: boolean
 }
 
 const defaultOptions: VisitOptions = {
   action: "advance",
-  delegate: {},
   historyChanged: false,
+  visitCachedSnapshot: () => {},
+  willRender: true,
 }
 
 export type VisitResponse = {
@@ -71,7 +72,8 @@ export class Visit implements FetchRequestDelegate {
   readonly action: Action
   readonly referrer?: URL
   readonly timingMetrics: TimingMetrics = {}
-  readonly optionalDelegate: Partial<VisitDelegate>
+  readonly visitCachedSnapshot: (snapshot: PageSnapshot) => void
+  readonly willRender: boolean
 
   followedRedirect = false
   frame?: number
@@ -91,14 +93,16 @@ export class Visit implements FetchRequestDelegate {
     this.location = location
     this.restorationIdentifier = restorationIdentifier || uuid()
 
-    const { action, historyChanged, referrer, snapshotHTML, response, delegate: optionalDelegate } = { ...defaultOptions, ...options }
+    const { action, historyChanged, referrer, snapshotHTML, response, visitCachedSnapshot, willRender } = { ...defaultOptions, ...options }
     this.action = action
     this.historyChanged = historyChanged
     this.referrer = referrer
     this.snapshotHTML = snapshotHTML
     this.response = response
     this.isSamePage = this.delegate.locationWithActionIsSamePage(this.location, this.action)
-    this.optionalDelegate = optionalDelegate
+    this.visitCachedSnapshot = visitCachedSnapshot
+    this.willRender = willRender
+    this.scrolled = !willRender
   }
 
   get adapter() {
@@ -127,7 +131,6 @@ export class Visit implements FetchRequestDelegate {
       this.state = VisitState.started
       this.adapter.visitStarted(this)
       this.delegate.visitStarted(this)
-      if (this.optionalDelegate.visitStarted) this.optionalDelegate.visitStarted(this)
     }
   }
 
@@ -213,7 +216,7 @@ export class Visit implements FetchRequestDelegate {
         this.cacheSnapshot()
         if (this.view.renderPromise) await this.view.renderPromise
         if (isSuccessful(statusCode) && responseHTML != null) {
-          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML))
+          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender)
           this.adapter.visitRendered(this)
           this.complete()
         } else {
@@ -255,7 +258,7 @@ export class Visit implements FetchRequestDelegate {
           this.adapter.visitRendered(this)
         } else {
           if (this.view.renderPromise) await this.view.renderPromise
-          await this.view.renderPage(snapshot, isPreview)
+          await this.view.renderPage(snapshot, isPreview, this.willRender)
           this.adapter.visitRendered(this)
           if (!isPreview) {
             this.complete()
@@ -386,15 +389,14 @@ export class Visit implements FetchRequestDelegate {
     } else if (this.action == "restore") {
       return !this.hasCachedSnapshot()
     } else {
-      return true
+      return this.willRender
     }
   }
 
   cacheSnapshot() {
     if (!this.snapshotCached) {
-      this.view.cacheSnapshot()
+      this.view.cacheSnapshot().then(snapshot => snapshot && this.visitCachedSnapshot(snapshot))
       this.snapshotCached = true
-      if (this.optionalDelegate.visitCachedSnapshot) this.optionalDelegate.visitCachedSnapshot(this)
     }
   }
 

diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts
@@ -4,7 +4,7 @@ import { FetchResponse } from "../../http/fetch_response"
 import { AppearanceObserver, AppearanceObserverDelegate } from "../../observers/appearance_observer"
 import { clearBusyState, getAttribute, parseHTMLDocument, markAsBusy } from "../../util"
 import { FormSubmission, FormSubmissionDelegate } from "../drive/form_submission"
-import { Visit, VisitDelegate } from "../drive/visit"
+import { PageSnapshot } from "../drive/page_snapshot"
 import { Snapshot } from "../snapshot"
 import { ViewDelegate } from "../view"
 import { getAction, expandURL, urlsAreEqual, locationIsVisitable, Locatable } from "../url"
@@ -23,6 +23,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   readonly formInterceptor: FormInterceptor
   currentURL?: string | null
   formSubmission?: FormSubmission
+  fetchResponseLoaded = (fetchResponse: FetchResponse) => {}
   private currentFetchRequest: FetchRequest | null = null
   private resolveVisitPromise = () => {}
   private connected = false
@@ -108,15 +109,18 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
       if (html) {
         const { body } = parseHTMLDocument(html)
         const snapshot = new Snapshot(await this.extractForeignFrameElement(body))
-        const renderer = new FrameRenderer(this.view.snapshot, snapshot, false)
+        const renderer = new FrameRenderer(this.view.snapshot, snapshot, false, false)
         if (this.view.renderPromise) await this.view.renderPromise
         await this.view.render(renderer)
         session.frameRendered(fetchResponse, this.element)
         session.frameLoaded(this.element)
+        this.fetchResponseLoaded(fetchResponse)
       }
     } catch (error) {
       console.error(error)
       this.view.invalidate()
+    } finally {
+      this.fetchResponseLoaded = () => {}
     }
   }
 
@@ -261,19 +265,16 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
     const action = getAttribute("data-turbo-action", submitter, element, frame)
 
     if (isAction(action)) {
-      const delegate = new SnapshotSubstitution(frame)
-      const proposeVisit = (event: Event) => {
-        const { target, detail: { fetchResponse } } = event as CustomEvent
-        if (target instanceof FrameElement && target.src) {
+      const { visitCachedSnapshot } = new SnapshotSubstitution(frame)
+      frame.delegate.fetchResponseLoaded = (fetchResponse: FetchResponse) => {
+        if (frame.src) {
           const { statusCode, redirected } = fetchResponse
-          const responseHTML = target.ownerDocument.documentElement.outerHTML
+          const responseHTML = frame.ownerDocument.documentElement.outerHTML
           const response = { statusCode, redirected, responseHTML }
 
-          session.visit(target.src, { action, response, delegate })
+          session.visit(frame.src, { action, response, visitCachedSnapshot, willRender: false })
         }
       }
-
-      frame.addEventListener("turbo:frame-render", proposeVisit , { once: true })
     }
   }
 
@@ -395,26 +396,19 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   }
 }
 
-class SnapshotSubstitution implements Partial<VisitDelegate> {
+class SnapshotSubstitution {
   private readonly clone: Node
   private readonly id: string
-  private snapshot?: Snapshot
 
   constructor(element: FrameElement) {
     this.clone = element.cloneNode(true)
     this.id = element.id
   }
 
-  visitStarted(visit: Visit) {
-    this.snapshot = visit.view.snapshot
-  }
-
-  visitCachedSnapshot() {
-    const { snapshot, id, clone } = this
+  visitCachedSnapshot = ({ element }: PageSnapshot) => {
+    const { id, clone } = this
 
-    if (snapshot) {
-      snapshot.element.querySelector("#" + id)?.replaceWith(clone)
-    }
+    element.querySelector("#" + id)?.replaceWith(clone)
   }
 }
 

diff --git a/src/core/renderer.ts b/src/core/renderer.ts
@@ -10,13 +10,15 @@ export abstract class Renderer<E extends Element, S extends Snapshot<E> = Snapsh
   readonly currentSnapshot: S
   readonly newSnapshot: S
   readonly isPreview: boolean
+  readonly willRender: boolean
   readonly promise: Promise<void>
   private resolvingFunctions?: ResolvingFunctions<void>
 
-  constructor(currentSnapshot: S, newSnapshot: S, isPreview: boolean) {
+  constructor(currentSnapshot: S, newSnapshot: S, isPreview: boolean, willRender = true) {
     this.currentSnapshot = currentSnapshot
     this.newSnapshot = newSnapshot
     this.isPreview = isPreview
+    this.willRender = willRender
     this.promise = new Promise((resolve, reject) => this.resolvingFunctions = { resolve, reject })
   }
 

diff --git a/src/core/session.ts b/src/core/session.ts
@@ -189,9 +189,6 @@ export class Session implements FormSubmitObserverDelegate, HistoryDelegate, Lin
     this.notifyApplicationAfterPageLoad(visit.getTimingMetrics())
   }
 
-  visitCachedSnapshot(visit: Visit) {
-  }
-
   locationWithActionIsSamePage(location: URL, action?: Action): boolean {
     return this.navigator.locationWithActionIsSamePage(location, action)
   }

diff --git a/src/elements/frame_element.ts b/src/elements/frame_element.ts
@@ -11,6 +11,7 @@ export interface FrameElementDelegate {
   formSubmissionIntercepted(element: HTMLFormElement, submitter?: HTMLElement): void
   linkClickIntercepted(element: Element, url: string): void
   loadResponse(response: FetchResponse): void
+  fetchResponseLoaded: (fetchResponse: FetchResponse) => void
   isLoading: boolean
 }
 

diff --git a/src/tests/fixtures/frames.html b/src/tests/fixtures/frames.html
@@ -1,5 +1,5 @@
 <!DOCTYPE html>
-<html id="html">
+<html id="html" data-skip-event-details="turbo:before-render">
   <head>
     <meta charset="utf-8">
     <title>Frame</title>
@@ -9,9 +9,14 @@
       addEventListener("click", ({ target }) => {
         if (target.id == "add-turbo-action-to-frame") {
           target.closest("turbo-frame")?.setAttribute("data-turbo-action", "advance")
+        } else if (target.id == "remove-target-from-hello") {
+          document.getElementById("hello").removeAttribute("target")
         }
       })
     </script>
+    <style>
+      .push-off-screen { margin-top: 1000px; }
+    </style>
   </head>
   <body>
     <h1>Frames</h1>
@@ -45,8 +50,12 @@ <h2>Frames: #frame</h2>
       <h2>Frames: #hello</h2>
 
       <a href="/src/tests/fixtures/frames/frame.html">Load #frame</a>
+      <button type="button" id="remove-target-from-hello">Remove #hello[target]</button>
+
     </turbo-frame>
 
+    <a id="link-hello-advance" href="/src/tests/fixtures/frames/hello.html" data-turbo-frame="hello" data-turbo-action="advance">advance #hello</a>
+
     <turbo-frame id="nested-root" target="frame">
       <h2>Frames: #nested-root</h2>
       <a id="inner-outer-frame-link" href="/src/tests/fixtures/frames/frame.html" data-turbo-frame="nested-child">Inner/Outer frame link</a>
@@ -104,5 +113,9 @@ <h2>Frames: #nested-child</h2>
     <form data-turbo-frame="frame" method="get" action="/src/tests/fixtures/frames/frame.html">
       <input id="outer-frame-submit" type="submit" value="Outer form submit">
     </form>
+
+    <hr class="push-off-screen">
+    <input id="below-the-fold-input">
+    <a id="below-the-fold-link-frame-action" data-turbo-action="advance" data-turbo-frame="frame" href="/src/tests/fixtures/frames/frame.html">Navigate #frame</a>
   </body>
 </html>
diff --git a/src/tests/fixtures/test.js b/src/tests/fixtures/test.js
@@ -7,7 +7,9 @@
   }
 
   function eventListener(event) {
-    eventLogs.push([event.type, event.detail, event.target.id])
+    const skipped = document.documentElement.getAttribute("data-skip-event-details") || ""
+
+    eventLogs.push([event.type, skipped.includes(event.type) ? {} : event.detail, event.target.id])
   }
   window.mutationLogs = []