From f7c454cfa24423df38e2ba603c69b3e3d558eda4 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Tue, 30 Nov 2021 21:17:34 -0500 Subject: [PATCH] Frame Visits: Cache Snapshot later in process Follow-up to [hotwired/turbo#441][] Depends on [hotwired/turbo#487][] Closes https://github.com/hotwired/turbo/issues/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 `` descendants have not been disconnected. The handling of the `` 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/turbo#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 `` 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/turbo#441]: https://github.com/hotwired/turbo/pull/441 [hotwired/turbo#487]: https://github.com/hotwired/turbo/pull/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 --- src/core/frames/frame_controller.ts | 38 ++++++++++++++------------- src/core/frames/frame_renderer.ts | 20 +++++++++++++- src/elements/frame_element.ts | 2 ++ src/tests/functional/frame_tests.ts | 2 ++ src/tests/functional/loading_tests.ts | 2 +- 5 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index b90808058..626b67669 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -41,6 +41,7 @@ export class FrameController private connected = false private hasBeenLoaded = false private ignoredAttributes: Set = new Set() + private previousContents?: DocumentFragment constructor(element: FrameElement) { this.element = element @@ -124,7 +125,7 @@ export class FrameController 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.complete = true @@ -250,6 +251,22 @@ export class FrameController 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) { @@ -280,7 +297,8 @@ export class FrameController 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 @@ -427,22 +445,6 @@ export class FrameController } } -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) diff --git a/src/core/frames/frame_renderer.ts b/src/core/frames/frame_renderer.ts index 559c3bd1e..3e57d6791 100644 --- a/src/core/frames/frame_renderer.ts +++ b/src/core/frames/frame_renderer.ts @@ -1,8 +1,26 @@ 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 { + private readonly delegate: FrameRendererDelegate + + constructor( + delegate: FrameRendererDelegate, + currentSnapshot: Snapshot, + newSnapshot: Snapshot, + isPreview: boolean, + willRender = true + ) { + super(currentSnapshot, newSnapshot, isPreview, willRender) + this.delegate = delegate + } + get shouldRender() { return true } @@ -22,7 +40,7 @@ export class FrameRenderer extends Renderer { 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() diff --git a/src/elements/frame_element.ts b/src/elements/frame_element.ts index 3f40937d7..7e8dfab5c 100644 --- a/src/elements/frame_element.ts +++ b/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", @@ -18,6 +19,7 @@ export interface FrameElementDelegate { linkClickIntercepted(element: Element, url: string): void loadResponse(response: FetchResponse): void fetchResponseLoaded: (fetchResponse: FetchResponse) => void + visitCachedSnapshot: (snapshot: Snapshot) => void isLoading: boolean } diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts index 0f0ec4045..a25ae721c 100644 --- a/src/tests/functional/frame_tests.ts +++ b/src/tests/functional/frame_tests.ts @@ -584,10 +584,12 @@ test("test navigating back after pushing URL state from a turbo-frame[data-turbo const title = await page.textContent("h1") const frameTitle = await page.textContent("#frame h2") + const src = new URL((await attributeForSelector(page, "#frame", "src")) || "") assert.equal(title, "Frames") assert.equal(frameTitle, "Frames: #frame") assert.equal(pathname(page.url()), "/src/tests/fixtures/frames.html") + assert.equal(src.pathname, "/src/tests/fixtures/frames/frame.html") assert.equal(await propertyForSelector(page, "#frame", "src"), null) }) diff --git a/src/tests/functional/loading_tests.ts b/src/tests/functional/loading_tests.ts index 4ea539518..1704a7aff 100644 --- a/src/tests/functional/loading_tests.ts +++ b/src/tests/functional/loading_tests.ts @@ -149,7 +149,7 @@ test("test changing [src] attribute on a [complete] frame with loading=lazy defe await page.click("#one") await nextEventNamed(page, "turbo:load") await page.goBack() - await nextBody(page) + await nextEventNamed(page, "turbo:load") await noNextEventNamed(page, "turbo:frame-load") let src = new URL((await attributeForSelector(page, "#hello", "src")) || "")