From c4e7674a34d6a381f7c747e61ba07ec0d588a70a Mon Sep 17 00:00:00 2001 From: raymondginger Date: Sun, 20 Sep 2026 21:59:04 +0800 Subject: [PATCH] fix(desktop): test font presence with a local() lookup, not document.fonts.check() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Appearance → Fonts picker is supposed to list only the families the machine actually has, but the test behind it was `document.fonts.check()`, which answers "can this text be rendered" rather than "is this family installed". An unmatched family still renders through the fallback, so `check()` answered true for everything and every candidate survived the filter. Measured on Edge/WebView2 153.0.4234 — the engine Tauri uses on Windows, and the one this picker has to work in — `check()` returned true for all 39 families swept out of the Windows font registry, including the sentinel `__Absent Font 12345__`. The picker therefore offered fonts the machine does not have. `local()` goes through font matching and answers the real question. In the same engine it resolved the six families this machine has and rejected the rest (Roboto, Helvetica, PingFang SC, …). Reconciliation against the Windows font registry: 15/17 candidates agree by exact name, and the two remaining rows are the registry labelling font *files* rather than families — `Cascadia Code Regular` is the full name of the family `Cascadia Code`, and `Segoe UI Variable` is the family name inside SegUIVar.ttf while only its optical sizes (`… Text`, `… Display`) resolve as families. `local("Segoe UI Variable")` fails, so the candidate list now carries `Segoe UI Variable Text`. No installed family is wrongly filtered out. The probe is asynchronous, so `isFontAvailable` and `availableFontCandidates` now return promises, and the settings component holds the result in state rather than a `useMemo`. A probe loads a throwaway face and never registers it, so it leaves `document.fonts` untouched. An engine without `FontFace`, or one that refuses local lookups, reports nothing rather than claiming every family exists. The suite is rebuilt around a stub faithful to the measured engine behaviour (resolve for an installed family, NetworkError otherwise), including a regression test that asserts `document.fonts.check` is never consulted. Gates: tsc --noEmit, eslint ., vitest run (39 files / 267 tests), npm run build, npm run build:web and cargo fmt --check all pass. Refs #155 --- desktop/src/app/fontCandidates.test.ts | 149 ++++++++++++++---- desktop/src/app/fontCandidates.ts | 66 ++++++-- .../features/settings/AppearanceSettings.tsx | 13 +- 3 files changed, 178 insertions(+), 50 deletions(-) diff --git a/desktop/src/app/fontCandidates.test.ts b/desktop/src/app/fontCandidates.test.ts index 97fa9e8cf..870035007 100644 --- a/desktop/src/app/fontCandidates.test.ts +++ b/desktop/src/app/fontCandidates.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi, type Mock } from "vitest"; import { appendFamily, @@ -7,60 +7,145 @@ import { isFontAvailable, } from "./fontCandidates"; -afterEach(() => { - vi.unstubAllGlobals(); - Reflect.deleteProperty(document, "fonts"); -}); +/** + * A stand-in for the engine's font matching, faithful to what Edge/WebView2 + * 153.0.4234 was measured doing: a `local()` lookup resolves for a family the + * machine has, and rejects with NetworkError for one it does not. + * + * The previous suite replaced `document.fonts.check()` with a fake that encoded + * the *assumption* it reports absence. That is how a picker which listed every + * family on the machine, installed or not, stayed green. + */ +function stubFontMatching( + installed: readonly string[], + options: { refuse?: boolean } = {}, +): string[] { + const sources: string[] = []; + class FakeFontFace { + family = ""; + + constructor(_name: string, source: string) { + // An engine can refuse the lookup itself, e.g. a malformed source. + if (options.refuse) throw new DOMException("refused", "SyntaxError"); + sources.push(source); + this.family = /^local\("(.*)"\)$/.exec(source)?.[1] ?? ""; + } -function stubFonts(installed: string[]): void { + load(): Promise { + return installed.includes(this.family) + ? Promise.resolve(this) + : Promise.reject( + new DOMException(`${this.family} is not available`, "NetworkError"), + ); + } + } + vi.stubGlobal("FontFace", FakeFontFace); + return sources; +} + +/** The engine's own answer about availability, for the record: true for anything. */ +function stubCheckAlwaysTrue(): Mock<() => boolean> { + const check = vi.fn(() => true); Object.defineProperty(document, "fonts", { configurable: true, - value: { - check: (font: string) => - installed.some((family) => font.includes(`"${family}"`)), - }, + value: { check }, }); + return check; } +afterEach(() => { + vi.unstubAllGlobals(); + Reflect.deleteProperty(document, "fonts"); +}); + describe("isFontAvailable", () => { - it("reports nothing when the Font Loading API is absent", () => { - // jsdom and older WebViews have no document.fonts. Claiming every family - // exists there would offer the user settings that do nothing. - Reflect.deleteProperty(document, "fonts"); - expect(isFontAvailable("Inter")).toBe(false); + it("does not consult document.fonts.check, which reports true for absent families", async () => { + // Measured on Edge/WebView2 153.0.4234 — the engine Tauri uses on Windows — + // where check() answered true for all 39 families in a sweep that included + // `__Absent Font 12345__`: an unmatched family still renders through the + // fallback. Asking it offers every candidate on every machine. + const check = stubCheckAlwaysTrue(); + stubFontMatching(["Inter"]); + + expect(check()).toBe(true); // the engine's answer, for the record + check.mockClear(); + + expect(await isFontAvailable("__Absent Font 12345__")).toBe(false); + expect(check).not.toHaveBeenCalled(); }); - it("asks the document rather than guessing", () => { - stubFonts(["Inter"]); - expect(isFontAvailable("Inter")).toBe(true); - expect(isFontAvailable("Definitely Not Installed")).toBe(false); + it("resolves a family the machine has", async () => { + stubFontMatching(["Inter"]); + + expect(await isFontAvailable("Inter")).toBe(true); }); - it("survives a family name that would break the shorthand", () => { + it("asks the engine about the family it was given", async () => { + const sources = stubFontMatching(["Inter"]); + + await isFontAvailable("Inter"); + + expect(sources).toEqual(['local("Inter")']); + }); + + it("reports a family the machine lacks as unavailable", async () => { + stubFontMatching(["Inter"]); + + expect(await isFontAvailable("Definitely Not Installed")).toBe(false); + }); + + it("survives a family name that would end the lookup string", async () => { + const sources = stubFontMatching(["broken"]); + + expect(await isFontAvailable('bro"ken')).toBe(true); + expect(sources).toEqual(['local("broken")']); + }); + + it("reports nothing when the engine has no FontFace", async () => { + // jsdom, and a WebView without the Font Loading API: claim nothing rather + // than offer the user settings that do nothing. + vi.stubGlobal("FontFace", undefined); + + expect(await isFontAvailable("Inter")).toBe(false); + }); + + it("reports nothing when the engine refuses the lookup", async () => { + stubFontMatching(["Inter"], { refuse: true }); + + expect(await isFontAvailable("Inter")).toBe(false); + }); + + it("leaves document.fonts untouched", async () => { + // Probing must not register anything: the face is loaded to ask a question, + // not to be used for rendering. + const add = vi.fn(); Object.defineProperty(document, "fonts", { configurable: true, - value: { - check: () => { - throw new SyntaxError("bad font shorthand"); - }, - }, + value: { add }, }); - expect(isFontAvailable('bro"ken')).toBe(false); + stubFontMatching(["Inter"]); + + await isFontAvailable("Inter"); + + expect(add).not.toHaveBeenCalled(); }); }); describe("availableFontCandidates", () => { - it("offers only what is installed", () => { - stubFonts(["Inter", "PingFang SC"]); - expect(availableFontCandidates().map((c) => c.family)).toEqual([ + it("offers only what the machine has, in declaration order", async () => { + stubFontMatching(["PingFang SC", "Consolas", "Inter"]); + + expect((await availableFontCandidates()).map((c) => c.family)).toEqual([ "Inter", + "Consolas", "PingFang SC", ]); }); - it("returns nothing rather than the whole list when probing is impossible", () => { - Reflect.deleteProperty(document, "fonts"); - expect(availableFontCandidates()).toEqual([]); + it("returns nothing rather than the whole list when probing is impossible", async () => { + vi.stubGlobal("FontFace", undefined); + + expect(await availableFontCandidates()).toEqual([]); }); it("covers each group so the picker is useful on any platform", () => { diff --git a/desktop/src/app/fontCandidates.ts b/desktop/src/app/fontCandidates.ts index ab760f6a0..e7f6b19de 100644 --- a/desktop/src/app/fontCandidates.ts +++ b/desktop/src/app/fontCandidates.ts @@ -4,9 +4,25 @@ * A plain dropdown of font names would be a hardcoded guess: the list that is * right on a Windows box with Microsoft YaHei is wrong on a Mac with PingFang, * and offering a family the system lacks produces a setting that silently does - * nothing. `document.fonts.check()` answers the question directly, so the - * candidates below are only ever *suggestions* — the UI shows the survivors - * and still accepts free text for anything not listed. + * nothing. The candidates below are therefore only ever *suggestions* — the UI + * shows the survivors and still accepts free text for anything not listed. + * + * Presence comes from a `local()` lookup, not from `document.fonts.check()`. + * That call is the obvious API and it does not answer this question: it reports + * whether the text *can* be rendered, and a family the machine lacks still + * renders through the fallback. Measured on Edge/WebView2 153.0.4234 — the + * engine Tauri uses on Windows — `check()` answered true for all 39 families in + * a sweep that included `__Absent Font 12345__`, so every candidate survived + * the filter and the picker offered fonts the machine did not have. `local()` + * goes through font matching instead: it rejected every family with no font on + * the machine (Roboto, Helvetica, PingFang SC, …) while resolving the rest, and + * agreed with the Windows font registry on every name it was asked about. + * `queryLocalFonts()` would enumerate the system list directly, but it needs a + * user gesture and a permission grant, so it cannot back a picker that is + * populated when the settings page opens. + * + * The probe is asynchronous, and an engine that cannot probe reports nothing + * rather than pretending every family exists. */ export interface FontCandidate { @@ -21,7 +37,11 @@ export interface FontCandidate { */ export const FONT_CANDIDATES: readonly FontCandidate[] = [ { family: "Inter", group: "Interface" }, - { family: "Segoe UI Variable", group: "Interface" }, + // Windows 11 ships its UI face as one variable font, and the system exposes + // its optical sizes as separate families; the bare "Segoe UI Variable" is not + // one of them. Measured on this engine: a local() lookup of the bare name + // fails, while "… Text" — the size used at body text — resolves. + { family: "Segoe UI Variable Text", group: "Interface" }, { family: "Segoe UI", group: "Interface" }, { family: "SF Pro Text", group: "Interface" }, { family: "Helvetica Neue", group: "Interface" }, @@ -39,26 +59,44 @@ export const FONT_CANDIDATES: readonly FontCandidate[] = [ { family: "Hiragino Sans GB", group: "CJK" }, ]; +/** Name for the throwaway face a probe loads. It is never registered. */ +const PROBE_FACE_NAME = "deepcode-font-probe"; + +/** Quote a family for a `local()` source, dropping characters that would end it. */ +function quote(family: string): string { + return `"${family.replaceAll('"', "").replaceAll("\\", "")}"`; +} + /** - * Whether `family` resolves on this machine. + * Whether the engine resolves `family` by name. * - * `document.fonts.check` needs a full font shorthand and throws on a malformed - * one, so the family is quoted and the call is guarded. An environment without - * the Font Loading API (jsdom, an old WebView) reports nothing rather than - * pretending every family exists. + * `local()` is the lookup that fails for a family the machine lacks, so a + * failed load is the negative answer. An engine without `FontFace` — or one + * that refuses local lookups — reports nothing rather than claiming every + * family exists. The face is never added to `document.fonts`, so probing + * leaves no trace. */ -export function isFontAvailable(family: string): boolean { - if (typeof document === "undefined" || !document.fonts?.check) return false; +export async function isFontAvailable(family: string): Promise { + if (typeof FontFace === "undefined") return false; try { - return document.fonts.check(`12px "${family.replaceAll('"', "")}"`); + await new FontFace(PROBE_FACE_NAME, `local(${quote(family)})`).load(); + return true; } catch { return false; } } /** The candidates present on this machine, in declaration order. */ -export function availableFontCandidates(): FontCandidate[] { - return FONT_CANDIDATES.filter((candidate) => isFontAvailable(candidate.family)); +export async function availableFontCandidates(): Promise { + const verdicts = await Promise.all( + FONT_CANDIDATES.map(async (candidate) => ({ + candidate, + present: await isFontAvailable(candidate.family), + })), + ); + return verdicts + .filter((verdict) => verdict.present) + .map((verdict) => verdict.candidate); } /** Append `family` to a comma-separated list, ignoring duplicates. */ diff --git a/desktop/src/features/settings/AppearanceSettings.tsx b/desktop/src/features/settings/AppearanceSettings.tsx index c5d9ff5ac..be004ba8f 100644 --- a/desktop/src/features/settings/AppearanceSettings.tsx +++ b/desktop/src/features/settings/AppearanceSettings.tsx @@ -1,5 +1,5 @@ import { Monitor, Moon, Sun } from "lucide-react"; -import { useMemo, useId, useState } from "react"; +import { useEffect, useId, useState } from "react"; import { APPEARANCE_DEFAULTS, @@ -11,6 +11,7 @@ import { import { appendFamily, availableFontCandidates, + type FontCandidate, } from "../../app/fontCandidates"; import { useAppearance } from "../../app/useAppearance"; import { parseVsCodeTheme, ThemeImportError } from "../../app/importedTheme"; @@ -82,9 +83,13 @@ export function AppearanceSettings() { const { t } = useTranslation(); const fieldId = useId(); const [importError, setImportError] = useState(null); - // Probed once per mount: the set of installed fonts does not change while - // the settings page is open. - const installed = useMemo(() => availableFontCandidates(), []); + // The installed set is probed once per mount — it does not change while the + // settings page is open — and the probe is asynchronous because the engine's + // only reliable answer, a `local()` font load, is. + const [installed, setInstalled] = useState([]); + useEffect(() => { + void availableFontCandidates().then(setInstalled); + }, []); const isDefault = APPEARANCE_SETTINGS.every( (setting) => appearance[setting.key] === APPEARANCE_DEFAULTS[setting.key], ) && appearance.importedTheme === null;