diff --git a/frontend/src/app/main/fonts.cljs b/frontend/src/app/main/fonts.cljs index 15a1824f18..f03f6b4967 100644 --- a/frontend/src/app/main/fonts.cljs +++ b/frontend/src/app/main/fonts.cljs @@ -139,8 +139,10 @@ ;; 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 node is pre-parsed eagerly ;; (`: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. -(defonce preview-sprite (l/atom {:status :idle :ids #{} :svg nil :node nil})) +;; covers (also pre-computed), so the UI can pick sprite vs fallback. `:refs` +;; counts concurrent attachers so concurrent selectors share one DOM node +;; instead of yanking it from under each other. +(defonce preview-sprite (l/atom {:status :idle :ids #{} :svg nil :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. @@ -162,7 +164,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 :node nil})) + (reset! preview-sprite {:status :error :ids #{} :svg nil :node nil :refs 0})) (defn- parse-sprite-svg "Parse the cached sprite markup as SVG (not HTML, so no innerHTML injection @@ -219,20 +221,31 @@ 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." + cheap appendChild. Concurrent attachers share the same cached node via a + refcount: only the first triggers the DOM append, only the last detach + removes it — so two selectors coexisting (right sidebar + in-canvas text + popover) don't yank the sprite out from under each other." [] - (let [{:keys [status node]} @preview-sprite] + (let [{:keys [status node refs]} @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)) + (when (zero? refs) + (when-let [body-el (unchecked-get globals/document "body")] + (dom/append-child! body-el node))) + (swap! preview-sprite update :refs (fnil inc 0)) node))) (defn detach-preview-sprite! "Remove the sprite node injected by `attach-preview-sprite!` from the DOM. The cached markup, parsed node, and `:ids` stay, so reopening re-attaches without - a refetch or re-parse." + a refetch or re-parse. Refcounted: only the last detach actually removes the + node, so concurrent selectors don't pull it out from under each other." [node] - (dom/remove! node)) + (let [refs (-> (swap! preview-sprite + (fn [s] + (update s :refs #(max 0 (dec (or % 0)))))) + :refs)] + (when (zero? refs) + (dom/remove! node)))) (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 379e4a2f3b..afd95a3c09 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 @@ -40,6 +40,7 @@ [app.util.timers :as tm] [cuerdas.core :as str] [goog.events :as events] + [okulary.core :as l] [promesa.core :as p] [rumext.v2 :as mf])) @@ -102,16 +103,14 @@ fonts, or the font's own name lazily loaded for custom fonts the sprite doesn't cover." {::mf/wrap [mf/memo]} - [{:keys [font]}] + [{:keys [font sprite-ids sprite-ready?]}] (let [font-id (:id font) - sprite (mf/deref fonts/preview-sprite) - in-sprite? (contains? (:ids sprite) font-id) + in-sprite? (contains? sprite-ids 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?)) + fallback? (and sprite-ready? (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. @@ -126,7 +125,7 @@ (mf/defc font-item* {::mf/wrap [mf/memo]} - [{:keys [font is-current on-click style]}] + [{:keys [font is-current on-click style sprite-ids sprite-ready?]}] (let [item-ref (mf/use-ref) on-click (mf/use-fn (mf/deps font) #(on-click font)) ;; FLAG :font-preview — gates the feature markup AND its row styling @@ -149,7 +148,9 @@ :font-item-preview-on preview? :selected is-current)} (if preview? - [:> font-item-preview* {:font font}] + [:> font-item-preview* {:font font + :sprite-ids sprite-ids + :sprite-ready? sprite-ready?}] [:span {:class (stl/css :font-item-label)} (:name font)]) (when is-current [:> icon* {:icon-id i/tick @@ -189,7 +190,18 @@ installed-ids (mf/with-memo [all-fonts] (into #{} (map :id) all-fonts)) - sprite-status (:status (mf/deref fonts/preview-sprite)) + ;; FLAG :font-preview — narrow the preview-sprite subscription to just + ;; `:status`/`:ids`, so unrelated swaps (e.g. a future `:refs` change) and + ;; the row reading the whole atom don't re-render every visible row. The + ;; lenses are stable across renders via `mf/with-memo []`; we only `deref` + ;; them when the flag is on, so the picker doesn't subscribe to the sprite + ;; at all when the feature is disabled. + preview? (contains? cf/flags :font-preview) + sprite-status (when preview? + (mf/deref (mf/with-memo [] (l/derived :status fonts/preview-sprite)))) + sprite-ids (when preview? + (mf/deref (mf/with-memo [] (l/derived :ids fonts/preview-sprite)))) + sprite-ready? (= :ready sprite-status) recent-fonts (mf/deref refs/recent-fonts) recent-fonts (mf/with-memo [state recent-fonts installed-ids] @@ -256,21 +268,19 @@ #(events/unlistenByKey key))) ;; 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. 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* (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)))))) + ;; the picker is open. Parsing and id collection happen once in + ;; `prefetch-preview-sprite!` (called on workspace mount), so attaching is just + ;; a cheap `appendChild` of the cached node — no need to defer it. Removing it + ;; on close keeps ~2000 nodes out of the DOM while the picker is closed. The + ;; attach is refcounted (see `attach-preview-sprite!`), so concurrent + ;; selectors share the node. Remove the flag clause to drop the feature. + (when preview? + (mf/with-effect [sprite-status] + (when (= :ready sprite-status) + (let [node (fonts/attach-preview-sprite!)] + (fn [] + (when-some [n node] + (fonts/detach-preview-sprite! n))))))) (mf/with-effect [@selected] (when-let [inst (mf/ref-val flist)] @@ -308,6 +318,8 @@ [:> font-item* {:key (dm/str "font-" idx) :font font :style {} + :sprite-ids sprite-ids + :sprite-ready? sprite-ready? :on-click on-select-and-close :is-current (= (:id font) (:id effective-selected))}])])] @@ -317,7 +329,7 @@ (fn [props] (let [width (unchecked-get props "width") height (unchecked-get props "height") - render #(row-renderer fonts effective-selected on-select-and-close %)] + render #(row-renderer fonts effective-selected on-select-and-close sprite-ids sprite-ready? %)] (mf/html [:> rvt/List #js {:height height :ref flist @@ -327,7 +339,7 @@ :rowRenderer render}])))]]]])) (defn row-renderer - [fonts selected on-select props] + [fonts selected on-select sprite-ids sprite-ready? props] (let [index (unchecked-get props "index") key (unchecked-get props "key") style (unchecked-get props "style") @@ -336,6 +348,8 @@ [:> font-item* {:key key :font font :style style + :sprite-ids sprite-ids + :sprite-ready? sprite-ready? :on-click on-select :is-current (= (:id font) (:id selected))}]))) diff --git a/frontend/test/frontend_tests/fonts_test.cljs b/frontend/test/frontend_tests/fonts_test.cljs index e2de0217e0..cf4b059f61 100644 --- a/frontend/test/frontend_tests/fonts_test.cljs +++ b/frontend/test/frontend_tests/fonts_test.cljs @@ -7,6 +7,7 @@ (ns frontend-tests.fonts-test (:require [app.main.fonts :as fonts] + [app.util.globals :as globals] [cljs.test :as t :include-macros true])) (def sample-font @@ -124,3 +125,153 @@ result (fonts/find-closest-variant font "200" nil)] (t/is (= "200" (:weight result))) (t/is (= "italic" (:style result)))))) + +;; --- preview sprite attach/detach ---------------------------------------- +;; `prefetch-preview-sprite!` pre-parses the sprite and caches the node under +;; `:node`; `attach-preview-sprite!` then just appends it and `detach-preview-sprite!` +;; removes it from the DOM while leaving the cache intact. The typography effect +;; (in typography.cljs) attaches synchronously on `:ready` and detaches on close; +;; these tests pin the contract that lets that pattern be re-render- and +;; reopen-safe: the cached `:node` survives attach/detach cycles in the atom, +;; the same JS instance is reused on reopen, and attach is a no-op unless +;; `:ready`+node. + +(defn- fake-sprite-state + ([node] + (fake-sprite-state node 0)) + ([node refs] + {:status :ready :ids #{} :svg "" :node node :refs refs})) + +(defn- fake-node + "A plain JS object that satisfies `dom/remove!` (`.remove`) and can be counted + when appended." + [append-append-cb] + (let [node #js {}] + (js/Object.defineProperty node "remove" + #js {:value (fn [] nil) :writable true}) + (js/Object.defineProperty node "appendChild" + #js {:value (fn [child] (when append-append-cb (append-append-cb)) child) + :writable true}) + node)) + +(t/use-fixtures :each + {:before (fn [] (reset! fonts/preview-sprite + {:status :idle :ids #{} :svg nil :node nil :refs 0})) + :after (fn [] (reset! fonts/preview-sprite + {:status :idle :ids #{} :svg nil :node nil :refs 0}))}) + +(t/deftest attach-preview-sprite-returns-cached-node-test + (t/testing "attach returns the same node instance cached by prefetch" + (let [node (fake-node nil)] + (reset! fonts/preview-sprite (fake-sprite-state node)) + (with-redefs [globals/browser? (constantly true) + globals/document #js {:body (fake-node nil)}] + (t/is (identical? node (fonts/attach-preview-sprite!))))))) + +(t/deftest attach-preview-sprite-nil-unless-ready-test + (t/testing "attach is a no-op unless the sprite is :ready with a parsed node" + (with-redefs [globals/browser? (constantly true)] + (doseq [status [:idle :loading :error]] + (reset! fonts/preview-sprite {:status status :ids #{} :svg nil :node nil :refs 0}) + (t/is (nil? (fonts/attach-preview-sprite!)) (str "status=" status))) + ;; :ready but no node (prefetch parsed nothing) + (reset! fonts/preview-sprite {:status :ready :ids #{} :svg nil :node nil :refs 0}) + (t/is (nil? (fonts/attach-preview-sprite!)))))) + +(t/deftest detach-preview-sprite-keeps-cache-test + (t/testing "detach removes the node from the DOM but leaves the cached node + in the atom, so reopening reattaches without re-parsing" + (let [node (fake-node nil)] + (reset! fonts/preview-sprite (fake-sprite-state node)) + (with-redefs [globals/browser? (constantly true)] + (fonts/detach-preview-sprite! node)) + (t/is (identical? node (:node @fonts/preview-sprite))) + (t/is (= :ready (:status @fonts/preview-sprite))) + ;; refcount dropped to zero + (t/is (zero? (:refs @fonts/preview-sprite)))))) + +(t/deftest attach-detach-reattach-reuses-same-instance-test + (t/testing "reattaching after detach appends the same cached node (no re-parse, + no duplicate nodes); mirrors a close/reopen of the font picker" + (let [appends (volatile! 0) + body (fake-node #(vswap! appends inc)) + node (fake-node nil)] + (reset! fonts/preview-sprite (fake-sprite-state node)) + (with-redefs [globals/browser? (constantly true) + globals/document #js {:body body}] + (let [n1 (fonts/attach-preview-sprite!) + _ (fonts/detach-preview-sprite! n1) + n2 (fonts/attach-preview-sprite!)] + (t/is (identical? n1 n2)) + (t/is (identical? node n2)) + ;; Two open cycles → two `appendChild` calls on body. detach doesn't + ;; reset the cache, so reopening never re-parses. + (t/is (= 2 @appends))))))) + +;; --- refcount: concurrent selectors share one DOM node ------------------- +;; Regression test for the bug where two `font-selector*` instances (e.g. right +;; sidebar + in-canvas text popover) mount at the same time, both read the same +;; cached `:node`, and the first unmount would `dom/remove!` it out from under +;; the second. The attach/detach pair is refcounted: the first attach puts the +;; node in the DOM; subsequent attaches just bump the counter (no second +;; appendChild); only the last detach removes it. + +(t/deftest attach-preview-sprite-refcount-no-double-append-test + (t/testing "concurrent attaches only append the DOM node once; the second + caller still receives the shared node and the refcount is 2" + (let [appends (volatile! 0) + body (fake-node #(vswap! appends inc)) + node (fake-node nil)] + (reset! fonts/preview-sprite (fake-sprite-state node)) + (with-redefs [globals/browser? (constantly true) + globals/document #js {:body body}] + (let [n1 (fonts/attach-preview-sprite!) + n2 (fonts/attach-preview-sprite!)] + (t/is (identical? n1 n2)) + (t/is (identical? node n2)) + (t/is (= 1 @appends) "appendChild called exactly once for two attaches") + (t/is (= 2 (:refs @fonts/preview-sprite)))))))) + +(t/deftest detach-preview-sprite-refcount-removes-only-on-last-test + (t/testing "with two concurrent attachers, detaching once keeps the node in the + DOM; only the second (last) detach actually removes it" + (let [removes (volatile! 0) + node (fake-node nil)] + ;; Track `.remove` invocations on the shared node. + (js/Object.defineProperty node "remove" + #js {:value #(vswap! removes inc) :writable true}) + (reset! fonts/preview-sprite (fake-sprite-state node)) + (with-redefs [globals/browser? (constantly true) + globals/document #js {:body (fake-node nil)}] + (let [_ (fonts/attach-preview-sprite!) + _ (fonts/attach-preview-sprite!)] + ;; First detach: counter 2→1, node must remain (remove not called) + (fonts/detach-preview-sprite! node) + (t/is (= 0 @removes) "first detach must not remove the shared node") + (t/is (= 1 (:refs @fonts/preview-sprite))) + ;; Second detach: counter 1→0, last detach removes the node + (fonts/detach-preview-sprite! node) + (t/is (= 1 @removes) "last detach removes the node") + (t/is (zero? (:refs @fonts/preview-sprite)))))))) + +(t/deftest detach-preview-sprite-refcount-clamps-at-zero-test + (t/testing "spurious detaches (counter already zero) clamp the refcount instead + of going negative (defensive against leaked/double detach calls)" + (let [node (fake-node nil)] + (reset! fonts/preview-sprite (fake-sprite-state node)) + (with-redefs [globals/browser? (constantly true)] + ;; Detach without an attach — counter is 0, must stay 0 (not -1) + (fonts/detach-preview-sprite! node) + (t/is (zero? (:refs @fonts/preview-sprite))))))) + +(t/deftest error-reset-clears-refcount-test + (t/testing "`:error` reset wipes the refcount, so a re-prefetch after an error + starts from a clean attach state (no stale refs pinning an old + detached node)" + (reset! fonts/preview-sprite (fake-sprite-state (fake-node nil) 3)) + (t/is (= 3 (:refs @fonts/preview-sprite))) + ;; Call the private reset fn directly (it's the public error path used by + ;; prefetch). + (#'fonts/reset-preview-sprite-error!) + (t/is (zero? (:refs @fonts/preview-sprite))) + (t/is (= :error (:status @fonts/preview-sprite))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 6d7f09855d..1db203704d 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -26,6 +26,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] @@ -118,6 +119,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