From bccbe401523b7793660d1b87678ea911ca677c3e Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Thu, 25 Nov 2021 21:26:27 -0500 Subject: [PATCH] Expose Frame load state via `[loaded]` attribute Closes https://github.com/hotwired/turbo/issues/429 --- Introduce the `` boolean attribute. The attribute's absence indicates that the frame has not yet been loaded, and is ready to be navigated. Its presence means that the contents of the frame have been fetch from its `[src]` attribute. Encoding the load state into the element's HTML aims to integrate with Snapshot caching. Once a frame is loaded, navigating away and then restoring a page's state from an Historical Snapshot should preserve the fact that the contents are already loaded. For both `eager` and `lazy` loaded frames, changing the element's `[src]` attribute (directly via JavaScript, or by clicking an `` element or submitting a `
` element) will remove the `[loaded]` attribute. Eager-loaded frames will immediately initiate a request to fetch the contents, and Lazy-loaded frames will initiate the request once they enter the viewport, or are changed to be eager-loading. When the `[src]` attribute is changed, the `FrameController` will only remove the `[loaded]` attribute if the element [isConnected][] to the document, so that the `[loaded]` attribute is not modified prior to Snapshot Caching or when re-mounting a Cached Snapshot. The act of "reloading" involves the removal of the `[loaded]` attribute, which can be done either by `FrameElement.reload()` or `document.getElementById("frame-element").removeAttribute("loaded")`. A side-effect of introducing the `[loaded]` attribute is that the `FrameController` no longer needs to internally track: 1. how the internal `currentURL` value compares to the external `sourceURL` value 2. whether or not the frame is "reloadable" By no longer tracking the `sourceURL` and `currentURL` separately, the implementation for the private `loadSourceURL` method can be simplified. Since there is no longer a `currentURL` property to rollback, the `try { ... } catch (error) { ... }` can be omitted, and the `this.sourceURL` presence check can be incorporated into the rest of the guard conditional. Finally, this commit introduce the `isIgnoringChangesTo()` and `ignoringChangesToAttribute()` private methods to disable FrameController observations for a given period of time. For example, when setting the `` attribute, previous implementation would set, then check the value of a `this.settingSourceURL` property to decide whether or not to fire attribute change callback code. This commit refines that pattern to support any property of the `FrameElement` that's returned from the `FrameElement.observedAttributes` static property, including the `"src"` or `"loaded"` value. When making internal modifications to those values, it's important to temporarily disable observation callbacks to avoid unnecessary requests and to limit the potential for infinitely recursing loops. [isConnected]: https://developer.mozilla.org/en-US/docs/Web/API/Node/isConnected --- src/core/frames/frame_controller.ts | 105 ++++++++++++++------------ src/core/frames/frame_redirector.ts | 1 - src/elements/frame_element.ts | 10 ++- src/tests/fixtures/loading.html | 4 +- src/tests/functional/frame_tests.ts | 7 ++ src/tests/functional/loading_tests.ts | 76 +++++++++++++++++++ 6 files changed, 151 insertions(+), 52 deletions(-) diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index 85d514387..e7eec013d 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -1,4 +1,9 @@ -import { FrameElement, FrameElementDelegate, FrameLoadingStyle } from "../../elements/frame_element" +import { + FrameElement, + FrameElementDelegate, + FrameLoadingStyle, + FrameElementObservedAttribute, +} from "../../elements/frame_element" import { FetchMethod, FetchRequest, FetchRequestDelegate, FetchRequestHeaders } from "../../http/fetch_request" import { FetchResponse } from "../../http/fetch_response" import { AppearanceObserver, AppearanceObserverDelegate } from "../../observers/appearance_observer" @@ -29,14 +34,13 @@ export class FrameController readonly appearanceObserver: AppearanceObserver readonly linkInterceptor: LinkInterceptor readonly formInterceptor: FormInterceptor - currentURL?: string | null formSubmission?: FormSubmission fetchResponseLoaded = (_fetchResponse: FetchResponse) => {} private currentFetchRequest: FetchRequest | null = null private resolveVisitPromise = () => {} private connected = false private hasBeenLoaded = false - private settingSourceURL = false + private ignoredAttributes: Set = new Set() constructor(element: FrameElement) { this.element = element @@ -49,13 +53,13 @@ export class FrameController connect() { if (!this.connected) { this.connected = true - this.reloadable = false if (this.loadingStyle == FrameLoadingStyle.lazy) { this.appearanceObserver.start() + } else { + this.loadSourceURL() } this.linkInterceptor.start() this.formInterceptor.start() - this.sourceURLChanged() } } @@ -75,11 +79,23 @@ export class FrameController } sourceURLChanged() { + if (this.isIgnoringChangesTo("src")) return + + if (this.element.isConnected) { + this.loaded = false + } + if (this.loadingStyle == FrameLoadingStyle.eager || this.hasBeenLoaded) { this.loadSourceURL() } } + loadedChanged() { + if (this.isIgnoringChangesTo("loaded")) return + + this.loadSourceURL() + } + loadingStyleChanged() { if (this.loadingStyle == FrameLoadingStyle.lazy) { this.appearanceObserver.start() @@ -89,26 +105,12 @@ export class FrameController } } - async loadSourceURL() { - if ( - !this.settingSourceURL && - this.enabled && - this.isActive && - (this.reloadable || this.sourceURL != this.currentURL) - ) { - const previousURL = this.currentURL - this.currentURL = this.sourceURL - if (this.sourceURL) { - try { - this.element.loaded = this.visit(expandURL(this.sourceURL)) - this.appearanceObserver.stop() - await this.element.loaded - this.hasBeenLoaded = true - } catch (error) { - this.currentURL = previousURL - throw error - } - } + private async loadSourceURL() { + if (this.enabled && this.isActive && !this.loaded && this.sourceURL) { + this.element.loaded = this.visit(expandURL(this.sourceURL)) + this.appearanceObserver.stop() + await this.element.loaded + this.hasBeenLoaded = true } } @@ -125,6 +127,7 @@ export class FrameController const renderer = new FrameRenderer(this.view.snapshot, snapshot, false, false) if (this.view.renderPromise) await this.view.renderPromise await this.view.render(renderer) + this.loaded = true session.frameRendered(fetchResponse, this.element) session.frameLoaded(this.element) this.fetchResponseLoaded(fetchResponse) @@ -154,7 +157,6 @@ export class FrameController } linkClickIntercepted(element: Element, url: string) { - this.reloadable = true this.navigateFrame(element, url) } @@ -169,7 +171,6 @@ export class FrameController this.formSubmission.stop() } - this.reloadable = false this.formSubmission = new FormSubmission(this, element, submitter) const { fetchRequest } = this.formSubmission this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest) @@ -272,7 +273,6 @@ export class FrameController this.proposeVisitIfNavigatedWithAction(frame, element, submitter) - frame.setAttribute("reloadable", "") frame.src = url } @@ -308,12 +308,12 @@ export class FrameController const id = CSS.escape(this.id) try { - element = activateElement(container.querySelector(`turbo-frame#${id}`), this.currentURL) + element = activateElement(container.querySelector(`turbo-frame#${id}`), this.sourceURL) if (element) { return element } - element = activateElement(container.querySelector(`turbo-frame[src][recurse~=${id}]`), this.currentURL) + element = activateElement(container.querySelector(`turbo-frame[src][recurse~=${id}]`), this.sourceURL) if (element) { await element.loaded return await this.extractForeignFrameElement(element) @@ -379,24 +379,9 @@ export class FrameController } set sourceURL(sourceURL: string | undefined) { - this.settingSourceURL = true - this.element.src = sourceURL ?? null - this.currentURL = this.element.src - this.settingSourceURL = false - } - - get reloadable() { - const frame = this.findFrameElement(this.element) - return frame.hasAttribute("reloadable") - } - - set reloadable(value: boolean) { - const frame = this.findFrameElement(this.element) - if (value) { - frame.setAttribute("reloadable", "") - } else { - frame.removeAttribute("reloadable") - } + this.ignoringChangesToAttribute("src", () => { + this.element.src = sourceURL ?? null + }) } get loadingStyle() { @@ -407,6 +392,20 @@ export class FrameController return this.formSubmission !== undefined || this.resolveVisitPromise() !== undefined } + get loaded() { + return this.element.hasAttribute("loaded") + } + + set loaded(value: boolean) { + this.ignoringChangesToAttribute("loaded", () => { + if (value) { + this.element.setAttribute("loaded", "") + } else { + this.element.removeAttribute("loaded") + } + }) + } + get isActive() { return this.element.isActive && this.connected } @@ -416,6 +415,16 @@ export class FrameController const root = meta?.content ?? "/" return expandURL(root) } + + private isIgnoringChangesTo(attributeName: FrameElementObservedAttribute): boolean { + return this.ignoredAttributes.has(attributeName) + } + + private ignoringChangesToAttribute(attributeName: FrameElementObservedAttribute, callback: () => void) { + this.ignoredAttributes.add(attributeName) + callback() + this.ignoredAttributes.delete(attributeName) + } } class SnapshotSubstitution { diff --git a/src/core/frames/frame_redirector.ts b/src/core/frames/frame_redirector.ts index 8e41aea8e..1ddb5a0d5 100644 --- a/src/core/frames/frame_redirector.ts +++ b/src/core/frames/frame_redirector.ts @@ -42,7 +42,6 @@ export class FrameRedirector implements LinkInterceptorDelegate, FormInterceptor formSubmissionIntercepted(element: HTMLFormElement, submitter?: HTMLElement) { const frame = this.findFrameElement(element, submitter) if (frame) { - frame.removeAttribute("reloadable") frame.delegate.formSubmissionIntercepted(element, submitter) } } diff --git a/src/elements/frame_element.ts b/src/elements/frame_element.ts index d64e552e0..83c9a8ce1 100644 --- a/src/elements/frame_element.ts +++ b/src/elements/frame_element.ts @@ -5,9 +5,12 @@ export enum FrameLoadingStyle { lazy = "lazy", } +export type FrameElementObservedAttribute = keyof FrameElement & ("disabled" | "loaded" | "loading" | "src") + export interface FrameElementDelegate { connect(): void disconnect(): void + loadedChanged(): void loadingStyleChanged(): void sourceURLChanged(): void disabledChanged(): void @@ -40,8 +43,8 @@ export class FrameElement extends HTMLElement { loaded: Promise = Promise.resolve() readonly delegate: FrameElementDelegate - static get observedAttributes() { - return ["disabled", "loading", "src"] + static get observedAttributes(): FrameElementObservedAttribute[] { + return ["disabled", "loaded", "loading", "src"] } constructor() { @@ -59,6 +62,7 @@ export class FrameElement extends HTMLElement { reload() { const { src } = this + this.removeAttribute("loaded") this.src = null this.src = src } @@ -66,6 +70,8 @@ export class FrameElement extends HTMLElement { attributeChangedCallback(name: string) { if (name == "loading") { this.delegate.loadingStyleChanged() + } else if (name == "loaded") { + this.delegate.loadedChanged() } else if (name == "src") { this.delegate.sourceURLChanged() } else { diff --git a/src/tests/fixtures/loading.html b/src/tests/fixtures/loading.html index 9c0b9a673..8b925933d 100644 --- a/src/tests/fixtures/loading.html +++ b/src/tests/fixtures/loading.html @@ -1,5 +1,5 @@ - + Turbo @@ -13,6 +13,8 @@ + Navigate #loading-lazy turbo-frame +
Eager-loaded diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts index 9ef2ff199..cf15f9508 100644 --- a/src/tests/functional/frame_tests.ts +++ b/src/tests/functional/frame_tests.ts @@ -450,6 +450,7 @@ 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.ok(await this.hasSelector("#frame[loaded]"), "marks the frame as [loaded]") } async "test navigating frame with a[data-turbo-action=advance] pushes URL state"() { @@ -464,6 +465,7 @@ 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.ok(await this.hasSelector("#frame[loaded]"), "marks the frame as [loaded]") } async "test navigating frame with form[method=get][data-turbo-action=advance] pushes URL state"() { @@ -478,6 +480,7 @@ 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.ok(await this.hasSelector("#frame[loaded]"), "marks the frame as [loaded]") } 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"() { @@ -505,6 +508,7 @@ 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.ok(await this.hasSelector("#frame[loaded]"), "marks the frame as [loaded]") } 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"() { @@ -518,6 +522,7 @@ export class FrameTests extends TurboDriveTestCase { 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]") + this.assert.ok(await this.hasSelector("#frame[loaded]"), "marks the frame as [loaded]") } async "test navigating frame with button[data-turbo-action=advance] pushes URL state"() { @@ -532,6 +537,7 @@ 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.ok(await this.hasSelector("#frame[loaded]"), "marks the frame as [loaded]") } async "test navigating back after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames previous contents"() { @@ -567,6 +573,7 @@ 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.ok(await this.hasSelector("#frame[loaded]"), "marks the frame as [loaded]") } async "test turbo:before-fetch-request fires on the frame element"() { diff --git a/src/tests/functional/loading_tests.ts b/src/tests/functional/loading_tests.ts index abd11af32..3a433c8df 100644 --- a/src/tests/functional/loading_tests.ts +++ b/src/tests/functional/loading_tests.ts @@ -14,6 +14,7 @@ export class LoadingTests extends TurboDriveTestCase { async "test eager loading within a details element"() { await this.nextBeat this.assert.ok(await this.hasSelector("#loading-eager turbo-frame#frame h2")) + this.assert.ok(await this.hasSelector("#loading-eager turbo-frame[loaded]"), "has [loaded] attribute") } async "test lazy loading within a details element"() { @@ -21,12 +22,14 @@ export class LoadingTests extends TurboDriveTestCase { const frameContents = "#loading-lazy turbo-frame h2" this.assert.notOk(await this.hasSelector(frameContents)) + this.assert.ok(await this.hasSelector("#loading-lazy turbo-frame:not([loaded])")) await this.clickSelector("#loading-lazy summary") await this.nextBeat const contents = await this.querySelector(frameContents) this.assert.equal(await contents.getVisibleText(), "Hello from a frame") + this.assert.ok(await this.hasSelector("#loading-lazy turbo-frame[loaded]"), "has [loaded] attribute") } async "test changing loading attribute from lazy to eager loads frame"() { @@ -98,8 +101,11 @@ export class LoadingTests extends TurboDriveTestCase { const frameContent = "#loading-eager turbo-frame#frame h2" this.assert.ok(await this.hasSelector(frameContent)) + this.assert.ok(await this.hasSelector("#loading-eager turbo-frame[loaded]"), "has [loaded] attribute") + await this.remote.execute(() => (document.querySelector("#loading-eager turbo-frame") as any)?.reload()) this.assert.ok(await this.hasSelector(frameContent)) + this.assert.ok(await this.hasSelector("#loading-eager turbo-frame:not([loaded])"), "clears [loaded] attribute") } async "test navigating away from a page does not reload its frames"() { @@ -111,6 +117,76 @@ export class LoadingTests extends TurboDriveTestCase { this.assert.equal(requestLogs.length, 1) } + async "test removing the [loaded] attribute of an eager frame reloads the content"() { + await this.nextEventOnTarget("frame", "turbo:frame-load") + await this.remote.execute(() => document.querySelector("#loading-eager turbo-frame")?.removeAttribute("loaded")) + await this.nextEventOnTarget("frame", "turbo:frame-load") + + this.assert.ok( + await this.hasSelector("#loading-eager turbo-frame[loaded]"), + "sets the [loaded] attribute after re-loading" + ) + } + + async "test changing [src] attribute on a [loaded] frame with loading=lazy defers navigation"() { + await this.nextEventOnTarget("frame", "turbo:frame-load") + await this.clickSelector("#loading-lazy summary") + await this.nextEventOnTarget("hello", "turbo:frame-load") + + this.assert.ok(await this.hasSelector("#loading-lazy turbo-frame[loaded]"), "lazy frame is loaded") + this.assert.equal(await (await this.querySelector("#hello h2")).getVisibleText(), "Hello from a frame") + + await this.clickSelector("#loading-lazy summary") + await this.clickSelector("#one") + await this.nextEventNamed("turbo:load") + await this.goBack() + await this.nextBody + await this.noNextEventNamed("turbo:frame-load") + + let src = new URL((await this.attributeForSelector("#hello", "src")) || "") + + this.assert.ok(await this.hasSelector("#loading-lazy turbo-frame[loaded]"), "lazy frame is loaded") + this.assert.equal(src.pathname, "/src/tests/fixtures/frames/hello.html", "lazy frame retains [src]") + + await this.clickSelector("#link-lazy-frame") + await this.noNextEventNamed("turbo:frame-load") + + this.assert.ok(await this.hasSelector("#loading-lazy turbo-frame:not([loaded])"), "lazy frame is not loaded") + + await this.clickSelector("#loading-lazy summary") + await this.nextEventOnTarget("hello", "turbo:frame-load") + + src = new URL((await this.attributeForSelector("#hello", "src")) || "") + + this.assert.equal( + await (await this.querySelector("#loading-lazy turbo-frame h2")).getVisibleText(), + "Frames: #hello" + ) + this.assert.ok(await this.hasSelector("#loading-lazy turbo-frame[loaded]"), "lazy frame is loaded") + this.assert.equal(src.pathname, "/src/tests/fixtures/frames.html", "lazy frame navigates") + } + + async "test navigating away from a page and then back does not reload its frames"() { + await this.clickSelector("#one") + await this.nextBody + await this.eventLogChannel.read() + await this.goBack() + await this.nextBody + + const eventLogs = await this.eventLogChannel.read() + const requestLogs = eventLogs.filter(([name]) => name == "turbo:before-fetch-request") + const requestsOnEagerFrame = requestLogs.filter((record) => record[2] == "frame") + const requestsOnLazyFrame = requestLogs.filter((record) => record[2] == "hello") + + this.assert.equal(requestsOnEagerFrame.length, 0, "does not reload eager frame") + this.assert.equal(requestsOnLazyFrame.length, 0, "does not reload lazy frame") + + await this.clickSelector("#loading-lazy summary") + await this.nextEventOnTarget("hello", "turbo:before-fetch-request") + await this.nextEventOnTarget("hello", "turbo:frame-render") + await this.nextEventOnTarget("hello", "turbo:frame-load") + } + async "test disconnecting and reconnecting a frame does not reload the frame"() { await this.nextBeat