From 5f6169e1d3e2f7acc6ce9d98908c88699967a248 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Mon, 7 Sep 2026 11:29:36 +0200 Subject: [PATCH] :bug: Fix typography sample errors (#11515) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :bug: Fix font-family sample not showing for numeric font names Setting style.fontFamily to a raw, unquoted family name (e.g. "Micro 5") parses it against CSS's grammar: a whitespace- separated sequence of s. "Micro" tokenizes fine, but a bare "5" isn't a valid CSS identifier (idents can't start with a digit) — it tokenizes as a number instead, so the whole property is invalid CSS and the browser silently drops it. Every other font in the list happened to avoid this because none of their names have a token that's purely numeric. Quote the family name, matching what font-item-preview* (the font selector's own preview, a few lines down in the same file) already does, so it's parsed as a CSS string instead of unquoted identifiers. Also falls back to the live fontsdb entry's family when the typography record's own :font-family is blank — a font that was unloaded when a typography's font/variant was last changed can leave that field nil (the same failure mode remove-nil-style-attrs already repairs for shape text spans) — and loads the font unconditionally in the collapsed asset row, matching the expanded editor, since the optical-offset cache can otherwise skip loading it entirely. AI-assisted-by: claude-sonnet-5 * :bug: Fix flaky typography sample position in automated tests The optical-centering offset for the "Ag" sample (and the font selector's fallback name label) resolves asynchronously: first paint is unshifted, then an idle-scheduled Canvas measurement lands and the sample jumps to its final position. Any test that checks position or takes a screenshot shortly after paint races that jump — whether it runs before or after is a timing accident, not a deterministic outcome, which is exactly the "sometimes a few pixels up, sometimes down" flakiness QA hit. use-optical-offset now returns [offset ready?], with both lazily initialized from the cache so a cache hit needs no async round-trip at all. sample-text-style hides the glyphs until ready?, so the sample only ever appears already in its final, correct position instead of visibly moving there after the fact. The font selector's own name label uses the same hook but always shows real text content rather than a decorative sample, so it keeps the old behavior instead: hiding it would blank out font names while scrolling, worse than the minor positional nicety it's fixing. AI-assisted-by: claude-sonnet-5 --- .../sidebar/options/menus/typography.cljs | 138 +++++++++++++----- 1 file changed, 101 insertions(+), 37 deletions(-) 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 c397c24448..04b3dcb8a6 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 @@ -130,6 +130,20 @@ (if-let [cached (get @optical-offset-cache key)] (p/resolved cached) (-> (fonts/ensure-loaded! font-id) + (p/then + (fn [_] + ;; ensure-loaded! only guarantees the @font-face CSS text has + ;; been injected, not that the browser has actually fetched and + ;; parsed the font file: that fetch is lazy, normally triggered + ;; by the browser laying out DOM text with the font. Canvas + ;; measureText doesn't reliably trigger it, so without this + ;; explicit wait it can silently measure the fallback font + ;; instead, and the resulting bogus offset then gets cached + ;; forever — pushing the sample glyphs outside the clipped + ;; sample box instead of just centering them slightly wrong. + (let [spec (dm/str (or weight "400") " " (or style "normal") " 16px \"" family "\"")] + (-> (.load js/document.fonts spec) + (p/catch (constantly nil)))))) (p/then (fn [_] (let [em (or (optical-offset-em family weight style text) 0)] @@ -139,36 +153,67 @@ (defn- use-optical-offset "Lazily resolve the optical-centering offset (in `em`) for sample text in a given font, measuring once per font/sample and caching it. Falls back to 0 - when the font isn't available or the metrics can't be measured." + when the font isn't available or the metrics can't be measured. + + Returns `[offset ready?]`. `ready?` is true immediately when a value is + already cached, and false only while the very first measurement of a given + font/sample is still pending. Callers should keep the sample hidden until + `ready?`: the offset (and so the sample's rendered position) jumps once + that first, async measurement resolves, and painting the glyphs before + then makes automated screenshot/position-based tests flaky — whether the + test runs before or after the jump is a timing race, not a deterministic + outcome." [font-id family weight style text] - (let [offset* (mf/use-state 0)] + (let [key (optical-offset-key family weight style text) + offset* (mf/use-state #(get @optical-offset-cache key 0)) + ready?* (mf/use-state #(contains? @optical-offset-cache key))] (mf/use-effect (mf/deps font-id family weight style text) (fn [] - (let [cancelled? (volatile! false) - key (optical-offset-key family weight style text)] + (let [cancelled? (volatile! false)] (if (contains? @optical-offset-cache key) - (reset! offset* (get @optical-offset-cache key)) + (do + (reset! offset* (get @optical-offset-cache key)) + (reset! ready?* true)) (let [task (tm/schedule-on-idle (fn [] (-> (load-optical-offset font-id family weight style text) (p/then (fn [em] (when-not @cancelled? - (reset! offset* em)))))))] + (reset! offset* em) + (reset! ready?* true)))))))] (fn [] (vreset! cancelled? true) (tm/dispose! task))))) nil)) - (deref offset*))) + [(deref offset*) (deref ready?*)])) (defn- sample-container-style "Inline style that applies the typography font to the (clipped, fixed-height) sample container. Must be a real JS object (`#js`), not a ClojureScript map: the `:style` value here is a runtime expression, not a literal recognized by - the hiccup macro, so it reaches React unconverted." - [typography] - #js {:fontFamily (:font-family typography) + the hiccup macro, so it reaches React unconverted. + + Falls back to `font-data` (the live fontsdb entry for the typography's + `:font-id`) when the typography's own `:font-family` is blank: a font that + was unloaded when a typography's font/variant was last changed can leave + that field nil on the record (the same failure mode `remove-nil-style-attrs` + repairs for shape text spans), and the sample would otherwise render in + whatever fallback font the browser picks instead of the intended one. + + The family name is quoted, matching `font-item-preview*` below: setting + `style.fontFamily` to a raw, unquoted string parses it as CSS's + `` grammar, i.e. whitespace-separated ``s. A + family like \"Micro 5\" then tokenizes as the ident `Micro` followed by + the *number* `5` — not a valid ident — so the whole property is invalid + CSS and the browser silently drops it. A quoted `` sidesteps that + entirely, since it isn't tokenized as identifiers at all." + [typography font-data] + #js {:fontFamily (let [family (:font-family typography) + family (if (str/blank? family) (:family font-data) family)] + (when-not (str/blank? family) + (dm/str "\"" family "\""))) :fontWeight (:font-weight typography) :fontStyle (:font-style typography)}) @@ -176,10 +221,14 @@ "Inline style that optically centers the sample glyphs. Must be applied to the text node itself, not to the clipped container: a transform on an `overflow: hidden` element moves its own clip region along with it, so it - would shift the whole box relative to the row instead of the glyphs inside it." - [em] - (when-not (zero? em) - #js {:transform (dm/str "translateY(" em "em)")})) + would shift the whole box relative to the row instead of the glyphs inside it. + + Hidden until `ready?` (see `use-optical-offset`), so the glyphs only ever + appear already in their final, correctly centered position instead of + visibly jumping there after the first paint." + [em ready?] + #js {:transform (when-not (zero? em) (dm/str "translateY(" em "em)")) + :visibility (when-not ready? "hidden")}) ;; --- FONT SELECTOR -------------------------------------------------------- @@ -210,11 +259,17 @@ ;; the row, so shift it by the measured offset once the font is known. ;; The label renders at `body-medium` (400/normal), which is the weight ;; and style we measure against. - label-offset (use-optical-offset font-id - (:family font) - "400" - "normal" - (:name font))] + ;; The selector always shows the font's name text as-is, unlike the + ;; small "Ag" sample elsewhere in this file, so unlike there this + ;; doesn't need to hide anything until the offset is ready — a + ;; shifting label is a minor, acceptable visual nicety here, not + ;; the row's only content. + [label-offset _label-ready?] + (use-optical-offset font-id + (:family font) + "400" + "normal" + (:name font))] (if in-sprite? ;; `fill: currentColor` (scss) makes the sprite glyph follow the row color. [:svg {:class (stl/css :font-item-preview) @@ -702,11 +757,12 @@ font-data (fonts/get-font-data (:font-id typography)) typography-id (:id typography) show-actions? (and is-asset? is-editable) - offset (use-optical-offset (:font-id typography) - (:font-family typography) - (:font-weight typography) - (:font-style typography) - "Ag") + [offset offset-ready?] + (use-optical-offset (:font-id typography) + (:font-family typography) + (:font-weight typography) + (:font-style typography) + "Ag") on-delete (mf/use-fn @@ -741,8 +797,8 @@ [:* [:div {:class (stl/css :font-name-wrapper)} [:div {:class (stl/css :typography-sample-input) - :style (sample-container-style typography)} - [:span {:style (sample-text-style offset)} + :style (sample-container-style typography font-data)} + [:span {:style (sample-text-style offset offset-ready?)} (tr "workspace.assets.typography.sample")]] [:input @@ -777,8 +833,8 @@ [:div {:class (stl/css :typography-info-wrapper)} [:div {:class (stl/css :typography-name-wrapper)} [:div {:class (stl/css :typography-sample) - :style (sample-container-style typography)} - [:span {:style (sample-text-style offset)} + :style (sample-container-style typography font-data)} + [:span {:style (sample-text-style offset offset-ready?)} (tr "workspace.assets.typography.sample")]] [:div {:class (stl/css :typography-name) @@ -826,11 +882,12 @@ open? (deref open*) font-data (fonts/get-font-data (:font-id typography)) name-only? (= (:name typography) (:name font-data)) - offset (use-optical-offset (:font-id typography) - (:font-family typography) - (:font-weight typography) - (:font-style typography) - "Ag") + [offset offset-ready?] + (use-optical-offset (:font-id typography) + (:font-family typography) + (:font-weight typography) + (:font-style typography) + "Ag") on-name-blur (mf/use-fn @@ -865,6 +922,13 @@ (when ^boolean esc? (dom/blur! input-node)))))] + ;; use-optical-offset only triggers a font load as a side effect of an + ;; uncached offset measurement, so on a cache hit (e.g. a `0` offset + ;; cached from before the font was ever loaded) the font itself never + ;; gets fetched and the sample silently renders in the fallback font. + ;; Load it unconditionally too, same as the advanced-options view does. + (fonts/ensure-loaded! (:font-id typography)) + (mf/with-effect [is-editing] (when is-editing (reset! open* is-editing))) @@ -888,8 +952,8 @@ [:div {:class (stl/css :font-name-wrapper)} [:div {:class (stl/css :typography-sample-input) - :style (sample-container-style typography)} - [:span {:style (sample-text-style offset)} + :style (sample-container-style typography font-data)} + [:span {:style (sample-text-style offset offset-ready?)} (tr "workspace.assets.typography.sample")]] [:input @@ -907,8 +971,8 @@ :on-context-menu on-context-menu} [:div {:class (stl/css :typography-sample) - :style (sample-container-style typography)} - [:span {:style (sample-text-style offset)} + :style (sample-container-style typography font-data)} + [:span {:style (sample-text-style offset offset-ready?)} (tr "workspace.assets.typography.sample")]] [:div {:class (stl/css :name-block)