🐛 Fix font selector dropdown takes noticeably long to open when changing font

2026-08-19 03:08:40 +00:00 · 2026-08-04 19:15:13 +02:00 · 2026-08-04 19:15:13 +02:00 · 5df6562799
commit 5df6562799
parent fb9f92ae6a
4 changed files with 190 additions and 44 deletions
--- a/frontend/src/app/main/fonts.cljs
+++ b/frontend/src/app/main/fonts.cljs
@ -18,6 +18,7 @@
   [app.util.globals :as globals]
   [app.util.http :as http]
   [app.util.object :as obj]
+   [app.util.timers :as tm]
   [beicon.v2.core :as rx]
   [cuerdas.core :as str]
   [okulary.core :as l]
@ -116,10 +117,11 @@
 ;; uploads, ones that fail to bake) use the runtime fallback.
 ;;
 ;; The sprite is heavy (~2000 nodes), so we DON'T keep it in the DOM: the fetched
-;; markup is cached here as a string (`:svg`) and the nodes are materialized only
-;; while the picker is open (attach/detach below). `:ids` are the font ids it
-;; covers, so the UI can pick sprite vs fallback.
-(defonce preview-sprite (l/atom {:status :idle :ids #{} :svg nil}))
+;; markup is parsed once eagerly into a cached node (`:node`) so attaching is a
+;; cheap appendChild. `:ids` are the font ids it covers (also pre-computed), so
+;; the UI can pick sprite vs fallback. `:refs` counts open dropdowns sharing the
+;; node, so the last one to close is the one that detaches it.
+(defonce preview-sprite (l/atom {:status :idle :ids #{} :node nil :refs 0}))

 ;; Id prefix shared with the generator and the UI's `<use href>`; referenced here
 ;; rather than re-declared so the contract stays in one place.
@ -141,7 +143,7 @@
  []
  ;; :error → the UI shows plain names (no previews, no per-font load storm); a
  ;; later `prefetch-preview-sprite!` call can retry.
-  (reset! preview-sprite {:status :error :ids #{} :svg nil}))
+  (reset! preview-sprite {:status :error :ids #{} :node nil :refs 0}))

 (defn- parse-sprite-svg
  "Parse the cached sprite markup as SVG (not HTML, so no innerHTML injection
@ -155,10 +157,10 @@
      root)))

 (defn prefetch-preview-sprite!
-  "Fetch the font-preview sprite markup and cache it in memory (no DOM yet — see
-  `attach-preview-sprite!`). Idempotent: fetches only when nothing is cached yet
-  (`:idle`) or a previous attempt failed (`:error`); no-op while `:loading` or
-  `:ready`."
+  "Fetch the font-preview sprite markup, pre-parse it on idle, and cache the
+  parsed DOM node with the font ids it covers. Idempotent: fetches only when
+  nothing is cached yet (`:idle`) or a previous attempt failed (`:error`); no-op
+  while `:loading` or `:ready`."
  []
  (when (and (globals/browser?)
             (contains? #{:idle :error} (:status @preview-sprite)))
@ -170,9 +172,24 @@
         (rx/subs!
          (fn [response]
            ;; http/send! doesn't reject on non-2xx; guard so an error body isn't
-            ;; cached as the sprite.
+            ;; cached as the sprite. The parse is deferred to idle so the
+            ;; ~2000-node import doesn't spike the main thread at load time;
+            ;; `:status` stays `:loading` until it's done.
            (if (http/success? response)
-              (swap! preview-sprite assoc :status :ready :svg (:body response))
+              (let [svg (:body response)]
+                (tm/schedule-on-idle
+                 (fn []
+                   (if-let [node (some-> (parse-sprite-svg svg) (dom/import-node))]
+                     (do
+                       (dom/set-attribute! node "id" "font-preview-sprite")
+                       (let [ids (collect-preview-ids node)]
+                         (swap! preview-sprite assoc
+                                :status :ready
+                                :node node
+                                :ids ids)))
+                     (do
+                       (log/wrn :hint "cannot parse font preview sprite")
+                       (reset-preview-sprite-error!))))))
              (do
                (log/wrn :hint "cannot load font preview sprite" :status (:status response))
                (reset-preview-sprite-error!))))
@ -181,32 +198,31 @@
            (reset-preview-sprite-error!))))))

 (defn attach-preview-sprite!
-  "Materialize the cached sprite into the DOM (hidden) so rows can reference its
-  glyph groups via `<use>`, and record the covered font ids. Returns the injected
-  node (pass it to `detach-preview-sprite!` on close), or nil if not ready / the
-  markup is invalid. Parsing happens here, not on prefetch, so the cost is paid
-  only while the picker is open."
+  "Append the pre-parsed sprite node into the DOM (hidden) so rows can reference
+  its glyph groups via `<use>`. Returns the node (pass it to
+  `detach-preview-sprite!` on close), or nil if not ready. Parsing and id
+  collection happen once during `prefetch-preview-sprite!`, so this is just a
+  cheap appendChild. Multiple dropdowns may share the node; each attach
+  increments `:refs` so the node is only detached when the last one closes."
  []
-  (let [{:keys [status svg]} @preview-sprite]
-    (when (and (globals/browser?) (= :ready status) (some? svg))
-      (if-let [node (some-> (parse-sprite-svg svg) (dom/import-node))]
-        ;; The node already carries display:none + aria-hidden from the generator.
-        (do
-          (dom/set-attribute! node "id" "font-preview-sprite")
-          (when-let [body-el (unchecked-get globals/document "body")]
-            (dom/append-child! body-el node))
-          (swap! preview-sprite assoc :ids (collect-preview-ids node))
-          node)
-        (do
-          (log/wrn :hint "cannot parse font preview sprite")
-          (reset-preview-sprite-error!)
-          nil)))))
+  (let [{:keys [status node]} @preview-sprite]
+    (when (and (globals/browser?) (= :ready status) (some? node))
+      (when-let [body-el (unchecked-get globals/document "body")]
+        (dom/append-child! body-el node))
+      (swap! preview-sprite update :refs inc)
+      node)))

 (defn detach-preview-sprite!
-  "Remove the sprite node injected by `attach-preview-sprite!` from the DOM. The
-  cached markup and `:ids` stay, so reopening re-attaches without a refetch."
+  "Remove the sprite node injected by `attach-preview-sprite!` from the DOM when
+  the last open dropdown closes. The cached node and `:ids` stay, so reopening
+  re-attaches without a refetch or re-parse."
  [node]
-  (dom/remove! node))
+  (let [refs (max 0 (dec (:refs @preview-sprite)))]
+    (if (pos? refs)
+      (swap! preview-sprite assoc :refs refs)
+      (do
+        (dom/remove! node)
+        (swap! preview-sprite assoc :refs 0)))))

 (defn- add-font-css!
  "Creates a style element and attaches it to the dom."
--- a/frontend/src/app/main/ui/workspace/sidebar/options/menus/typography.cljs
+++ b/frontend/src/app/main/ui/workspace/sidebar/options/menus/typography.cljs
@ -105,13 +105,18 @@
  [{:keys [font]}]
  (let [font-id    (:id font)
        sprite     (mf/deref fonts/preview-sprite)
-        in-sprite? (contains? (:ids sprite) font-id)

-        ;; Fallback is ONLY for custom fonts: ones the (ready) sprite doesn't
-        ;; cover. If the sprite isn't ready (loading/error) we show the plain name
-        ;; rather than runtime-loading the whole catalog.
-        fallback?  (and (= :ready (:status sprite))
-                        (not in-sprite?))
+        ;; The sprite is only referenceable once it's been attached to the DOM,
+        ;; so the `<use>` glyph is gated on `attached?`. Until then we show the
+        ;; plain name: no blank rows, and no per-font load storm either (see
+        ;; `fallback?` below).
+        attached?  (pos? (:refs sprite))
+
+        ;; Fallback is ONLY for custom fonts: ones the (attached) sprite doesn't
+        ;; cover. If the sprite isn't ready (loading/error) or not yet attached,
+        ;; we show the plain name rather than runtime-loading the whole catalog.
+        in-sprite? (and attached? (contains? (:ids sprite) font-id))
+        fallback?  (and (= :ready (:status sprite)) attached? (not in-sprite?))
        loaded?    (use-font-lazy-load font-id fallback?)]
    (if in-sprite?
      ;; `fill: currentColor` (scss) makes the sprite glyph follow the row color.
@ -257,13 +262,20 @@

    ;; FLAG :font-preview — materialize the preview sprite into the DOM only while
    ;; the picker is open (markup is prefetched on workspace load), removing it on
-    ;; close so its ~2000 nodes aren't kept around idle. Remove the flag clause to
-    ;; drop the feature.
+    ;; close so its ~2000 nodes aren't kept around idle. The attachment is deferred
+    ;; so the dropdown can paint first with plain names, then the sprite swaps in
+    ;; on the next tick. Remove the flag clause to drop the feature.
    (mf/with-effect [sprite-status]
      (when (and (contains? cf/flags :font-preview)
                 (= :ready sprite-status))
-        (let [node (fonts/attach-preview-sprite!)]
-          #(fonts/detach-preview-sprite! node))))
+        (let [node*  (volatile! nil)
+              task   (tm/schedule
+                      (fn []
+                        (vreset! node* (fonts/attach-preview-sprite!))))]
+          (fn []
+            (tm/dispose! task)
+            (when-some [n @node*]
+              (fonts/detach-preview-sprite! n))))))

    (mf/with-effect [@selected]
      (when-let [inst (mf/ref-val flist)]
--- a/frontend/test/frontend_tests/fonts_test.cljs
+++ b/frontend/test/frontend_tests/fonts_test.cljs
@ -7,7 +7,11 @@
 (ns frontend-tests.fonts-test
  (:require
   [app.main.fonts :as fonts]
-   [cljs.test :as t :include-macros true]))
+   [app.util.globals :as globals]
+   [app.util.http :as http]
+   [beicon.v2.core :as rx]
+   [cljs.test :as t :include-macros true]
+   [frontend-tests.helpers.mock :as mock]))

 (def sample-font
  {:id "sourcesanspro"
@ -124,3 +128,115 @@
          result (fonts/find-closest-variant font "200" nil)]
      (t/is (= "200" (:weight result)))
      (t/is (= "italic" (:style result))))))
+
+;; --- preview sprite ----------------------------------------------------------
+;;
+;; The sprite feature (FLAG :font-preview) caches a pre-parsed SVG node shared by
+;; every open font dropdown. `:refs` counts the open dropdowns so the node is only
+;; detached when the last one closes. The unit test runner has no browser DOM, so
+;; the environment boundary (`globals/browser?`) is mocked and DOM nodes are
+;; replaced with minimal fakes exposing only what attach/detach touches.
+
+(t/use-fixtures
+  :each
+  (fn [test-fn]
+    (reset! fonts/preview-sprite {:status :idle :ids #{} :node nil :refs 0})
+    (test-fn)))
+
+(defn- fake-node
+  "A minimal DOM-like node exposing only what the sprite attach/detach touches."
+  []
+  #js {:remove (fn [] nil)})
+
+(t/deftest attach-preview-sprite-returns-nil-while-sprite-is-not-ready
+  (mock/with-mocks
+    {globals/browser? (mock/stub (constantly true))}
+    (fn [done]
+      (reset! fonts/preview-sprite {:status :loading :ids #{} :node nil :refs 0})
+      (t/is (nil? (fonts/attach-preview-sprite!)))
+      (t/is (= 0 (:refs @fonts/preview-sprite)))
+
+      (reset! fonts/preview-sprite {:status :error :ids #{} :node nil :refs 0})
+      (t/is (nil? (fonts/attach-preview-sprite!)))
+      (t/is (= 0 (:refs @fonts/preview-sprite)))
+      (done))
+    (fn [] nil)))
+
+(t/deftest attach-preview-sprite-increments-refs-and-returns-the-node
+  (mock/with-mocks
+    {globals/browser? (mock/stub (constantly true))}
+    (fn [done]
+      (let [node (fake-node)]
+        (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0})
+        (t/is (identical? node (fonts/attach-preview-sprite!)))
+        (t/is (= 1 (:refs @fonts/preview-sprite)))
+        (t/is (identical? node (fonts/attach-preview-sprite!)))
+        (t/is (= 2 (:refs @fonts/preview-sprite)))
+        (done)))
+    (fn [] nil)))
+
+(t/deftest detach-preview-sprite-removes-node-only-when-last-reference-drops
+  (mock/with-mocks
+    {globals/browser? (mock/stub (constantly true))}
+    (fn [done]
+      (let [removed? (volatile! false)
+            node     #js {:remove (fn [] (vreset! removed? true))}]
+        (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0})
+        (fonts/attach-preview-sprite!)
+        (fonts/attach-preview-sprite!)
+
+        ;; First detach keeps the node: another dropdown is still open.
+        (fonts/detach-preview-sprite! node)
+        (t/is (= 1 (:refs @fonts/preview-sprite)))
+        (t/is (false? @removed?))
+
+        ;; Second detach reaches zero refs, so the node is removed from the DOM.
+        (fonts/detach-preview-sprite! node)
+        (t/is (= 0 (:refs @fonts/preview-sprite)))
+        (t/is (true? @removed?))
+        (done)))
+    (fn [] nil)))
+
+(t/deftest detach-preview-sprite-clamps-refs-at-zero
+  (mock/with-mocks
+    {globals/browser? (mock/stub (constantly true))}
+    (fn [done]
+      (let [removed? (volatile! false)
+            node     #js {:remove (fn [] (vreset! removed? true))}]
+        (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0})
+        (fonts/detach-preview-sprite! node)
+        (t/is (= 0 (:refs @fonts/preview-sprite)))
+        (t/is (true? @removed?))
+        (done)))
+    (fn [] nil)))
+
+(t/deftest prefetch-preview-sprite-fetches-only-from-idle-or-error
+  (let [calls (volatile! 0)
+        fetch (mock/stub (fn [& _]
+                           (vswap! calls inc)
+                           (rx/empty)))]
+    (mock/with-mocks
+      {globals/browser? (mock/stub (constantly true))
+       http/fetch fetch}
+      (fn [done]
+        ;; :ready → no refetch
+        (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node (fake-node) :refs 0})
+        (fonts/prefetch-preview-sprite!)
+        (t/is (= 0 @calls))
+
+        ;; :loading → no refetch (an earlier request is in flight)
+        (reset! fonts/preview-sprite {:status :loading :ids #{} :node nil :refs 0})
+        (fonts/prefetch-preview-sprite!)
+        (t/is (= 0 @calls))
+
+        ;; :error → retries
+        (reset! fonts/preview-sprite {:status :error :ids #{} :node nil :refs 0})
+        (fonts/prefetch-preview-sprite!)
+        (t/is (= 1 @calls))
+
+        ;; :idle → first fetch
+        (reset! fonts/preview-sprite {:status :idle :ids #{} :node nil :refs 0})
+        (fonts/prefetch-preview-sprite!)
+        (t/is (= 2 @calls))
+        (done))
+      (fn [] nil))))
--- a/frontend/test/frontend_tests/runner.cljs
+++ b/frontend/test/frontend_tests/runner.cljs
@ -27,6 +27,7 @@
   [frontend-tests.data.workspace-texts-test]
   [frontend-tests.data.workspace-thumbnails-test]
   [frontend-tests.errors-test]
+   [frontend-tests.fonts-test]
   [frontend-tests.helpers-shapes-test]
   [frontend-tests.logic.comp-remove-swap-slots-test]
   [frontend-tests.logic.components-and-tokens]
@ -124,6 +125,7 @@
   'frontend-tests.data.workspace-texts-test
   'frontend-tests.data.workspace-thumbnails-test
   'frontend-tests.errors-test
+   'frontend-tests.fonts-test
   'frontend-tests.helpers-shapes-test
   'frontend-tests.logic.comp-remove-swap-slots-test
   'frontend-tests.logic.components-and-tokens