From 5df65627993411dc7a495b8d413aebd0cc29c610 Mon Sep 17 00:00:00 2001 From: Luis de Dios Date: Tue, 4 Aug 2026 19:15:13 +0200 Subject: [PATCH] :bug: Fix font selector dropdown takes noticeably long to open when changing font --- frontend/src/app/main/fonts.cljs | 82 +++++++----- .../sidebar/options/menus/typography.cljs | 32 +++-- frontend/test/frontend_tests/fonts_test.cljs | 118 +++++++++++++++++- frontend/test/frontend_tests/runner.cljs | 2 + 4 files changed, 190 insertions(+), 44 deletions(-) diff --git a/frontend/src/app/main/fonts.cljs b/frontend/src/app/main/fonts.cljs index b41a9d6135..7943e688c6 100644 --- 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 ``; 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 ``, 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 ``. 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." diff --git a/frontend/src/app/main/ui/workspace/sidebar/options/menus/typography.cljs b/frontend/src/app/main/ui/workspace/sidebar/options/menus/typography.cljs index 4dc3473644..1a085ef1c4 100644 --- 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 `` 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)] diff --git a/frontend/test/frontend_tests/fonts_test.cljs b/frontend/test/frontend_tests/fonts_test.cljs index e2de0217e0..40645fcfc4 100644 --- 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)))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 6cc1faff3e..270e66fb7c 100644 --- 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