From 539b249d82f44f5a4fd6bd245fe1e45718b31508 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Fri, 19 Nov 2021 09:46:19 -0500 Subject: [PATCH] Preserve page state while promoting Frame-to-Visit (#448) 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 `` 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 `` 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: } }` 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 `` 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]: https://github.com/hotwired/turbo/pull/398 [430]: https://github.com/hotwired/turbo/pull/430 [441]: https://github.com/hotwired/turbo/pull/441 [444]: https://github.com/hotwired/turbo/pull/444 [Stale Element Reference]: https://developer.mozilla.org/en-US/docs/Web/WebDriver/Errors/StaleElementReference --- src/core/drive/navigator.ts | 4 -- src/core/drive/page_renderer.ts | 4 +- src/core/drive/page_view.ts | 8 ++- src/core/drive/visit.ts | 27 ++++---- src/core/frames/frame_controller.ts | 35 +++++------ src/core/renderer.ts | 4 +- src/core/session.ts | 3 - src/elements/frame_element.ts | 1 + src/tests/fixtures/frames.html | 15 ++++- src/tests/fixtures/test.js | 4 +- src/tests/functional/frame_tests.ts | 98 ++++++++++++++++++++++++++--- 11 files changed, 148 insertions(+), 55 deletions(-) diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts index e9d3c6c97..59b2ad0e1 100644 --- 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 index 6fdb58768..a5318673b 100644 --- a/src/core/drive/page_renderer.ts +++ b/src/core/drive/page_renderer.ts @@ -11,7 +11,9 @@ export class PageRenderer extends Renderer { } 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 index f168ac03a..37496f595 100644 --- a/src/core/drive/page_view.ts +++ b/src/core/drive/page_view.ts @@ -15,8 +15,8 @@ export class PageView extends View historyChanged: boolean, referrer?: URL, snapshotHTML?: string, response?: VisitResponse + visitCachedSnapshot(snapshot: Snapshot): void + willRender: boolean } const defaultOptions: VisitOptions = { action: "advance", - delegate: {}, historyChanged: false, + visitCachedSnapshot: () => {}, + willRender: true, } export type VisitResponse = { @@ -71,7 +73,8 @@ export class Visit implements FetchRequestDelegate { readonly action: Action readonly referrer?: URL readonly timingMetrics: TimingMetrics = {} - readonly optionalDelegate: Partial + readonly visitCachedSnapshot: (snapshot: Snapshot) => void + readonly willRender: boolean followedRedirect = false frame?: number @@ -91,14 +94,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 +132,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 +217,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 +259,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 +390,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 index e4287c570..be7b8e363 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -4,7 +4,6 @@ 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 { Snapshot } from "../snapshot" import { ViewDelegate } from "../view" import { getAction, expandURL, urlsAreEqual, locationIsVisitable, Locatable } from "../url" @@ -23,6 +22,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 +108,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 +264,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 +395,19 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest } } -class SnapshotSubstitution implements Partial { +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 }: Snapshot) => { + 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 index 981bd7676..7b5def56a 100644 --- a/src/core/renderer.ts +++ b/src/core/renderer.ts @@ -10,13 +10,15 @@ export abstract class Renderer = Snapsh readonly currentSnapshot: S readonly newSnapshot: S readonly isPreview: boolean + readonly willRender: boolean readonly promise: Promise private resolvingFunctions?: ResolvingFunctions - 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 index 70bd16133..064e809e6 100644 --- 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 index bfde5d66c..94f4ac60b 100644 --- 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 index ed74efa3f..e475268eb 100644 --- a/src/tests/fixtures/frames.html +++ b/src/tests/fixtures/frames.html @@ -1,5 +1,5 @@ - + Frame @@ -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") } }) +

Frames

@@ -48,8 +53,12 @@

Frames: #frame

Frames: #hello

Load #frame + + + advance #hello +

Frames: #nested-root

Inner/Outer frame link @@ -107,5 +116,9 @@

Frames: #nested-child

+ +
+ + Navigate #frame diff --git a/src/tests/fixtures/test.js b/src/tests/fixtures/test.js index 0cc3ea174..66a6871e4 100644 --- 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 = [] diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts index 043742b02..87f2ea034 100644 --- a/src/tests/functional/frame_tests.ts +++ b/src/tests/functional/frame_tests.ts @@ -288,13 +288,13 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await this.nextAttributeMutationNamed("frame", "aria-busy"), "true", "sets aria-busy on the ") await this.nextEventOnTarget("frame", "turbo:before-fetch-request") await this.nextEventOnTarget("frame", "turbo:before-fetch-response") - await this.nextEventOnTarget("html", "turbo:before-visit") - await this.nextEventOnTarget("html", "turbo:visit") await this.nextEventOnTarget("frame", "turbo:frame-render") await this.nextEventOnTarget("frame", "turbo:frame-load") this.assert.notOk(await this.nextAttributeMutationNamed("frame", "aria-busy"), "removes aria-busy from the ") this.assert.equal(await this.nextAttributeMutationNamed("html", "aria-busy"), "true", "sets aria-busy on the ") + await this.nextEventOnTarget("html", "turbo:before-visit") + await this.nextEventOnTarget("html", "turbo:visit") await this.nextEventOnTarget("html", "turbo:before-cache") await this.nextEventOnTarget("html", "turbo:before-render") await this.nextEventOnTarget("html", "turbo:render") @@ -321,7 +321,34 @@ export class FrameTests extends TurboDriveTestCase { async "test navigating turbo-frame[data-turbo-action=advance] from within pushes URL state"() { await this.clickSelector("#add-turbo-action-to-frame") await this.clickSelector("#link-frame") - await this.nextBeat + await this.nextEventNamed("turbo:load") + + 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 navigating turbo-frame[data-turbo-action=advance] to the same URL clears the [aria-busy] and [data-turbo-preview] state"() { + await this.clickSelector("#link-outside-frame-action-advance") + await this.nextEventNamed("turbo:load") + await this.clickSelector("#link-outside-frame-action-advance") + await this.nextEventNamed("turbo:load") + await this.clickSelector("#link-outside-frame-action-advance") + await this.nextEventNamed("turbo:load") + + this.assert.equal(await this.attributeForSelector("#frame", "aria-busy"), null, "clears turbo-frame[aria-busy]") + this.assert.equal(await this.attributeForSelector("#html", "aria-busy"), null, "clears html[aria-busy]") + this.assert.equal(await this.attributeForSelector("#html", "data-turbo-preview"), null, "clears html[aria-busy]") + } + + async "test navigating a turbo-frame with an a[data-turbo-action=advance] preserves page state"() { + await this.scrollToSelector("#below-the-fold-input") + await this.fillInSelector("#below-the-fold-input", "a value") + await this.clickSelector("#below-the-fold-link-frame-action") + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -331,11 +358,32 @@ export class FrameTests extends TurboDriveTestCase { 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") + this.assert.equal(await this.propertyForSelector("#below-the-fold-input", "value"), "a value", "preserves page state") + + const { y } = await this.scrollPosition + this.assert.notEqual(y, 0, "preserves Y scroll position") + } + + async "test a turbo-frame that has been driven by a[data-turbo-action] can be navigated normally"() { + await this.clickSelector("#remove-target-from-hello") + await this.clickSelector("#link-hello-advance") + await this.nextEventNamed("turbo:load") + + this.assert.equal(await (await this.querySelector("h1")).getVisibleText(), "Frames") + this.assert.equal(await (await this.querySelector("#hello h2")).getVisibleText(), "Hello from a frame") + this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/hello.html") + + await this.clickSelector("#hello a") + await this.nextEventOnTarget("hello", "turbo:frame-load") + await this.noNextEventNamed("turbo:load") + + this.assert.equal(await (await this.querySelector("#hello h2")).getVisibleText(), "Frames: #hello") + this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/hello.html") } async "test navigating turbo-frame from within with a[data-turbo-action=advance] pushes URL state"() { await this.clickSelector("#link-nested-frame-action-advance") - await this.nextBeat + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -349,7 +397,7 @@ export class FrameTests extends TurboDriveTestCase { async "test navigating frame with a[data-turbo-action=advance] pushes URL state"() { await this.clickSelector("#link-outside-frame-action-advance") - await this.nextBeat + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -363,7 +411,7 @@ export class FrameTests extends TurboDriveTestCase { async "test navigating frame with form[method=get][data-turbo-action=advance] pushes URL state"() { await this.clickSelector("#form-get-frame-action-advance button") - await this.nextBeat + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -375,9 +423,22 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") } + async "test navigating frame with form[method=get][data-turbo-action=advance] to the same URL clears the [aria-busy] and [data-turbo-preview] state"() { + await this.clickSelector("#form-get-frame-action-advance button") + await this.nextEventNamed("turbo:load") + await this.clickSelector("#form-get-frame-action-advance button") + await this.nextEventNamed("turbo:load") + await this.clickSelector("#form-get-frame-action-advance button") + await this.nextEventNamed("turbo:load") + + this.assert.equal(await this.attributeForSelector("#frame", "aria-busy"), null, "clears turbo-frame[aria-busy]") + this.assert.equal(await this.attributeForSelector("#html", "aria-busy"), null, "clears html[aria-busy]") + this.assert.equal(await this.attributeForSelector("#html", "data-turbo-preview"), null, "clears html[aria-busy]") + } + async "test navigating frame with form[method=post][data-turbo-action=advance] pushes URL state"() { await this.clickSelector("#form-post-frame-action-advance button") - await this.nextBeat + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -389,9 +450,22 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") } + async "test navigating frame with form[method=post][data-turbo-action=advance] to the same URL clears the [aria-busy] and [data-turbo-preview] state"() { + await this.clickSelector("#form-post-frame-action-advance button") + await this.nextEventNamed("turbo:load") + await this.clickSelector("#form-post-frame-action-advance button") + await this.nextEventNamed("turbo:load") + await this.clickSelector("#form-post-frame-action-advance button") + await this.nextEventNamed("turbo:load") + + this.assert.equal(await this.attributeForSelector("#frame", "aria-busy"), null, "clears turbo-frame[aria-busy]") + this.assert.equal(await this.attributeForSelector("#html", "aria-busy"), null, "clears html[aria-busy]") + this.assert.equal(await this.attributeForSelector("#html", "data-turbo-preview"), null, "clears html[aria-busy]") + } + async "test navigating frame with button[data-turbo-action=advance] pushes URL state"() { await this.clickSelector("#button-frame-action-advance") - await this.nextBeat + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -448,6 +522,14 @@ export class FrameTests extends TurboDriveTestCase { this.assert.ok(await this.nextEventOnTarget("frame", "turbo:before-fetch-response")) } + async fillInSelector(selector: string, value: string) { + const element = await this.querySelector(selector) + + await element.click() + + return element.type(value) + } + get frameScriptEvaluationCount(): Promise { return this.evaluate(() => window.frameScriptEvaluationCount) }