From 357382a17bc76b197b1d8eff3a7ae72835bc9d9a Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Thu, 11 Nov 2021 14:14:13 -0500 Subject: [PATCH] Integrate Frame to Page Visits with Snapshot Cache [Follow-up to hotwired/turbo#398][]. The original implementation achieved the desired outcome: navigate the page to reflect the URL of a ``. Unfortunately, the `session.visit()` call happens late-enough that the `` element's contents have already been updated. This means that when navigating back or forward through the browser's History API, the snapshots _already_ reflect the "new" frame's HTML. This means that navigating back _won't change the page's HTML_. To resolve that issue, expands the `VisitDelegate` to include a caching callback, then expands the `VisitOptions` type to include a `Partial`. Throughout the lifecycle, a `Visit` will delegate to _both_ its instance property and any present `VisitOption` delegate hooks. This commit aims to fix the broken behavior before the `7.1.0-rc` release, but if a concept like a `FrameVisit` introduced in [#430][] were to ship, it might be more straightforward to manage. [Follow-up to hotwired/turbo#398]: https://github.com/hotwired/turbo/pull/398 [#430]: https://github.com/hotwired/turbo/pull/430 --- src/core/drive/navigator.ts | 4 ++++ src/core/drive/visit.ts | 9 +++++++- src/core/frames/frame_controller.ts | 24 +++++++++++++++++----- src/core/session.ts | 3 +++ src/tests/functional/frame_tests.ts | 32 +++++++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 6 deletions(-) diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts index 59b2ad0e1..e9d3c6c97 100644 --- a/src/core/drive/navigator.ts +++ b/src/core/drive/navigator.ts @@ -133,6 +133,10 @@ 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/visit.ts b/src/core/drive/visit.ts index 7105d923b..1abdf8c8d 100644 --- a/src/core/drive/visit.ts +++ b/src/core/drive/visit.ts @@ -15,6 +15,7 @@ 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 } @@ -38,6 +39,7 @@ export enum VisitState { export type VisitOptions = { action: Action, + delegate: Partial historyChanged: boolean, willRender: boolean referrer?: URL, @@ -47,6 +49,7 @@ export type VisitOptions = { const defaultOptions: VisitOptions = { action: "advance", + delegate: {}, historyChanged: false, willRender: true } @@ -70,6 +73,7 @@ export class Visit implements FetchRequestDelegate { readonly action: Action readonly referrer?: URL readonly timingMetrics: TimingMetrics = {} + readonly optionalDelegate: Partial willRender: boolean followedRedirect = false @@ -90,7 +94,7 @@ export class Visit implements FetchRequestDelegate { this.location = location this.restorationIdentifier = restorationIdentifier || uuid() - const { action, historyChanged, referrer, snapshotHTML, response, willRender } = { ...defaultOptions, ...options } + const { action, historyChanged, referrer, snapshotHTML, response, willRender, delegate: optionalDelegate } = { ...defaultOptions, ...options } this.willRender = willRender this.action = action this.historyChanged = historyChanged @@ -98,6 +102,7 @@ export class Visit implements FetchRequestDelegate { this.snapshotHTML = snapshotHTML this.response = response this.isSamePage = this.delegate.locationWithActionIsSamePage(this.location, this.action) + this.optionalDelegate = optionalDelegate } get adapter() { @@ -126,6 +131,7 @@ 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) } } @@ -392,6 +398,7 @@ export class Visit implements FetchRequestDelegate { if (!this.snapshotCached) { this.view.cacheSnapshot() 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 index e3c78ee95..a911d9e0d 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -4,6 +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 } from "../drive/visit" import { Snapshot } from "../snapshot" import { ViewDelegate } from "../view" import { getAction, expandURL, urlsAreEqual, locationIsVisitable, Locatable } from "../url" @@ -260,11 +261,24 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest const action = getAttribute("data-turbo-action", submitter, element, frame) if (isAction(action)) { - const proposeVisit = async (event: Event) => { - const { detail: { fetchResponse: { location, redirected, statusCode } } } = event as CustomEvent - const responseHTML = document.documentElement.outerHTML - - session.visit(location, { willRender: false, action, response: { redirected, responseHTML, statusCode } }) + const clone = frame.cloneNode(true) + const proposeVisit = () => { + const { ownerDocument, id, src } = frame + if (src) { + const snapshotHTML = ownerDocument.documentElement.outerHTML + let snapshot: Snapshot + + const delegate = { + visitStarted(visit: Visit) { + snapshot = visit.view.snapshot + }, + visitCachedSnapshot() { + snapshot.element.querySelector("#" + id)?.replaceWith(clone) + } + } + + session.visit(src, { willRender: false, action, snapshotHTML, delegate }) + } } frame.addEventListener("turbo:frame-render", proposeVisit , { once: true }) diff --git a/src/core/session.ts b/src/core/session.ts index 064e809e6..70bd16133 100644 --- a/src/core/session.ts +++ b/src/core/session.ts @@ -189,6 +189,9 @@ 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/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts index 0c7653bb8..9b83171a6 100644 --- a/src/tests/functional/frame_tests.ts +++ b/src/tests/functional/frame_tests.ts @@ -357,6 +357,38 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") } + 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.nextBeat + await this.goBack() + await this.nextBody + + const title = await this.querySelector("h1") + const frameTitle = await this.querySelector("#frame h2") + + 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") + } + + 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.nextBeat + await this.goBack() + await this.nextBody + await this.goForward() + await this.nextBody + + const title = await this.querySelector("h1") + const frameTitle = await this.querySelector("#frame h2") + + this.assert.equal(await title.getVisibleText(), "Frames") + this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") + this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + } + async "test turbo:before-fetch-request fires on the frame element"() { await this.clickSelector("#hello a") this.assert.ok(await this.nextEventOnTarget("frame", "turbo:before-fetch-request"))