From 0389435cede669c9d85c6b57da5797f526f05be2 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Mon, 28 Sep 2026 18:33:08 +0200 Subject: [PATCH] :bug: Block typography creation from missing fonts or multiple texts (#11938) Creating a typography from a text whose font is no longer installed baked the broken font-id into a new asset. With several texts selected there is no single style to capture either. The add-typography event now does nothing in both cases, and the "Add typography" button in the local library is disabled with a label that explains why. The button lives in its own component so selection, shape and editor changes re-render only the button, not the typography list, and shared libraries do not subscribe to that state at all. The event and the button read the editor data through the new `editor-text-options`, so both see the same font for the wasm, v2 and v1 text editors. AI-assisted-by: claude-opus-5-5 --- .../workspace-assets-add-typography.spec.js | 131 ++++++++++++++++++ .../app/main/data/workspace/texts_events.cljs | 46 ++++-- .../sidebar/assets/typographies.cljs | 75 ++++++++-- .../data/workspace_texts_test.cljs | 68 +++++++++ frontend/translations/en.po | 4 + frontend/translations/es.po | 4 + 6 files changed, 305 insertions(+), 23 deletions(-) create mode 100644 frontend/playwright/ui/specs/workspace-assets-add-typography.spec.js diff --git a/frontend/playwright/ui/specs/workspace-assets-add-typography.spec.js b/frontend/playwright/ui/specs/workspace-assets-add-typography.spec.js new file mode 100644 index 0000000000..b9639cb7a0 --- /dev/null +++ b/frontend/playwright/ui/specs/workspace-assets-add-typography.spec.js @@ -0,0 +1,131 @@ +import { test, expect } from "@playwright/test"; +import { WorkspacePage } from "../pages/WorkspacePage"; + +// --------------------------------------------------------------------------- +// The "Add typography" button in the local library typographies section +// (src/app/main/ui/workspace/sidebar/assets/typographies.cljs) creates a +// typography from the selected text. It is disabled, and its label explains +// why, when: +// - the selected text uses a font that is not installed +// - two or more texts are selected +// --------------------------------------------------------------------------- + +function addTypographyButton(workspace, name) { + return workspace.sidebar.getByRole("button", { + name, + exact: typeof name === "string", + }); +} + +test.beforeEach(async ({ page }) => { + await WorkspacePage.init(page); +}); + +test.describe("font of the selected text", () => { + // render-wasm/get-file-text-custom-fonts.json has a text shape + // ("Penpot & Dragons") using a custom team font-id. The get-font-variants + // mock decides whether that font is installed or missing. + const FILE = { + id: "434b0541-fa2f-802f-8006-59827d964a9b", + pageId: "434b0541-fa2f-802f-8006-59827d964a9c", + }; + + test("Add typography is enabled when the selected text font is installed", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "render-wasm/get-file-text-custom-fonts.json", + ); + await workspace.mockRPC( + "get-font-variants?team-id=*", + "render-wasm/get-font-variants-custom-fonts.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Penpot & Dragons"); + await workspace.clickAssets(); + + await expect( + addTypographyButton(workspace, "Add typography"), + ).toBeEnabled(); + }); + + test("Add typography is disabled when the selected text font is missing", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "render-wasm/get-file-text-custom-fonts.json", + ); + await workspace.mockRPC( + "get-font-variants?team-id=*", + "workspace/get-font-variants-empty.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Penpot & Dragons"); + await workspace.clickAssets(); + + await expect( + addTypographyButton(workspace, /is no longer available/), + ).toBeDisabled(); + }); +}); + +test.describe("number of selected texts", () => { + // workspace/get-file-text-multiple-selection.json has two text shapes that + // use the same built-in font, so only the selection count changes. + const FILE = { + id: "434b0541-fa2f-802f-8006-6a827d964a9b", + pageId: "434b0541-fa2f-802f-8006-6a827d964a9c", + }; + + test("Add typography is disabled when two texts are selected", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "workspace/get-file-text-multiple-selection.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Text multiple selection one"); + await workspace.clickLeafLayer("Text multiple selection two", { + modifiers: ["Shift"], + }); + await workspace.clickAssets(); + + await expect( + addTypographyButton( + workspace, + "Select one text to create a typography style", + ), + ).toBeDisabled(); + }); + + test("Add typography is enabled when one text is selected", async ({ + page, + }) => { + const workspace = new WorkspacePage(page); + await workspace.setupEmptyFile(); + await workspace.mockRPC( + /get\-file\?/, + "workspace/get-file-text-multiple-selection.json", + ); + await workspace.goToWorkspace({ fileId: FILE.id, pageId: FILE.pageId }); + + await workspace.clickLeafLayer("Text multiple selection one"); + await workspace.clickAssets(); + + await expect( + addTypographyButton(workspace, "Add typography"), + ).toBeEnabled(); + }); +}); diff --git a/frontend/src/app/main/data/workspace/texts_events.cljs b/frontend/src/app/main/data/workspace/texts_events.cljs index 4dc4591cf9..b9ef657f76 100644 --- a/frontend/src/app/main/data/workspace/texts_events.cljs +++ b/frontend/src/app/main/data/workspace/texts_events.cljs @@ -7,7 +7,6 @@ (ns app.main.data.workspace.texts-events (:require [app.common.data :as d] - [app.common.data.macros :as dm] [app.common.files.helpers :as cfh] [app.common.math :as mth] [app.common.types.text :as txt] @@ -18,9 +17,24 @@ [app.main.data.workspace.libraries :as dwl] [app.main.data.workspace.pages :as-alias dwpg] [app.main.data.workspace.texts :as dwt] + [app.main.features :as features] + [app.main.fonts :as fonts] [beicon.v2.core :as rx] [potok.v2.core :as ptk])) +(defn editor-text-options + "Editor data that `dwt/current-text-values` needs for `shape-id`, + taken from the text editor that is active in `state`." + [state shape-id] + (let [wasm? (features/active-feature? state "text-editor-wasm/v1") + v2? (features/active-feature? state "text-editor/v2") + state-map (if wasm? + (:workspace-wasm-editor-styles state) + (:workspace-editor-state state))] + {:editor-styles (when wasm? (get state-map shape-id)) + :editor-state (when-not v2? (get state-map shape-id)) + :editor-instance (when v2? (:workspace-editor state))})) + ;; This function must be separated from app.main.data.workspace.texts to avoid a circular ;; dependency due main.data.workspace.libraries eventually calling app.main.data.workspace.texts. @@ -45,14 +59,20 @@ shape (first shapes) values (dwt/current-text-values - {:editor-state (dm/get-in state [:workspace-editor-state (:id shape)]) - :shape shape - :attrs txt/text-node-attrs}) + (assoc (editor-text-options state (:id shape)) + :shape shape + :attrs txt/text-node-attrs)) multiple? (or (> 1 (count shapes)) (d/seek (partial = :multiple) (vals values))) + too-many-texts? (> (count shapes) 1) + + font-missing? (and (not multiple?) + (some? (:font-id values)) + (not (fonts/installed? (:font-id values)))) + values (-> (d/without-nils values) (select-keys (d/concat-vec txt/text-font-attrs @@ -74,12 +94,14 @@ (cond-> (string? group-path) (update :name #(str group-path " / " %))))] - (rx/concat - (rx/of (dwl/add-typography typ) - (ev/event {::ev/name "add-asset-to-library" - :asset-type "typography"})) + (if (or font-missing? too-many-texts?) + (rx/empty) + (rx/concat + (rx/of (dwl/add-typography typ) + (ev/event {::ev/name "add-asset-to-library" + :asset-type "typography"})) - (when (not multiple?) - (rx/of (dwt/update-attrs (:id shape) - {:typography-ref-id typ-id - :typography-ref-file file-id}))))))))) + (when (not multiple?) + (rx/of (dwt/update-attrs (:id shape) + {:typography-ref-id typ-id + :typography-ref-file file-id})))))))))) diff --git a/frontend/src/app/main/ui/workspace/sidebar/assets/typographies.cljs b/frontend/src/app/main/ui/workspace/sidebar/assets/typographies.cljs index 5ad9cae78e..f9ec09486f 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/assets/typographies.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/assets/typographies.cljs @@ -9,7 +9,9 @@ (:require [app.common.data :as d] [app.common.data.macros :as dm] + [app.common.files.helpers :as cfh] [app.common.path-names :as cpn] + [app.common.types.text :as txt] [app.main.data.event :as ev] [app.main.data.modal :as modal] [app.main.data.workspace :as dw] @@ -17,6 +19,7 @@ [app.main.data.workspace.texts :as dwt] [app.main.data.workspace.texts-events :as dwte] [app.main.data.workspace.undo :as dwu] + [app.main.fonts :as fonts] [app.main.refs :as refs] [app.main.store :as st] [app.main.ui.context :as ctx] @@ -243,6 +246,66 @@ :selected-full selected-full :is-read-only is-read-only}]))])])) +;; Kept apart from `typographies-section*` so that selection, shape and +;; editor changes only re-render this button, not the typography list. +(mf/defc add-typography-button* + {::mf/private true} + [{:keys [file-id]}] + (let [selected (mf/deref refs/selected-shapes) + objects (mf/deref refs/workspace-page-objects) + text-shapes (mf/with-memo [selected objects] + (into [] + (comp (keep (d/getf objects)) + (filter cfh/text-shape?)) + selected)) + many-texts? (> (count text-shapes) 1) + shape (when-not many-texts? (first text-shapes)) + shape-id (:id shape) + + ;; The v2 editor instance is mutable, so its per-shape state is + ;; included only to re-render when the editor styles change. + editor-ref (mf/with-memo [shape-id] + (l/derived (fn [state] + (when shape-id + [(dwte/editor-text-options state shape-id) + (dm/get-in state [:workspace-v2-editor-state shape-id])])) + st/state =)) + editor-data (mf/deref editor-ref) + loaded-fonts (mf/deref fonts/fontsdb) + + values (mf/with-memo [shape editor-data] + (when shape + (dwt/current-text-values + (assoc (first editor-data) + :shape shape + :attrs txt/text-node-attrs)))) + font-id (:font-id values) + font-missing? (and (some? font-id) + (not (d/seek (partial = :multiple) (vals values))) + (not (contains? loaded-fonts font-id))) + + on-click + (mf/use-fn + (mf/deps file-id) + (fn [_] + (st/emit! (dw/set-assets-section-open file-id :typographies true)) + (st/emit! (dwte/add-typography file-id))))] + + [:> icon-button* {:variant "ghost" + :aria-label (cond + many-texts? + (tr "workspace.assets.typography.multiple-texts-selected") + + font-missing? + (tr "workspace.options.font-not-available" + (:font-family values)) + + :else + (tr "workspace.assets.typography.add-typography")) + :on-click on-click + :disabled (or many-texts? font-missing?) + :icon i/add}])) + (mf/defc typographies-section* [{:keys [file file-id typographies open-status-ref selected is-local is-open is-force-open is-reverse-sort @@ -274,13 +337,6 @@ open-groups (mf/deref open-groups-ref) - add-typography - (mf/use-fn - (mf/deps file-id) - (fn [_] - (st/emit! (dw/set-assets-section-open file-id :typographies true)) - (st/emit! (dwte/add-typography file-id)))) - handle-change (mf/use-fn (mf/deps file-id) @@ -427,10 +483,7 @@ (when is-local [:> cmm/asset-section-block* {:role :title-button} (when-not read-only? - [:> icon-button* {:variant "ghost" - :aria-label (tr "workspace.assets.typography.add-typography") - :on-click add-typography - :icon i/add}])]) + [:> add-typography-button* {:file-id file-id}])]) [:> cmm/asset-section-block* {:role :content} [:& typographies-group {:file-id file-id diff --git a/frontend/test/frontend_tests/data/workspace_texts_test.cljs b/frontend/test/frontend_tests/data/workspace_texts_test.cljs index 252a86cc09..6994f1a219 100644 --- a/frontend/test/frontend_tests/data/workspace_texts_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_texts_test.cljs @@ -379,6 +379,74 @@ (t/is (= "0.1" (:letter-spacing (first typographies))) "float letter-spacing is normalised to 2-decimal string"))))))) +;; --------------------------------------------------------------------------- +;; Tests: add-typography must not create an asset from a deleted font +;; +;; A text shape can keep referencing a font-id that is no longer installed +;; (the font was removed, or belongs to an unavailable library). Creating a +;; typography from it would bake that broken font-id into a brand-new asset. +;; --------------------------------------------------------------------------- + +(t/deftest add-typography-skips-shape-with-deleted-font + (t/async + done + (let [content (txt/change-text nil "hello" :font-id "deleted-font-id") + file (-> (cthf/sample-file :file1) + (cths/add-sample-shape :text1 + :type :text + :x 0 :y 0 + :content content)) + shape-id (:id (cths/get-shape file :text1)) + file-id (:id file) + store (ths/setup-store file) + events [(fn [state] (assoc-in state [:workspace-local :selected] #{shape-id})) + (dwte/add-typography file-id)]] + + (ths/run-store + store done events + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + typographies (vals (get-in file' [:data :typographies])) + shape' (cths/get-shape file' :text1)] + (t/is (= 0 (count typographies)) + "no typography was added for a shape with a deleted font") + (t/is (nil? (:typography-ref-id shape')) + "the shape was not linked to a typography"))))))) + +;; --------------------------------------------------------------------------- +;; Tests: add-typography must not create an asset from two or more selected texts +;; +;; Deriving a typography from a specific text only makes sense for a single +;; shape; with several texts selected there is no one style to capture. +;; --------------------------------------------------------------------------- + +(t/deftest add-typography-skips-multiple-selected-texts + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (cths/add-sample-shape :text1 + :type :text + :x 0 :y 0 + :content (txt/change-text nil "hello")) + (cths/add-sample-shape :text2 + :type :text + :x 0 :y 100 + :content (txt/change-text nil "world"))) + shape-id-1 (:id (cths/get-shape file :text1)) + shape-id-2 (:id (cths/get-shape file :text2)) + file-id (:id file) + store (ths/setup-store file) + events [(fn [state] (assoc-in state [:workspace-local :selected] #{shape-id-1 shape-id-2})) + (dwte/add-typography file-id)]] + + (ths/run-store + store done events + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + typographies (vals (get-in file' [:data :typographies]))] + (t/is (= 0 (count typographies)) + "no typography was added when two texts are selected"))))))) + ;; --------------------------------------------------------------------------- ;; Tests: save-default-font must not persist typography refs into the global default font ;; diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 0ccebacee0..815de4addb 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -6245,6 +6245,10 @@ msgstr "Typographies" msgid "workspace.assets.typography.add-typography" msgstr "Add typography" +#: src/app/main/ui/workspace/sidebar/assets/typographies.cljs:461 +msgid "workspace.assets.typography.multiple-texts-selected" +msgstr "Select one text to create a typography style" + #: src/app/main/ui/workspace/sidebar/options/menus/typography.cljs:855 msgid "workspace.assets.typography.font-size" msgstr "Size" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index 0726cbb4d3..2d22dedfdf 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -6314,6 +6314,10 @@ msgstr "Tipografías" msgid "workspace.assets.typography.add-typography" msgstr "Añadir tipografía" +#: src/app/main/ui/workspace/sidebar/assets/typographies.cljs:461 +msgid "workspace.assets.typography.multiple-texts-selected" +msgstr "Selecciona un solo texto para crear un estilo de tipografía" + #: src/app/main/ui/workspace/sidebar/options/menus/typography.cljs:855 msgid "workspace.assets.typography.font-size" msgstr "Tamaño"