Frame Visits: Cache Snapshot later in process

Follow-up to [hotwired#441][]
Depends on [hotwired#487][]
Closes hotwired#472

---

When caching Snapshots during a `Visit`, elements are not cached until
after the `turbo:before-cache` event fires. This affords client
applications with an opportunity to disconnect and deconstruct and
JavaScript state, and provides an opportunity to encode that state into
the HTML so that it can survive the caching process.

The timing of the construction of the `SnapshotSubstitution` instance
occurs too early in the frame rendering process: the `<turbo-frame>`
descendants have not been disconnected. The handling of the
`<turbo-frame>` caching is already an exception from the norm.

Unfortunately, the current implementation is caching _too early_ in the
process. If the Snapshot were cached too late in the process with the
rest of the page (as described in [hotwired#441][]), the `[src]`
attribute and descendant content would have already changed, so any
previous state would be lost.

This commit strikes a balance between the two extremes by introducing
the `FrameRendererDelegate` interface and the `frameContentsExtracted()`
hook. During `<turbo-frame>` rendering, the `FrameRenderer` instance
selects a [Range][] of nodes and removes them by calling
[Range.deleteContents][]. The `deleteContents()` method removes the
Nodes and discards them.

This commit replaces the `deleteContents()` call with one to
[Range.extractContents][], so that the Nodes are retained as a
[DocumentFragment][] instance. While handling the callback, the
`FrameController` retains that instance by setting an internal
`previousContents` property.

Later on in the Frame rendering-to-Visit-promotion process, the
`FrameController` implements the `visitCachedSnapshot()` hook to read
from the `previousContents` property and substitute the frame's contents
with the `previousContents`, replacing the need for the
`SnapshotSubstitution` class.

[hotwired#441]: hotwired#441
[hotwired#487]: hotwired#487
[Range]: https://developer.mozilla.org/en-US/docs/Web/API/Range
[Range.deleteContents]: https://developer.mozilla.org/en-US/docs/Web/API/range/deleteContents
[Range.extractContents]: https://developer.mozilla.org/en-US/docs/Web/API/Range/extractContents
[DocumentFragment]: https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment

Author: seanpdoyle

src/core/frames/frame_controller.ts

@@ -29,6 +29,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   private connected = false
   private hasBeenLoaded = false
   private ignoredAttributes: Set<ObservedAttribute> = new Set
+  private previousContents?: DocumentFragment
 
   constructor(element: FrameElement) {
     this.element = element
@@ -112,7 +113,7 @@ 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, false)
+        const renderer = new FrameRenderer(this, this.view.snapshot, snapshot, false, false)
         if (this.view.renderPromise) await this.view.renderPromise
         await this.view.render(renderer)
         this.loaded = true
@@ -236,6 +237,22 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   viewInvalidated() {
   }
 
+  // Frame renderer delegate
+  frameContentsExtracted(fragment: DocumentFragment) {
+    this.previousContents = fragment
+  }
+
+  visitCachedSnapshot = ({ element }: Snapshot) => {
+    const frame = element.querySelector("#" + this.element.id)
+
+    if (frame && this.previousContents) {
+      frame.innerHTML = ""
+      frame.append(this.previousContents)
+    }
+
+    delete this.previousContents
+  }
+
   // Private
 
   private async visit(url: URL) {
@@ -266,7 +283,8 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
     const action = getAttribute("data-turbo-action", submitter, element, frame)
 
     if (isAction(action)) {
-      const { visitCachedSnapshot } = new SnapshotSubstitution(frame)
+      const { visitCachedSnapshot } = frame.delegate
+
       frame.delegate.fetchResponseLoaded = (fetchResponse: FetchResponse) => {
         if (frame.src) {
           const { statusCode, redirected } = fetchResponse
@@ -406,22 +424,6 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   }
 }
 
-class SnapshotSubstitution {
-  private readonly clone: Node
-  private readonly id: string
-
-  constructor(element: FrameElement) {
-    this.clone = element.cloneNode(true)
-    this.id = element.id
-  }
-
-  visitCachedSnapshot = ({ element }: Snapshot) => {
-    const { id, clone } = this
-
-    element.querySelector("#" + id)?.replaceWith(clone)
-  }
-}
-
 function getFrameElementById(id: string | null) {
   if (id != null) {
     const element = document.getElementById(id)

src/core/frames/frame_renderer.ts

@@ -1,8 +1,20 @@
 import { FrameElement } from "../../elements/frame_element"
 import { nextAnimationFrame } from "../../util"
 import { Renderer } from "../renderer"
+import { Snapshot } from "../snapshot"
+
+export interface FrameRendererDelegate {
+  frameContentsExtracted(fragment: DocumentFragment): void
+}
 
 export class FrameRenderer extends Renderer<FrameElement> {
+  private readonly delegate: FrameRendererDelegate
+
+  constructor(delegate: FrameRendererDelegate, currentSnapshot: Snapshot<FrameElement>, newSnapshot: Snapshot<FrameElement>, isPreview: boolean, willRender = true) {
+    super(currentSnapshot, newSnapshot, isPreview, willRender)
+    this.delegate = delegate
+  }
+
   get shouldRender() {
     return true
   }
@@ -22,7 +34,7 @@ export class FrameRenderer extends Renderer<FrameElement> {
   loadFrameElement() {
     const destinationRange = document.createRange()
     destinationRange.selectNodeContents(this.currentElement)
-    destinationRange.deleteContents()
+    this.delegate.frameContentsExtracted(destinationRange.extractContents())
 
     const frameElement = this.newElement
     const sourceRange = frameElement.ownerDocument?.createRange()

src/elements/frame_element.ts

@@ -1,4 +1,5 @@
 import { FetchResponse } from "../http/fetch_response"
+import { Snapshot } from "../core/snapshot"
 
 export enum FrameLoadingStyle { eager = "eager", lazy = "lazy" }
 
@@ -15,6 +16,7 @@ export interface FrameElementDelegate {
   linkClickIntercepted(element: Element, url: string): void
   loadResponse(response: FetchResponse): void
   fetchResponseLoaded: (fetchResponse: FetchResponse) => void
+  visitCachedSnapshot: (snapshot: Snapshot) => void
   isLoading: boolean
 }
 

src/tests/functional/frame_tests.ts

@@ -521,11 +521,12 @@ export class FrameTests extends TurboDriveTestCase {
 
     const title = await this.querySelector("h1")
     const frameTitle = await this.querySelector("#frame h2")
+    const src = new URL(await this.attributeForSelector("#frame", "src") || "")
 
     this.assert.equal(await title.getVisibleText(), "Frames")
     this.assert.equal(await frameTitle.getVisibleText(), "Frames: #frame")
     this.assert.equal(await this.pathname, "/src/tests/fixtures/frames.html")
-    this.assert.equal(await this.propertyForSelector("#frame", "src"), null)
+    this.assert.equal(src.pathname, "/src/tests/fixtures/frames/frame.html")
   }
 
   async "test navigating back then forward after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames next contents"() {

src/tests/functional/loading_tests.ts

@@ -133,7 +133,7 @@ export class LoadingTests extends TurboDriveTestCase {
     await this.clickSelector("#one")
     await this.nextEventNamed("turbo:load")
     await this.goBack()
-    await this.nextBody
+    await this.nextEventNamed("turbo:load")
     await this.noNextEventNamed("turbo:frame-load")
 
     let src = new URL(await this.attributeForSelector("#hello", "src") || "")