Expose Frame load state via [loaded] attribute

Closes #429 --- Introduce the `<turbo-frame loaded>` 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 `<a>` element or submitting a `<form>` 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 `<turbo-frame src="...">` 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
hotwired · Dec 1, 2021 · 9c26441 · 9c26441
1 parent aa9724d
commit 9c26441
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 51 deletions.
diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts
@@ -1,4 +1,4 @@
-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"
@@ -14,20 +14,21 @@ import { FrameRenderer } from "./frame_renderer"
 import { session } from "../index"
 import { isAction } from "../types"
 
+type ObservedAttribute = keyof FrameElement & FrameElementObservedAttribute
+
 export class FrameController implements AppearanceObserverDelegate, FetchRequestDelegate, FormInterceptorDelegate, FormSubmissionDelegate, FrameElementDelegate, LinkInterceptorDelegate, ViewDelegate<Snapshot<FrameElement>> {
   readonly element: FrameElement
   readonly view: FrameView
   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<ObservedAttribute> = new Set
 
   constructor(element: FrameElement) {
     this.element = element
@@ -40,13 +41,13 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   connect() {
     if (!this.connected) {
       this.connected = true
-      this.reloadable = false
+      this.linkInterceptor.start()
+      this.formInterceptor.start()
       if (this.loadingStyle == FrameLoadingStyle.lazy) {
         this.appearanceObserver.start()
+      } else {
+        this.loadSourceURL()
       }
-      this.linkInterceptor.start()
-      this.formInterceptor.start()
-      this.sourceURLChanged()
     }
   }
 
@@ -66,11 +67,23 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   }
 
   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()
@@ -80,21 +93,12 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
     }
   }
 
-  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
     }
   }
 
@@ -111,6 +115,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
         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)
@@ -140,7 +145,6 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
   }
 
   linkClickIntercepted(element: Element, url: string) {
-    this.reloadable = true
     this.navigateFrame(element, url)
   }
 
@@ -155,7 +159,6 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
       this.formSubmission.stop()
     }
 
-    this.reloadable = false
     this.formSubmission = new FormSubmission(this, element, submitter)
     const { fetchRequest } = this.formSubmission
     this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest)
@@ -256,7 +259,6 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
 
     this.proposeVisitIfNavigatedWithAction(frame, element, submitter)
 
-    frame.setAttribute("reloadable", "")
     frame.src = url
   }
 
@@ -287,11 +289,11 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
     const id = CSS.escape(this.id)
 
     try {
-      if (element = activateElement(container.querySelector(`turbo-frame#${id}`), this.currentURL)) {
+      if (element = activateElement(container.querySelector(`turbo-frame#${id}`), this.sourceURL)) {
         return element
       }
 
-      if (element = activateElement(container.querySelector(`turbo-frame[src][recurse~=${id}]`), this.currentURL)) {
+      if (element = activateElement(container.querySelector(`turbo-frame[src][recurse~=${id}]`), this.sourceURL)) {
         await element.loaded
         return await this.extractForeignFrameElement(element)
       }
@@ -355,25 +357,10 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
     }
   }
 
-  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")
-    }
-  }
-
   set sourceURL(sourceURL: string | undefined) {
-    this.settingSourceURL = true
-    this.element.src = sourceURL ?? null
-    this.currentURL = this.element.src
-    this.settingSourceURL = false
+    this.ignoringChangesToAttribute("src", () => {
+      this.element.src = sourceURL ?? null
+    })
   }
 
   get loadingStyle() {
@@ -384,6 +371,20 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
     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
   }
@@ -393,6 +394,16 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
     const root = meta?.content ?? "/"
     return expandURL(root)
   }
+
+  private isIgnoringChangesTo(attributeName: ObservedAttribute): boolean {
+    return this.ignoredAttributes.has(attributeName)
+  }
+
+  private ignoringChangesToAttribute(attributeName: ObservedAttribute, 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
@@ -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
@@ -2,9 +2,12 @@ import { FetchResponse } from "../http/fetch_response"
 
 export enum FrameLoadingStyle { eager = "eager", lazy = "lazy" }
 
+export type FrameElementObservedAttribute = "disabled" | "loaded" | "loading" | "src"
+
 export interface FrameElementDelegate {
   connect(): void
   disconnect(): void
+  loadedChanged(): void
   loadingStyleChanged(): void
   sourceURLChanged(): void
   disabledChanged(): void
@@ -37,8 +40,8 @@ export class FrameElement extends HTMLElement {
   loaded: Promise<FetchResponse | void> = Promise.resolve()
   readonly delegate: FrameElementDelegate
 
-  static get observedAttributes() {
-    return ["disabled", "loading", "src"]
+  static get observedAttributes(): FrameElementObservedAttribute[] {
+    return ["disabled", "loaded", "loading", "src"]
   }
 
   constructor() {
@@ -56,13 +59,16 @@ export class FrameElement extends HTMLElement {
 
   reload() {
     const { src } = this;
+    this.removeAttribute("loaded")
     this.src = null;
     this.src = src;
   }
 
   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
@@ -1,5 +1,5 @@
 <!DOCTYPE html>
-<html>
+<html data-skip-event-details="turbo:before-render">
   <head>
     <meta charset="utf-8">
     <title>Turbo</title>
@@ -13,6 +13,8 @@
       <turbo-frame id="hello" src="/src/tests/fixtures/frames/hello.html" loading="lazy"></turbo-frame>
     </details>
 
+    <a id="link-lazy-frame" href="/src/tests/fixtures/frames.html" data-turbo-frame="hello">Navigate #loading-lazy turbo-frame</a>
+
     <details id="loading-eager">
       <summary>Eager-loaded</summary>
 

diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts
@@ -422,6 +422,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"() {
@@ -436,6 +437,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"() {
@@ -450,6 +452,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"() {
@@ -477,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=post][data-turbo-action=advance] to the same URL clears the [aria-busy] and [data-turbo-preview] state"() {
@@ -490,6 +494,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"() {
@@ -504,6 +509,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"() {
@@ -539,6 +545,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"() {