diff --git a/common/src/app/common/math.cljc b/common/src/app/common/math.cljc index 043559e257..839079efee 100644 --- a/common/src/app/common/math.cljc +++ b/common/src/app/common/math.cljc @@ -34,12 +34,14 @@ #?(:cljs (js/isNaN v) :clj (Double/isNaN v))) -;; NOTE: on cljs we don't need to check for `number?` so we explicitly -;; ommit it for performance reasons. +;; NOTE: we need `number?` guard on cljs because `js/isFinite` coerces +;; strings to numbers, accepting "16" as finite when it shouldn't. +;; This caused a bug where string values from format-number were +;; propagated through the system until Malli rejected them (issue #10638). (defn finite? [v] - #?(:cljs (and (not (nil? v)) (js/isFinite v)) + #?(:cljs (and (not (nil? v)) (number? v) (js/isFinite v)) :clj (and (not (nil? v)) (number? v) (Double/isFinite v)))) (defn finite diff --git a/common/test/common_tests/math_test.cljc b/common/test/common_tests/math_test.cljc new file mode 100644 index 0000000000..e644101895 --- /dev/null +++ b/common/test/common_tests/math_test.cljc @@ -0,0 +1,59 @@ +;; 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 Sucursal en España SL + +(ns common-tests.math-test + (:require + [app.common.math :as mth] + [clojure.test :as t])) + +(t/deftest finite?-number-test + (t/testing "finite? returns true for positive integer" + (t/is (true? (mth/finite? 16)))) + + (t/testing "finite? returns true for zero" + (t/is (true? (mth/finite? 0)))) + + (t/testing "finite? returns true for negative float" + (t/is (true? (mth/finite? -42.5)))) + + (t/testing "finite? returns true for very large number" + (t/is (true? (mth/finite? 1e308))))) + +(t/deftest finite?-string-test + (t/testing "finite? returns false for numeric string" + (t/is (false? (mth/finite? "16")))) + + (t/testing "finite? returns false for non-numeric string" + (t/is (false? (mth/finite? "abc")))) + + (t/testing "finite? returns false for empty string" + (t/is (false? (mth/finite? "")))) + + (t/testing "finite? returns false for string with spaces" + (t/is (false? (mth/finite? " "))))) + +(t/deftest finite?-nil-test + (t/testing "finite? returns false for nil" + (t/is (false? (mth/finite? nil))))) + +(t/deftest finite?-other-types-test + (t/testing "finite? returns false for keyword" + (t/is (false? (mth/finite? :foo)))) + + (t/testing "finite? returns false for vector" + (t/is (false? (mth/finite? [1 2 3])))) + + (t/testing "finite? returns false for map" + (t/is (false? (mth/finite? {:a 1}))))) + +#_:clj-kondo/ignore +(t/deftest finite?-nan-test + #?(:cljs + (t/testing "finite? returns false for js/NaN (CLJS)" + (t/is (false? (mth/finite? js/NaN)))) + :clj + (t/testing "finite? returns false for Double/NaN (CLJ)" + (t/is (false? (mth/finite? Double/NaN)))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 9451a72931..79cd209747 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -59,6 +59,7 @@ [common-tests.logic.swap-as-override-test] [common-tests.logic.token-test] [common-tests.logic.variants-switch-test] + [common-tests.math-test] [common-tests.media-test] [common-tests.path-names-test] [common-tests.record-test] @@ -133,6 +134,7 @@ 'common-tests.logic.swap-as-override-test 'common-tests.logic.token-test 'common-tests.logic.variants-switch-test + 'common-tests.math-test 'common-tests.media-test 'common-tests.path-names-test 'common-tests.record-test diff --git a/frontend/playwright/data/design/get-file-fragment-10638.json b/frontend/playwright/data/design/get-file-fragment-10638.json new file mode 100644 index 0000000000..f54380fed7 --- /dev/null +++ b/frontend/playwright/data/design/get-file-fragment-10638.json @@ -0,0 +1 @@ +{"~:id":"~u525a5d8b-028e-80e7-8005-aa705347b678","~:file-id":"~u525a5d8b-028e-80e7-8005-aa6cad42f27d","~:created-at":"~m1738332498199","~:data":{"~:options":{},"~:objects":{"~u00000000-0000-0000-0000-000000000000":{"~#shape":{"~:y":0,"~:hide-fill-on-export":false,"~:transform":{"~#matrix":{"~:a":1.0,"~:b":0.0,"~:c":0.0,"~:d":1.0,"~:e":0.0,"~:f":0.0}},"~:rotation":0,"~:name":"Root Frame","~:width":0.01,"~:type":"~:frame","~:points":[{"~#point":{"~:x":0.0,"~:y":0.0}},{"~#point":{"~:x":0.01,"~:y":0.0}},{"~#point":{"~:x":0.01,"~:y":0.01}},{"~#point":{"~:x":0.0,"~:y":0.01}}],"~:r2":0,"~:proportion-lock":false,"~:transform-inverse":{"~#matrix":{"~:a":1.0,"~:b":0.0,"~:c":0.0,"~:d":1.0,"~:e":0.0,"~:f":0.0}},"~:r3":0,"~:r1":0,"~:id":"~u00000000-0000-0000-0000-000000000000","~:parent-id":"~u00000000-0000-0000-0000-000000000000","~:frame-id":"~u00000000-0000-0000-0000-000000000000","~:strokes":[],"~:x":0,"~:proportion":1.0,"~:r4":0,"~:selrect":{"~#rect":{"~:x":0,"~:y":0,"~:width":0.01,"~:height":0.01,"~:x1":0,"~:y1":0,"~:x2":0.01,"~:y2":0.01}},"~:fills":[{"~:fill-color":"#FFFFFF","~:fill-opacity":1}],"~:flip-x":null,"~:height":0.01,"~:flip-y":null,"~:shapes":["~uc42485d8-8a02-80e3-8005-aa6caf76e409","~uc42485d8-8a02-80e3-8005-aa6caf76e40a"]}},"~uc42485d8-8a02-80e3-8005-aa6caf76e409":{"~#shape":{"~:y":0,"~:layout-grid-columns":[{"~:type":"~:flex","~:value":1},{"~:type":"~:flex","~:value":1}],"~:hide-fill-on-export":false,"~:layout-gap-type":"~:multiple","~:layout-padding":{"~:p1":0,"~:p2":0,"~:p3":0,"~:p4":0},"~:transform":{"~#matrix":{"~:a":1.0,"~:b":0.0,"~:c":0.0,"~:d":1.0,"~:e":0.0,"~:f":0.0}},"~:rotation":0,"~:grow-type":"~:fixed","~:layout":"~:grid","~:hide-in-viewer":false,"~:name":"Board","~:layout-align-items":"~:start","~:width":200,"~:layout-grid-cells":{"~uc42485d8-8a02-80e3-8005-aa6cb3b09187":{"~:justify-self":"~:auto","~:column":1,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b09187","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":1,"~:row-span":1,"~:shapes":[]},"~uc42485d8-8a02-80e3-8005-aa6cb3b09188":{"~:justify-self":"~:auto","~:column":2,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b09188","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":1,"~:row-span":1,"~:shapes":[]},"~uc42485d8-8a02-80e3-8005-aa6cb3b09189":{"~:justify-self":"~:auto","~:column":1,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b09189","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":2,"~:row-span":1,"~:shapes":[]},"~uc42485d8-8a02-80e3-8005-aa6cb3b0918a":{"~:justify-self":"~:auto","~:column":2,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b0918a","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":2,"~:row-span":1,"~:shapes":[]}},"~:layout-padding-type":"~:simple","~:type":"~:frame","~:points":[{"~#point":{"~:x":0,"~:y":0}},{"~#point":{"~:x":200,"~:y":0}},{"~#point":{"~:x":200,"~:y":200}},{"~#point":{"~:x":0,"~:y":200}}],"~:r2":0,"~:proportion-lock":false,"~:layout-gap":{"~:row-gap":0,"~:column-gap":0},"~:transform-inverse":{"~#matrix":{"~:a":1.0,"~:b":0.0,"~:c":0.0,"~:d":1.0,"~:e":0.0,"~:f":0.0}},"~:r3":0,"~:layout-justify-content":"~:stretch","~:r1":0,"~:id":"~uc42485d8-8a02-80e3-8005-aa6caf76e409","~:layout-justify-items":"~:start","~:parent-id":"~u00000000-0000-0000-0000-000000000000","~:layout-align-content":"~:stretch","~:frame-id":"~u00000000-0000-0000-0000-000000000000","~:strokes":[],"~:x":0,"~:proportion":1,"~:r4":0,"~:layout-grid-rows":[{"~:type":"~:flex","~:value":1},{"~:type":"~:flex","~:value":1}],"~:selrect":{"~#rect":{"~:x":0,"~:y":0,"~:width":200,"~:height":200,"~:x1":0,"~:y1":0,"~:x2":200,"~:y2":200}},"~:fills":[{"~:fill-color":"#FFFFFF","~:fill-opacity":1}],"~:layout-grid-dir":"~:row","~:flip-x":null,"~:height":200,"~:flip-y":null,"~:shapes":[]}},"~uc42485d8-8a02-80e3-8005-aa6caf76e40a":{"~#shape":{"~:y":0,"~:layout-grid-columns":[{"~:type":"~:flex","~:value":1},{"~:type":"~:flex","~:value":1}],"~:hide-fill-on-export":false,"~:layout-gap-type":"~:multiple","~:layout-padding":{"~:p1":33,"~:p2":33,"~:p3":33,"~:p4":33},"~:transform":{"~#matrix":{"~:a":1.0,"~:b":0.0,"~:c":0.0,"~:d":1.0,"~:e":0.0,"~:f":0.0}},"~:rotation":0,"~:grow-type":"~:fixed","~:layout":"~:grid","~:hide-in-viewer":false,"~:name":"Second","~:layout-align-items":"~:start","~:width":200,"~:layout-grid-cells":{"~uc42485d8-8a02-80e3-8005-aa6cb3b09187":{"~:justify-self":"~:auto","~:column":1,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b09187","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":1,"~:row-span":1,"~:shapes":[]},"~uc42485d8-8a02-80e3-8005-aa6cb3b09188":{"~:justify-self":"~:auto","~:column":2,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b09188","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":1,"~:row-span":1,"~:shapes":[]},"~uc42485d8-8a02-80e3-8005-aa6cb3b09189":{"~:justify-self":"~:auto","~:column":1,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b09189","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":2,"~:row-span":1,"~:shapes":[]},"~uc42485d8-8a02-80e3-8005-aa6cb3b0918a":{"~:justify-self":"~:auto","~:column":2,"~:id":"~uc42485d8-8a02-80e3-8005-aa6cb3b0918a","~:position":"~:auto","~:column-span":1,"~:align-self":"~:auto","~:row":2,"~:row-span":1,"~:shapes":[]}},"~:layout-padding-type":"~:simple","~:type":"~:frame","~:points":[{"~#point":{"~:x":0,"~:y":0}},{"~#point":{"~:x":200,"~:y":0}},{"~#point":{"~:x":200,"~:y":200}},{"~#point":{"~:x":0,"~:y":200}}],"~:r2":0,"~:proportion-lock":false,"~:layout-gap":{"~:row-gap":0,"~:column-gap":0},"~:transform-inverse":{"~#matrix":{"~:a":1.0,"~:b":0.0,"~:c":0.0,"~:d":1.0,"~:e":0.0,"~:f":0.0}},"~:r3":0,"~:layout-justify-content":"~:stretch","~:r1":0,"~:id":"~uc42485d8-8a02-80e3-8005-aa6caf76e40a","~:layout-justify-items":"~:start","~:parent-id":"~u00000000-0000-0000-0000-000000000000","~:layout-align-content":"~:stretch","~:frame-id":"~u00000000-0000-0000-0000-000000000000","~:strokes":[],"~:x":0,"~:proportion":1,"~:r4":0,"~:layout-grid-rows":[{"~:type":"~:flex","~:value":1},{"~:type":"~:flex","~:value":1}],"~:selrect":{"~#rect":{"~:x":0,"~:y":0,"~:width":200,"~:height":200,"~:x1":0,"~:y1":0,"~:x2":200,"~:y2":200}},"~:fills":[{"~:fill-color":"#FFFFFF","~:fill-opacity":1}],"~:layout-grid-dir":"~:row","~:flip-x":null,"~:height":200,"~:flip-y":null,"~:shapes":[]}}},"~:id":"~u525a5d8b-028e-80e7-8005-aa6cad42f27e","~:name":"Page 1"}} diff --git a/frontend/playwright/ui/specs/numeric-input.spec.js b/frontend/playwright/ui/specs/numeric-input.spec.js index 52a4726f91..49d3d1ca6b 100644 --- a/frontend/playwright/ui/specs/numeric-input.spec.js +++ b/frontend/playwright/ui/specs/numeric-input.spec.js @@ -120,7 +120,7 @@ test("BUG 10001: Negative margins are allowed on the numeric input", async ({ await expect(layoutSection).toBeVisible(); await workspacePage.layers.getByTestId("layer-row").nth(6).click(); - await page.waitForTimeout(500); + await page.waitForTimeout(500); const layoutItemSection = workspacePage.rightSidebar.getByRole("region", { name: "Layout item section", @@ -138,3 +138,95 @@ test("BUG 10001: Negative margins are allowed on the numeric input", async ({ await verticalMarginInput.press("Enter"); await expect(verticalMarginInput).toHaveValue("-10"); }); + +test("BUG 10638 - Invalid padding input on multi-selection must not persist strings", async ({ + page, +}) => { + const workspacePage = new WasmWorkspacePage(page); + + await workspacePage.setupEmptyFile(); + await workspacePage.mockRPC(/get\-file\?/, "design/get-file-9543.json"); + await workspacePage.mockRPC( + "get-file-fragment?file-id=*&fragment-id=*", + "design/get-file-fragment-10638.json", + ); + await workspacePage.mockRPC( + "update-file?id=*", + "design/update-file-9543.json", + ); + + // Inspect every persisted change: layout padding and gap values must be + // numbers (the backend rejects strings with a Malli validation error). + // In the transit payload, :set operations carry the attr name and its + // value as siblings: {"~:attr": "~:layout-padding", "~:val": {...}}. + const layoutAttrs = ["~:layout-padding", "~:layout-gap"]; + const seenLayoutValues = []; + const badLayoutValues = []; + const collectLayoutValues = (node) => { + if (!node || typeof node !== "object") { + return; + } + const attr = node["~:attr"]; + const val = node["~:val"]; + if (layoutAttrs.includes(attr) && val && typeof val === "object") { + for (const [prop, leaf] of Object.entries(val)) { + seenLayoutValues.push(`${attr} ${prop}`); + if (typeof leaf !== "number") { + badLayoutValues.push(`${attr} ${prop} = ${JSON.stringify(leaf)}`); + } + } + } + for (const value of Object.values(node)) { + collectLayoutValues(value); + } + }; + page.on("request", (request) => { + if (request.url().includes("/api/main/methods/update-file")) { + const body = request.postData(); + if (body) { + collectLayoutValues(JSON.parse(body)); + } + } + }); + + await workspacePage.goToWorkspace({ + fileId: "525a5d8b-028e-80e7-8005-aa6cad42f27d", + pageId: "525a5d8b-028e-80e7-8005-aa6cad42f27e", + }); + + await workspacePage.clickLeafLayer("Board"); + await workspacePage.clickLeafLayer("Second", { modifiers: ["Shift"] }); + + const toggle = workspacePage.page.getByRole("button", { + name: "Show 4 sided padding options", + }); + await toggle.click(); + + // Paddings differ between the boards (0 vs 33), so the expanded inputs + // have no committed value. Committing invalid text must persist nothing. + const topPaddingInput = workspacePage.page.getByRole("textbox", { + name: "Top padding", + }); + await topPaddingInput.click(); + await topPaddingInput.fill("abc"); + await topPaddingInput.press("Enter"); + + // A valid change afterwards guarantees at least one persisted update. + const leftPaddingInput = workspacePage.page.getByRole("textbox", { + name: "Left padding", + }); + const updateRequest = page.waitForRequest( + "**/api/main/methods/update-file?*", + { + timeout: 15000, + }, + ); + await leftPaddingInput.fill("5"); + await leftPaddingInput.press("Enter"); + await updateRequest; + + // Guard against a vacuous pass: the valid change above must have + // persisted at least one layout-padding value. + expect(seenLayoutValues).not.toEqual([]); + expect(badLayoutValues).toEqual([]); +}); diff --git a/frontend/src/app/main/ui/ds/controls/numeric-input.stories.jsx b/frontend/src/app/main/ui/ds/controls/numeric-input.stories.jsx index 277cde04f3..ffb5684c99 100644 --- a/frontend/src/app/main/ui/ds/controls/numeric-input.stories.jsx +++ b/frontend/src/app/main/ui/ds/controls/numeric-input.stories.jsx @@ -5,6 +5,7 @@ // Copyright (c) KALEIDOS INC Sucursal en España SL import * as React from "react"; import Components from "@target/components"; +import { userEvent, within, expect } from "storybook/test"; const { NumericInput } = Components; const { icons } = Components.meta; @@ -128,3 +129,255 @@ export const WithTokens = { }, render: ({ ...args }) => , }; + +// Regression tests for https://github.com/penpot/penpot/issues/10638 — +// on-change must never receive a string; only numbers, nil, or token ops. + +const invalidInputCalls = []; + +export const TestInvalidInputNeverEmitsString = { + args: { + // Mirrors the expanded padding fields on a mixed multi-selection: + // no value, not nillable (layout_container.cljs multiple-padding-selection*). + nillable: false, + min: 0, + icon: undefined, + property: "padding", + onChange: (value) => invalidInputCalls.push(value), + }, + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + const input = await canvas.getByRole("textbox"); + + await step("Invalid text commits no string values", async () => { + invalidInputCalls.length = 0; + + await userEvent.click(input); + await userEvent.type(input, "abc"); + await userEvent.keyboard("{Enter}"); + + const nonNumeric = invalidInputCalls.filter( + (v) => typeof v !== "number" && v !== null && v !== undefined, + ); + expect(nonNumeric).toEqual([]); + }); + + await step("Whitespace-only input commits nothing", async () => { + invalidInputCalls.length = 0; + + await userEvent.click(input); + await userEvent.clear(input); + await userEvent.type(input, " "); + await userEvent.keyboard("{Enter}"); + + expect(invalidInputCalls).toEqual([]); + }); + }, +}; + +const precisionCalls = []; + +export const TestPrecisionRevertEmitsNothing = { + args: { + // Full-precision committed value (e.g. from a 10/3 expression): the + // display shows the rounded "3.33". Invalid input must revert the + // display without emitting the rounded value — this exact divergence + // leaked the rounded STRING before the fix (issue #10638). + value: 3.3333333333333335, + nillable: false, + min: 0, + icon: undefined, + property: "padding", + onChange: (value) => precisionCalls.push(value), + }, + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + const input = await canvas.getByRole("textbox"); + + await step("Invalid text reverts the display, emits nothing", async () => { + precisionCalls.length = 0; + + await userEvent.click(input); + await userEvent.clear(input); + await userEvent.type(input, "abc"); + await userEvent.keyboard("{Enter}"); + + expect(input).toHaveValue("3.33"); + expect(precisionCalls).toEqual([]); + }); + }, +}; + +const recommitCalls = []; + +export const TestRecommitSameValueEmitsOnce = { + args: { + value: 33, + nillable: false, + min: 0, + icon: undefined, + property: "padding", + onChange: (value) => recommitCalls.push(value), + }, + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + const input = await canvas.getByRole("textbox"); + + await step("Committing the same value twice emits once", async () => { + recommitCalls.length = 0; + + await userEvent.click(input); + await userEvent.clear(input); + await userEvent.type(input, "50"); + await userEvent.keyboard("{Enter}"); + + await userEvent.click(input); + await userEvent.clear(input); + await userEvent.type(input, "50"); + await userEvent.keyboard("{Enter}"); + + expect(recommitCalls).toEqual([50]); + }); + }, +}; + +const nillableClearCalls = []; + +export const TestNillableClearEmitsNil = { + args: { + value: 33, + nillable: true, + icon: undefined, + property: "gap", + onChange: (value) => nillableClearCalls.push(value), + }, + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + const input = await canvas.getByRole("textbox"); + + await step("Clearing a nillable input emits null", async () => { + nillableClearCalls.length = 0; + + await userEvent.click(input); + await userEvent.clear(input); + await userEvent.keyboard("{Enter}"); + + expect(nillableClearCalls).toEqual([null]); + expect(input).toHaveValue(""); + }); + }, +}; + +const unifyCalls = []; + +export const TestTypingShownValueUnifiesMixed = { + args: { + // Same mixed-selection shape as above: the field displays "0" while the + // shapes hold differing values, so committing "0" must still emit. + nillable: false, + min: 0, + icon: undefined, + property: "padding", + onChange: (value) => unifyCalls.push(value), + }, + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + const input = await canvas.getByRole("textbox"); + + await step("Committing the displayed default emits a number", async () => { + unifyCalls.length = 0; + + await userEvent.click(input); + await userEvent.clear(input); + await userEvent.type(input, "0"); + await userEvent.keyboard("{Enter}"); + + expect(unifyCalls).toEqual([0]); + }); + }, +}; + +const escCalls = []; + +export const TestEscRevertsWithoutCommit = { + args: { + value: 33, + nillable: false, + min: 0, + icon: undefined, + property: "padding", + onChange: (value) => escCalls.push(value), + }, + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + const input = await canvas.getByRole("textbox"); + + await step("Escape discards typed text and commits nothing", async () => { + escCalls.length = 0; + + await userEvent.click(input); + await userEvent.clear(input); + await userEvent.type(input, "50"); + await userEvent.keyboard("{Escape}"); + + expect(input).toHaveValue("33"); + expect(escCalls).toEqual([]); + }); + }, +}; + +const tokenCalls = []; + +export const TestTokenApplyAlwaysEmits = { + args: { + ...WithTokens.args, + // Token resolved value equals the current committed value on purpose: + // applying it must still emit the token ops. + value: 30, + icon: undefined, + property: "dimension", + onChange: (value) => tokenCalls.push(value), + }, + parameters: WithTokens.parameters, + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + + await step( + "Applying a token with the same value emits token ops", + async () => { + tokenCalls.length = 0; + + const button = await canvas.getByRole("button"); + await userEvent.click(button); + + const option = await canvas.findByRole("option", { + name: /dimension-1/, + }); + await userEvent.click(option); + + expect(tokenCalls.length).toBe(1); + // Token ops arrive as a CLJS vector (not a JS array): a non-null + // object, never a scalar number/string. + expect(typeof tokenCalls[0]).toBe("object"); + expect(tokenCalls[0]).not.toBeNull(); + }, + ); + + await step( + "Re-applying the same token emits nothing (toggle-token would unapply it)", + async () => { + const pill = await canvas.findByRole("button", { + name: /dimension-1/, + }); + await userEvent.click(pill); + + const option = await canvas.findByRole("option", { + name: /dimension-1/, + }); + await userEvent.click(option); + + expect(tokenCalls.length).toBe(1); + }, + ); + }, +}; diff --git a/frontend/src/app/main/ui/ds/controls/numeric_input.cljs b/frontend/src/app/main/ui/ds/controls/numeric_input.cljs index 03971210e2..ed7d4c4096 100644 --- a/frontend/src/app/main/ui/ds/controls/numeric_input.cljs +++ b/frontend/src/app/main/ui/ds/controls/numeric_input.cljs @@ -44,7 +44,7 @@ [val step min-val max-val] (mth/clamp (- val step) min-val max-val)) -(defn- parse-value +(defn parse-value "Parses and clamps `raw-value` as a number within bounds; returns nil if invalid or empty." [raw-value last-value min-value max-value nillable] @@ -188,10 +188,13 @@ token-has-errors? (-> token-applied :errors seq boolean) is-multiple? (= :multiple value) + ;; NOTE: from here on `value` is the committed value as a number, + ;; or nil when the selection has no single committed value (mixed + ;; selection, or an absent value). value (cond is-multiple? nil (and nillable (nil? value)) nil - :else (d/parse-double value default)) + :else (d/parse-double value (d/parse-double default))) ;; Default props nillable (d/nilv nillable false) @@ -234,6 +237,11 @@ raw-value* (mf/use-ref nil) last-value* (mf/use-ref nil) + ;; Name of the token last applied through this input; dedups + ;; repeated applications of the same token (toggle-token would + ;; otherwise unapply it), while still emitting for a token whose + ;; resolved value equals the current committed value. + last-token-applied* (mf/use-ref nil) ;; Flag to prevent effect from overwriting token during selection ;; This prevents race condition between blur and token selection @@ -292,7 +300,7 @@ apply-value (mf/use-fn - (mf/deps on-change update-input value nillable min max) + (mf/deps on-change update-input value nillable min max default) (fn [raw-value] (let [raw-value (str/trim (str raw-value))] (if-let [parsed (parse-value raw-value (mf/ref-val last-value*) min max nillable)] @@ -315,21 +323,26 @@ (when (fn? on-change) (on-change nil))) - (let [fallback-value (or (mf/ref-val last-value*) default)] - (mf/set-ref-val! raw-value* fallback-value) - (mf/set-ref-val! last-value* fallback-value) + ;; Invalid input: restore the display to the last committed + ;; value (or the default) WITHOUT emitting on-change — the + ;; shapes still hold their previous values, so there is + ;; nothing to commit. Emitting here leaked non-numeric + ;; values into shape data (issue #10638). + (let [fallback-value (or (mf/ref-val last-value*) default) + fallback-text (if (some? fallback-value) + (fmt/format-number fallback-value) + "")] + (mf/set-ref-val! raw-value* fallback-text) (reset! token-applied-name* nil) - (update-input (fmt/format-number fallback-value)) - - (when (and (fn? on-change) (not= fallback-value (str value))) - (on-change fallback-value)))))))) + (update-input fallback-text))))))) apply-token (mf/use-fn (mf/deps min max nillable on-change tokens) (fn [value name] (let [parsed (parse-value value (mf/ref-val last-value*) min max nillable)] - (when-not (= parsed (mf/ref-val last-value*)) + (when-not (= name (mf/ref-val last-token-applied*)) + (mf/set-ref-val! last-token-applied* name) (mf/set-ref-val! last-value* parsed) (when (fn? on-change) (on-change (get-token-op tokens name))))))) @@ -462,8 +475,13 @@ (handle-blur event)) esc? - (do - (update-input (fmt/format-number (mf/ref-val last-value*))) + ;; Discard the typed text entirely: restore the display AND + ;; the pending raw value, and clear the dirty flag so the + ;; blur below does not commit the discarded text. + (let [restore-text (or (fmt/format-number (mf/ref-val last-value*)) "")] + (update-input restore-text) + (mf/set-ref-val! raw-value* restore-text) + (mf/set-ref-val! dirty-ref false) (reset! is-open* false) (dom/blur! node)) @@ -641,6 +659,7 @@ (when-not disabled (dom/prevent-default event) (dom/stop-propagation event) + (mf/set-ref-val! last-token-applied* nil) (reset! token-applied-name* nil) (reset! selected-id* nil) (reset! focused-id* nil) @@ -706,7 +725,7 @@ :placeholder (if is-multiple? (tr "labels.mixed-values") placeholder) - :default-value (or (mf/ref-val last-value*) (fmt/format-number value)) + :default-value (fmt/format-number (or (mf/ref-val last-value*) value)) :on-blur handle-blur :on-key-down on-key-down :on-focus on-focus @@ -739,8 +758,7 @@ (dm/str))) label (or (get token :name) applied-token-name) token-value (or (get token :resolved-value) - (or (mf/ref-val last-value*) - (fmt/format-number value))) + (fmt/format-number (or (mf/ref-val last-value*) value))) token-value (if (and (some? id) (= name :opacity)) (* 100 token-value) token-value)] @@ -783,13 +801,18 @@ "" :else - (fmt/format-number (d/parse-double value default)))] + (fmt/format-number (d/nilv value default)))] (mf/set-ref-val! raw-value* value') - (mf/set-ref-val! last-value* value') + ;; Keep the committed NUMBER (or nil) in last-value*; only the DOM + ;; and raw-value* hold formatted strings. Storing the formatted + ;; string here made the invalid-input fallback leak strings into + ;; on-change (see issue #10638). + (mf/set-ref-val! last-value* value) ;; Only sync token state if not in the middle of a selection ;; This prevents race condition between blur and token selection (when-not (mf/ref-val token-selection-in-progress*) + (mf/set-ref-val! last-token-applied* applied-token-name) (reset! token-applied-name* applied-token-name) (if applied-token-name (let [token-id (:id (get-option-by-name dropdown-options applied-token-name))] diff --git a/frontend/src/app/main/ui/workspace/sidebar/options/menus/layout_container.cljs b/frontend/src/app/main/ui/workspace/sidebar/options/menus/layout_container.cljs index 70050dd38e..c02b3e6b90 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/options/menus/layout_container.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/options/menus/layout_container.cljs @@ -1186,7 +1186,7 @@ (mf/use-fn (mf/deps ids) (fn [multiple? type val] - (let [val (mth/finite val 0)] + (let [val (mth/finite (d/parse-double val 0) 0)] (cond ^boolean multiple? (st/emit! (dwsl/update-layout ids {:layout-gap {:row-gap val :column-gap val}})) @@ -1194,6 +1194,7 @@ (some? type) (st/emit! (dwsl/update-layout ids {:layout-gap {type val}})))))) + ;; Padding on-padding-type-change (mf/use-fn @@ -1205,7 +1206,7 @@ (mf/use-fn (mf/deps ids) (fn [type prop val] - (let [val (mth/finite val 0)] + (let [val (mth/finite (d/parse-double val 0) 0)] (cond (and (= type :simple) (or (= prop :p1) (= prop #{:p1 :p3}))) (st/emit! (dwsl/update-layout ids {:layout-padding {:p1 val :p3 val}})) @@ -1431,7 +1432,7 @@ (mf/use-fn (mf/deps ids) (fn [multiple? type val] - (let [val (mth/finite val 0)] + (let [val (mth/finite (d/parse-double val 0) 0)] (if multiple? (st/emit! (dwsl/update-layout ids {:layout-gap {:row-gap val :column-gap val}})) (st/emit! (dwsl/update-layout ids {:layout-gap {type val}})))))) @@ -1445,7 +1446,7 @@ on-padding-change (fn [type prop val] - (let [val (mth/finite val 0)] + (let [val (mth/finite (d/parse-double val 0) 0)] (cond (and (= type :simple) (= prop :p1)) (st/emit! (dwsl/update-layout ids {:layout-padding {:p1 val :p3 val}})) diff --git a/frontend/test/frontend_tests/ui/ds_controls_numeric_input_test.cljs b/frontend/test/frontend_tests/ui/ds_controls_numeric_input_test.cljs index 5f9f28d7af..a4e8ce7ef2 100644 --- a/frontend/test/frontend_tests/ui/ds_controls_numeric_input_test.cljs +++ b/frontend/test/frontend_tests/ui/ds_controls_numeric_input_test.cljs @@ -6,9 +6,94 @@ (ns frontend-tests.ui.ds-controls-numeric-input-test (:require - [app.main.ui.ds.controls.numeric-input :refer [next-focus-index]] + [app.common.data :as d] + [app.main.ui.ds.controls.numeric-input :refer [next-focus-index parse-value]] + [app.main.ui.formats :as fmt] [cljs.test :as t :include-macros true])) +;; ── format-number / parse-double roundtrip ── +;; These tests guard against the contamination chain that caused issue #10638: +;; format-number returns string → last-value* contaminated → mth/finite? on +;; CLJS accepts strings via js/isFinite → backend Malli rejects → 500. + +(t/deftest test-format-number-returns-string + (t/testing "format-number returns a string, not a number" + (let [result (fmt/format-number 16)] + (t/is (string? result)) + (t/is (not (number? result)))))) + +(t/deftest test-parse-double-roundtrip + (t/testing "parse-double of a formatted number returns a number" + (let [formatted (fmt/format-number 16) + reparsed (d/parse-double formatted)] + (t/is (number? reparsed)) + (t/is (= 16 reparsed)))) + + (t/testing "parse-double of empty string returns nil" + (t/is (nil? (d/parse-double "")))) + + (t/testing "parse-double of nil returns nil" + (t/is (nil? (d/parse-double nil)))) + + (t/testing "parse-double of non-numeric string returns nil" + (t/is (nil? (d/parse-double "abc")))) + + (t/testing "parse-double is idempotent for numbers" + (let [result (d/parse-double (d/parse-double (fmt/format-number 42)))] + (t/is (number? result)) + (t/is (= 42 result))))) + +;; Regression pins for https://github.com/penpot/penpot/issues/10638: +;; parse-value is the only source of committed values, and it must yield +;; numbers or nil — never strings. last-value* holds its numeric output. + +(t/deftest test-parse-value-returns-numbers-or-nil + (t/testing "plain numbers parse" + (t/is (= 33 (parse-value "33" nil nil nil false))) + (t/is (= 33.5 (parse-value "33.5" nil nil nil false)))) + + (t/testing "decimal comma is accepted" + (t/is (= 33.5 (parse-value "33,5" nil nil nil false)))) + + (t/testing "invalid text yields nil, not a fallback string" + (t/is (nil? (parse-value "abc" 33 nil nil false))) + (t/is (nil? (parse-value "abc" 33 nil nil true)))) + + (t/testing "unit suffixes are rejected" + (t/is (nil? (parse-value "33px" 10 nil nil false)))) + + (t/testing "empty input yields nil" + (t/is (nil? (parse-value "" 33 nil nil false))) + (t/is (nil? (parse-value nil nil nil nil true)))) + + (t/testing "expressions evaluate at full precision" + (t/is (= (/ 10 3) (parse-value "10/3" nil nil nil false)))) + + (t/testing "relative expressions use the last committed NUMBER as base" + (t/is (= 25 (parse-value "+5" 20 nil nil false))) + (t/is (= 40 (parse-value "*2" 20 nil nil false))) + (t/is (= 10 (parse-value "50%" 20 nil nil false)))) + + (t/testing "relative expressions with no committed value fall back to 0" + (t/is (= 5 (parse-value "+5" nil nil nil false)))) + + (t/testing "whitespace-only input yields nil" + (t/is (nil? (parse-value " " 33 nil nil false)))) + + (t/testing "division by zero yields nil, not Infinity" + (t/is (nil? (parse-value "1/0" 5 nil nil false)))) + + (t/testing "leading/trailing dot forms parse" + (t/is (= 0.5 (parse-value ".5" nil nil nil false))) + (t/is (= 33 (parse-value "33." nil nil nil false)))) + + (t/testing "huge values clamp to the safe-int range" + (t/is (= 1073741823.5 (parse-value "99999999999999999999" nil nil nil false)))) + + (t/testing "min/max clamping produces numbers" + (t/is (= 0 (parse-value "-5" nil 0 nil false))) + (t/is (= 100 (parse-value "500" nil 0 100 false))))) + (def ^:private sample-options [{:id "a" :type :item :name "Alpha"} {:id "b" :type :group :name "Group"}