github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-09-10 05:58:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Fix typography sample errors (#11515)

Browse Source 
* 🐛 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 <family-name> grammar: a whitespace-
separated sequence of <custom-ident>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

* 🐛 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
This commit is contained in:
Eva Marco 2026-09-07 11:29:36 +02:00 committed by GitHub
parent 9462543fb5
commit 5f6169e1d3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 101 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

138
frontend/src/app/main/ui/workspace/sidebar/options/menus/typography.cljs
View File

@ -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
`<family-name>` grammar, i.e. whitespace-separated `<custom-ident>`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 `<string>` 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)