🐛 Fix multiselection error with typography texts (#9071)

* 🐛 Ensure typography-ref attrs are always present and fix nil encoding

Add :typography-ref-file and :typography-ref-id (both defaulting to nil)
to default-text-attrs so these keys are always present in text node maps,
whether or not a typography is attached.

Skip nil values in attrs-to-styles (Draft.js style encoder) and in
attrs->styles (v2 CSS custom-property mapper) so nil typography-ref
entries are never serialised to CSS.

Replace when with if/acc in get-styles-from-style-declaration to prevent
the accumulator from being clobbered to nil when a mixed-value entry is
skipped during style decoding.

* 🎉 Add test

---------

Co-authored-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
Eva Marco 2026-04-23 16:08:56 +02:00 committed by GitHub
parent 96722fde4b
commit 0c60db56a2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 2089 additions and 12 deletions

View File

@ -37,7 +37,9 @@
(defn attrs-to-styles
[attrs]
(reduce-kv (fn [res k v]
(conj res (encode-style k v)))
(if (some? v)
(conj res (encode-style k v))
res))
#{}
attrs))

View File

@ -95,7 +95,9 @@
:text-direction "ltr"})
(def default-text-attrs
{:font-id "sourcesanspro"
{:typography-ref-file nil
:typography-ref-id nil
:font-id "sourcesanspro"
:font-family "sourcesanspro"
:font-variant-id "regular"
:font-size "14"

View File

@ -0,0 +1,151 @@
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS INC
(ns common-tests.attrs-test
(:require
[app.common.attrs :as attrs]
[clojure.test :as t]))
(t/deftest get-attrs-multi-same-value
(t/testing "returns value when all objects have the same attribute value"
(let [objs [{:attr "red"}
{:attr "red"}
{:attr "red"}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {:attr "red"} result))))
(t/testing "returns nil when all objects have nil value"
(let [objs [{:attr nil}
{:attr nil}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {:attr nil} result)))))
(t/deftest get-attrs-multi-different-values
(t/testing "returns :multiple when objects have different concrete values"
(let [objs [{:attr "red"}
{:attr "blue"}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {:attr :multiple} result)))))
(t/deftest get-attrs-multi-missing-key
(t/testing "returns value when one object has the attribute and another doesn't"
(let [objs [{:attr "red"}
{:other "value"}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {:attr "red"} result))))
(t/testing "returns value when one object has UUID and another is missing"
(let [uuid #uuid "550e8400-e29b-41d4-a716-446655440000"
objs [{:attr uuid}
{:other "value"}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {:attr uuid} result))))
(t/testing "returns :multiple when some objects have the key and some don't"
(let [objs [{:attr "red"}
{:other "value"}
{:attr "blue"}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {:attr :multiple} result))))
(t/testing "returns nil when one object has nil and another is missing"
(let [objs [{:attr nil}
{:other "value"}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {:attr nil} result)))))
(t/deftest get-attrs-multi-all-missing
(t/testing "all missing → attribute NOT included in result"
(let [objs [{:other "value"}
{:different "data"}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {} result)
"Attribute should not be in result when all objects are missing")))
(t/testing "all missing with empty maps → attribute NOT included"
(let [objs [{} {}]
result (attrs/get-attrs-multi objs [:attr])]
(t/is (= {} result)
"Attribute should not be in result"))))
(t/deftest get-attrs-multi-multiple-attributes
(t/testing "handles multiple attributes with different merge results"
(let [objs [{:attr1 "red" :attr2 "blue"}
{:attr1 "red" :attr2 "green"}
{:attr1 "red"}] ; :attr2 missing
result (attrs/get-attrs-multi objs [:attr1 :attr2])]
(t/is (= {:attr1 "red" :attr2 :multiple} result))))
(t/testing "handles mixed scenarios: same, different, and missing"
(let [uuid #uuid "550e8400-e29b-41d4-a716-446655440000"
uuid2 #uuid "550e8400-e29b-41d4-a716-446655440001"
objs [{:id :a :ref uuid}
{:id :b :ref uuid2}
{:id :c}] ; :ref missing
result (attrs/get-attrs-multi objs [:id :ref])]
(t/is (= {:id :multiple :ref :multiple} result)))))
(t/deftest get-attrs-multi-typography-ref-id-scenario
(t/testing "the specific bug scenario: typography-ref-id with UUID vs missing"
(let [uuid #uuid "550e8400-e29b-41d4-a716-446655440000"
;; Shape 1 has typography-ref-id with a UUID
shape1 {:id :shape1 :typography-ref-id uuid}
;; Shape 2 does NOT have typography-ref-id at all
shape2 {:id :shape2}
result (attrs/get-attrs-multi [shape1 shape2] [:typography-ref-id])]
(t/is (= {:typography-ref-id uuid} result))))
(t/testing "both shapes missing → attribute NOT included in result"
(let [shape1 {:id :shape1}
shape2 {:id :shape2}
result (attrs/get-attrs-multi [shape1 shape2] [:typography-ref-id])]
(t/is (= {} result)
"Expected empty map when all shapes are missing the attribute"))))
(t/deftest get-attrs-multi-bug-missing-vs-present
(t/testing "BUG FIXED: one shape has :typography-ref-id, other does NOT → returns uuid"
(let [uuid #uuid "550e8400-e29b-41d4-a716-446655440000"
shape1 {:id :shape1 :typography-ref-id uuid}
shape2 {:id :shape2}
result (attrs/get-attrs-multi [shape1 shape2] [:typography-ref-id])]
(t/is (= {:typography-ref-id uuid} result))))
(t/testing "both missing → empty map (attribute not in result)"
(let [shape1 {:id :shape1}
shape2 {:id :shape2}
result (attrs/get-attrs-multi [shape1 shape2] [:typography-ref-id])]
(t/is (= {} result)
"Expected empty map when all shapes are missing the attribute")))
(t/testing "both equal values → return the value"
(let [uuid #uuid "550e8400-e29b-41d4-a716-446655440000"
shape1 {:id :shape1 :typography-ref-id uuid}
shape2 {:id :shape2 :typography-ref-id uuid}
result (attrs/get-attrs-multi [shape1 shape2] [:typography-ref-id])]
(t/is (= {:typography-ref-id uuid} result))))
(t/testing "different values → return :multiple"
(let [uuid1 #uuid "550e8400-e29b-41d4-a716-446655440000"
uuid2 #uuid "550e8400-e29b-41d4-a716-446655440001"
shape1 {:id :shape1 :typography-ref-id uuid1}
shape2 {:id :shape2 :typography-ref-id uuid2}
result (attrs/get-attrs-multi [shape1 shape2] [:typography-ref-id])]
(t/is (= {:typography-ref-id :multiple} result)))))
(t/deftest get-attrs-multi-default-equal
(t/testing "numbers use close? for equality"
(let [objs [{:value 1.0}
{:value 1.0000001}]
result (attrs/get-attrs-multi objs [:value])]
(t/is (= {:value 1.0} result)
"Numbers within tolerance should be considered equal")))
(t/testing "different floating point positions beyond tolerance are :multiple"
(let [objs [{:x -26}
{:x -153}]
result (attrs/get-attrs-multi objs [:x])]
(t/is (= {:x :multiple} result)
"Different positions should be :multiple"))))

View File

@ -8,6 +8,7 @@
(:require
#?(:clj [common-tests.fressian-test])
[clojure.test :as t]
[common-tests.attrs-test]
[common-tests.buffer-test]
[common-tests.colors-test]
[common-tests.data-test]
@ -85,6 +86,7 @@
(defn -main
[& args]
(t/run-tests
'common-tests.attrs-test
'common-tests.buffer-test
'common-tests.colors-test
'common-tests.data-test

File diff suppressed because it is too large Load Diff

View File

@ -0,0 +1,258 @@
import { test, expect } from "@playwright/test";
import { WasmWorkspacePage } from "../pages/WasmWorkspacePage";
test.beforeEach(async ({ page }) => {
await WasmWorkspacePage.init(page);
});
test("Multiselection - check multiple values in measures", async ({ page }) => {
const workspacePage = new WasmWorkspacePage(page);
await workspacePage.setupEmptyFile(page);
await workspacePage.mockRPC(
/get\-file\?/,
"workspace/get-file-copy-paste.json",
);
await workspacePage.mockRPC(
"get-file-fragment?file-id=*&fragment-id=*",
"workspace/get-file-copy-paste-fragment.json",
);
await workspacePage.goToWorkspace({
fileId: "870f9f10-87b5-8137-8005-934804124660",
pageId: "870f9f10-87b5-8137-8005-934804124661",
});
// Select first shape (single selection first)
await page.getByTestId("layer-item").getByRole("button").first().click();
await workspacePage.layers.getByTestId("layer-row").nth(0).click();
// === CHECK SINGLE SELECTION - ALL MEASURE FIELDS ===
const measuresSection = workspacePage.rightSidebar.getByRole('region', { name: 'shape-measures-section' });
await expect(measuresSection).toBeVisible();
// Width
const widthInput = measuresSection.getByTitle('Width', { exact: true }).getByRole('textbox');
await expect(widthInput).toHaveValue("360");
// Height
const heightInput = measuresSection.getByTitle('Height', { exact: true }).getByRole('textbox');
await expect(heightInput).toHaveValue("53");
// X Position (using "X axis" title)
const xPosInput = measuresSection.getByTitle('X axis', { exact: true }).getByRole('textbox');
await expect(xPosInput).toHaveValue("1094");
// Y Position (using "Y axis" title)
const yPosInput = measuresSection.getByTitle('Y axis', { exact: true }).getByRole('textbox');
await expect(yPosInput).toHaveValue("856");
// === CHECK MULTI-SELECTION - MIXED VALUES ===
// Shift+click to add second layer to selection
await workspacePage.layers.getByTestId("layer-row").nth(1).click({ modifiers: ['Shift'] });
// All measure fields should show "Mixed" placeholder when values differ
await expect(widthInput).toHaveAttribute('placeholder', 'Mixed');
await expect(heightInput).toHaveAttribute('placeholder', 'Mixed');
await expect(xPosInput).toHaveAttribute('placeholder', 'Mixed');
await expect(yPosInput).toHaveAttribute('placeholder', 'Mixed');
});
test("Multiselection - check fill multiple values", async ({ page }) => {
const workspacePage = new WasmWorkspacePage(page);
await workspacePage.setupEmptyFile(page);
await workspacePage.mockRPC(
/get\-file\?/,
"workspace/get-file-copy-paste.json",
);
await workspacePage.mockRPC(
"get-file-fragment?file-id=*&fragment-id=*",
"workspace/get-file-copy-paste-fragment.json",
);
await workspacePage.goToWorkspace({
fileId: "870f9f10-87b5-8137-8005-934804124660",
pageId: "870f9f10-87b5-8137-8005-934804124661",
});
await page.getByTestId("layer-item").getByRole("button").first().click();
await workspacePage.layers.getByTestId("layer-row").nth(0).click();
// Fill section
const fillSection = workspacePage.rightSidebar.getByRole('region', { name: "Fill section" });
await expect(fillSection).toBeVisible();
// Single selection - fill color should be visible (not "Mixed")
await expect(fillSection.getByText(/Mixed/i)).not.toBeVisible();
// Multi-selection with Shift+click
await workspacePage.layers.getByTestId("layer-row").nth(1).click({ modifiers: ['Shift'] });
// Should show "Mixed" for fills when shapes have different fill colors
await expect(fillSection.getByText('Mixed')).toBeVisible();
});
test("Multiselection - check stroke multiple values", async ({ page }) => {
const workspacePage = new WasmWorkspacePage(page);
await workspacePage.setupEmptyFile(page);
await workspacePage.mockRPC(
/get\-file\?/,
"workspace/get-file-copy-paste.json",
);
await workspacePage.mockRPC(
"get-file-fragment?file-id=*&fragment-id=*",
"workspace/get-file-copy-paste-fragment.json",
);
await workspacePage.goToWorkspace({
fileId: "870f9f10-87b5-8137-8005-934804124660",
pageId: "870f9f10-87b5-8137-8005-934804124661",
});
await page.getByTestId("layer-item").getByRole("button").first().click();
await workspacePage.layers.getByTestId("layer-row").nth(0).click();
// Stroke section
const strokeSection = workspacePage.rightSidebar.getByRole('region', { name: "Stroke section" });
await expect(strokeSection).toBeVisible();
// Single selection - stroke should be visible (not "Mixed")
await expect(strokeSection.getByText(/Mixed/i)).not.toBeVisible();
// Multi-selection
await workspacePage.layers.getByTestId("layer-row").nth(1).click({ modifiers: ['Shift'] });
// Should show "Mixed" for strokes when shapes have different stroke colors
await expect(strokeSection.getByText('Mixed')).toBeVisible();
});
test("Multiselection - check rotation multiple values", async ({ page }) => {
const workspacePage = new WasmWorkspacePage(page);
await workspacePage.setupEmptyFile(page);
await workspacePage.mockRPC(
/get\-file\?/,
"workspace/get-file-copy-paste.json",
);
await workspacePage.mockRPC(
"get-file-fragment?file-id=*&fragment-id=*",
"workspace/get-file-copy-paste-fragment.json",
);
await workspacePage.goToWorkspace({
fileId: "870f9f10-87b5-8137-8005-934804124660",
pageId: "870f9f10-87b5-8137-8005-934804124661",
});
await page.getByTestId("layer-item").getByRole("button").first().click();
await workspacePage.layers.getByTestId("layer-row").nth(1).click();
// Measures section contains rotation
const measuresSection = workspacePage.rightSidebar.getByRole('region', { name: 'shape-measures-section' });
await expect(measuresSection).toBeVisible();
// Rotation field exists
const rotationInput = measuresSection.getByTitle('Rotation', { exact: true }).getByRole('textbox');
await expect(rotationInput).toBeVisible();
// Rotate that shape
await rotationInput.fill("45");
await page.keyboard.press('Enter');
await expect(rotationInput).toHaveValue("45"); // Rotation should be 45
// Multi-selection
await workspacePage.layers.getByTestId("layer-row").nth(0).click({ modifiers: ['Shift'] });
// Rotation should show "Mixed" placeholder
await expect(rotationInput).toHaveAttribute('placeholder', 'Mixed');
});
test("Multiselection of text and typographies", async ({ page }) => {
const workspacePage = new WasmWorkspacePage(page);
await workspacePage.setupEmptyFile(page);
await workspacePage.mockRPC(
/get\-file\?/,
"workspace/multiselection-typography.json",
);
await workspacePage.goToWorkspace({
fileId: "1062e0a0-8fe0-80ae-8007-e70b4993f5ef",
pageId: "1062e0a0-8fe0-80ae-8007-e70b4993f5f0",
});
const plainTextLayer = workspacePage.layers.getByTestId("layer-row").nth(5);
const plainTextLayerTwo = workspacePage.layers.getByTestId("layer-row").nth(2);
const typographyTextLayerOne = workspacePage.layers.getByTestId("layer-row").nth(7);
const typographyTextLayerTwo = workspacePage.layers.getByTestId("layer-row").nth(4);
const tokenTypographyTextLayerOne = workspacePage.layers.getByTestId("layer-row").nth(6);
const tokenTypographyTextLayerTwo = workspacePage.layers.getByTestId("layer-row").nth(3);
const rectangleLayer = workspacePage.layers.getByTestId("layer-row").nth(1);
const elipseLayer = workspacePage.layers.getByTestId("layer-row").nth(0);
const textSection = workspacePage.rightSidebar.getByRole('region', { name: "Text section" });
// Select rectangle and elipse together
await rectangleLayer.click();
await elipseLayer.click({ modifiers: ['Control'] });
await expect(textSection).not.toBeVisible();
// Select plain text layer
await plainTextLayer.click();
await expect(textSection).toBeVisible();
await expect(textSection.getByText("Multiple typographies")).not.toBeVisible();
// Select two plain text layer with different font family
await plainTextLayerTwo.click({ modifiers: ['Control'] });
await expect(textSection).toBeVisible();
await expect(textSection.getByTitle("Font family").getByText("--")).toBeVisible();
// Select typography text layer
await typographyTextLayerOne.click();
await expect(textSection).toBeVisible();
await expect(textSection.getByText("Typography one")).toBeVisible();
// Select two typography text layer with different typography
await typographyTextLayerTwo.click({ modifiers: ['Control'] });
await expect(textSection).toBeVisible();
await expect(textSection.getByText("Multiple typographies")).toBeVisible();
// Select token typography text layer
// TODO: CHANGE WHEN TOKEN TYPOGRAPHY ROW IS READY
await tokenTypographyTextLayerOne.click();
await expect(textSection).toBeVisible();
await expect(textSection.getByText('Metrophobic')).toBeVisible();
// Select two token typography text layer with different token typography
// TODO: CHANGE WHEN TOKEN TYPOGRAPHY ROW IS READY
await tokenTypographyTextLayerTwo.click({ modifiers: ['Control'] });
await expect(textSection).toBeVisible();
await expect(textSection.getByTitle("Font family").getByText("--")).toBeVisible();
//Select plain text layer and typography text layer together
await plainTextLayer.click();
await typographyTextLayerOne.click({ modifiers: ['Control'] });
await expect(textSection).toBeVisible();
await expect(textSection.getByText("Multiple typographies")).toBeVisible();
//Select plain text layer and typography text layer together on reverse order
await typographyTextLayerOne.click();
await plainTextLayer.click({ modifiers: ['Control'] });
await expect(textSection).toBeVisible();
await expect(textSection.getByText("Multiple typographies")).toBeVisible();
//Selen token typography text layer and typography text layer together
await tokenTypographyTextLayerOne.click();
await typographyTextLayerOne.click({ modifiers: ['Control'] });
await expect(textSection).toBeVisible();
await expect(textSection.getByText("Multiple typographies")).toBeVisible();
//Select token typography text layer and typography text layer together on reverse order
await typographyTextLayerOne.click();
await tokenTypographyTextLayerOne.click({ modifiers: ['Control'] });
await expect(textSection).toBeVisible();
await expect(textSection.getByText("Multiple typographies")).toBeVisible();
// Select rectangle and elipse together
await rectangleLayer.click();
await elipseLayer.click({ modifiers: ['Control'] });
await expect(textSection).not.toBeVisible();
});

View File

@ -738,7 +738,7 @@ test.describe("Tokens: Apply token", () => {
// Check if token pill is visible on right sidebar
const strokeSectionSidebar = rightSidebar.getByRole("region", {
name: "stroke-section",
name: "Stroke section",
});
await expect(strokeSectionSidebar).toBeVisible();
const firstStrokeRow = strokeSectionSidebar.getByLabel("stroke-row-0");

View File

@ -113,7 +113,8 @@
result))
result)))]
[:div {:class (stl/css :element-list) :data-testid "layer-item"}
[:div {:class (stl/css :element-list)
:data-testid "layer-item"}
[:> hooks/sortable-container* {}
(for [obj shapes]
(if (cfh/frame-shape? obj)

View File

@ -195,7 +195,8 @@
(dom/set-attribute! checkbox "indeterminate" true)
(dom/remove-attribute! checkbox "indeterminate"))))
[:div {:class (stl/css :fill-section)}
[:section {:class (stl/css :fill-section)
:aria-label "Fill section"}
[:div {:class (stl/css :fill-title)}
[:> title-bar* {:collapsable has-fills?
:collapsed (not open?)

View File

@ -177,7 +177,7 @@
:shape-ids ids}))))]
[:section {:class (stl/css :stroke-section)
:aria-label "stroke-section"}
:aria-label "Stroke section"}
[:div {:class (stl/css :stroke-title)}
[:> title-bar* {:collapsable has-strokes?
:collapsed (not open?)

View File

@ -315,7 +315,8 @@
expand-stream
#(swap! state* assoc-in [:more-options] true))
[:div {:class (stl/css :element-set)}
[:section {:class (stl/css :element-set)
:aria-label "Text section"}
[:div {:class (stl/css :element-title)}
[:> title-bar* {:collapsable true
:collapsed (not main-menu-open?)

View File

@ -132,7 +132,9 @@
"Maps attrs to styles"
[styles]
(let [mapped-styles
(into {} (map attr->style styles))]
(into {} (comp (filter (fn [[_ v]] (some? v)))
(map attr->style))
styles)]
(clj->js mapped-styles)))
(defn style-needs-mapping?
@ -199,12 +201,14 @@
(let [style-name (get-style-name-as-css-variable k)
[_ style-decode] (get mapping k)
style-value (.getPropertyValue style-declaration style-name)]
(when (or (not removed-mixed) (not (contains? mixed-values style-value)))
(assoc acc k (style-decode style-value))))
(if (or (not removed-mixed) (not (contains? mixed-values style-value)))
(assoc acc k (style-decode style-value))
acc))
(let [style-name (get-style-name k)
style-value (normalize-attr-value k (.getPropertyValue style-declaration style-name))]
(when (or (not removed-mixed) (not (contains? mixed-values style-value)))
(assoc acc k style-value))))) {} txt/text-style-attrs))
(if (or (not removed-mixed) (not (contains? mixed-values style-value)))
(assoc acc k style-value)
acc)))) {} txt/text-style-attrs))
(defn get-styles-from-event
"Returns a ClojureScript object compatible with text nodes"