From 03549a9df9562cb9ba14d716b24dbaf0b77716c6 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sun, 8 Oct 2023 18:49:42 -0400 Subject: [PATCH] Infer `renderElement` during `Renderer` construction Follow-up to [#1192][] Closes [#1303][] Remove the argument from all `Renderer` subclass constructor call sites. In its place, define the default value for the `Renderer.renderElement` property based on the `static renderElement(currentElement, newElement)` defined by the inheriting class. By automatically determining which `renderElement` implementation based on class, this commit enables a simpler Frame morphing strategy. Instead of attaching one-time event listeners, this commit maintains a private `#shouldMorphFrame` property that is set, then un-set during the rendering lifecycle of a frame. Similarly, this commit implements the `MorphingFrameRenderer.preservingPermanentElements` method in the same style as the `MorphingPageRenderer.preservingPermanentElements`. Since _instances_ of `MorphingFrameRenderer` are now being used instead of the static `MorphingFrameRenderer.renderElement`, we're able to utilize an instance method instead of passing around an additional anonymous function (which is the implementation that [#1303][] proposed). [#1192]: https://github.com/hotwired/turbo/pull/1192 [#1303]: https://github.com/hotwired/turbo/pull/1303 Co-authored-by: Micah Geisel --- src/core/drive/page_view.js | 4 ++-- src/core/frames/frame_controller.js | 15 +++++++-------- src/core/frames/morphing_frame_renderer.js | 4 ++++ src/core/renderer.js | 8 ++++++-- src/tests/fixtures/frames.html | 1 + src/tests/functional/frame_tests.js | 12 ++++++++++++ 6 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/core/drive/page_view.js b/src/core/drive/page_view.js index 1bcb04134..58790d900 100644 --- a/src/core/drive/page_view.js +++ b/src/core/drive/page_view.js @@ -19,7 +19,7 @@ export class PageView extends View { const shouldMorphPage = this.isPageRefresh(visit) && this.snapshot.shouldMorphPage const rendererClass = shouldMorphPage ? MorphingPageRenderer : PageRenderer - const renderer = new rendererClass(this.snapshot, snapshot, rendererClass.renderElement, isPreview, willRender) + const renderer = new rendererClass(this.snapshot, snapshot, isPreview, willRender) if (!renderer.shouldRender) { this.forceReloaded = true @@ -32,7 +32,7 @@ export class PageView extends View { renderError(snapshot, visit) { visit?.changeHistory() - const renderer = new ErrorRenderer(this.snapshot, snapshot, ErrorRenderer.renderElement, false) + const renderer = new ErrorRenderer(this.snapshot, snapshot, false) return this.render(renderer) } diff --git a/src/core/frames/frame_controller.js b/src/core/frames/frame_controller.js index f07d58859..963e3190d 100644 --- a/src/core/frames/frame_controller.js +++ b/src/core/frames/frame_controller.js @@ -33,6 +33,7 @@ export class FrameController { #connected = false #hasBeenLoaded = false #ignoredAttributes = new Set() + #shouldMorphFrame = false action = null constructor(element) { @@ -90,13 +91,10 @@ export class FrameController { } sourceURLReloaded() { - if (this.element.shouldReloadWithMorph) { - this.element.addEventListener("turbo:before-frame-render", ({ detail }) => { - detail.render = MorphingFrameRenderer.renderElement - }, { once: true }) - } + const { refresh, src } = this.element + + this.#shouldMorphFrame = src && refresh === "morph" - const { src } = this.element this.element.removeAttribute("complete") this.element.src = null this.element.src = src @@ -139,6 +137,7 @@ export class FrameController { } } } finally { + this.#shouldMorphFrame = false this.fetchResponseLoaded = () => Promise.resolve() } } @@ -304,11 +303,11 @@ export class FrameController { async #loadFrameResponse(fetchResponse, document) { const newFrameElement = await this.extractForeignFrameElement(document.body) + const rendererClass = this.#shouldMorphFrame ? MorphingFrameRenderer : FrameRenderer if (newFrameElement) { const snapshot = new Snapshot(newFrameElement) - const renderer = new FrameRenderer(this, this.view.snapshot, snapshot, FrameRenderer.renderElement, false, false) - + const renderer = new rendererClass(this, this.view.snapshot, snapshot, false, false) if (this.view.renderPromise) await this.view.renderPromise this.changeHistory() diff --git a/src/core/frames/morphing_frame_renderer.js b/src/core/frames/morphing_frame_renderer.js index fe42065d5..4d5ea9c34 100644 --- a/src/core/frames/morphing_frame_renderer.js +++ b/src/core/frames/morphing_frame_renderer.js @@ -11,4 +11,8 @@ export class MorphingFrameRenderer extends FrameRenderer { morphChildren(currentElement, newElement) } + + async preservingPermanentElements(callback) { + return await callback() + } } diff --git a/src/core/renderer.js b/src/core/renderer.js index f0a02f581..8ede76e65 100644 --- a/src/core/renderer.js +++ b/src/core/renderer.js @@ -3,12 +3,16 @@ import { Bardo } from "./bardo" export class Renderer { #activeElement = null - constructor(currentSnapshot, newSnapshot, renderElement, isPreview, willRender = true) { + static renderElement(currentElement, newElement) { + // Abstract method + } + + constructor(currentSnapshot, newSnapshot, isPreview, willRender = true) { this.currentSnapshot = currentSnapshot this.newSnapshot = newSnapshot this.isPreview = isPreview this.willRender = willRender - this.renderElement = renderElement + this.renderElement = this.constructor.renderElement this.promise = new Promise((resolve, reject) => (this.resolvingFunctions = { resolve, reject })) } diff --git a/src/tests/fixtures/frames.html b/src/tests/fixtures/frames.html index e505852de..1749de14b 100644 --- a/src/tests/fixtures/frames.html +++ b/src/tests/fixtures/frames.html @@ -41,6 +41,7 @@

Frames: #frame

Navigate #frame with ?key=value Navigate #frame from within with a[data-turbo-action="advance"] Visit one.html +
diff --git a/src/tests/functional/frame_tests.js b/src/tests/functional/frame_tests.js index 11d327cb9..264024720 100644 --- a/src/tests/functional/frame_tests.js +++ b/src/tests/functional/frame_tests.js @@ -306,6 +306,18 @@ test("calling reload on a frame[refresh=morph] morphs the contents", async ({ pa expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy() }) +test("calling reload on a frame[refresh=morph] preserves [data-turbo-permanent] elements", async ({ page }) => { + await page.click("#add-src-to-frame") + await page.click("#add-refresh-morph-to-frame") + const input = await page.locator("#permanent-input") + + await input.fill("Preserve me") + await page.evaluate(() => document.getElementById("frame").reload()) + + await expect(input).toBeFocused() + await expect(input).toHaveValue("Preserve me") +}) + test("following a link in rapid succession cancels the previous request", async ({ page }) => { await page.click("#outside-frame-form") await page.click("#outer-frame-link")