mirror of
https://github.com/penpot/penpot.git
synced 2026-07-31 18:36:18 +00:00
🐛 Fix layout padding persisted as string after invalid input (#10758)
Combined fixes from PR #10656 (numeric-input redesign) and PR #10696 (global finite? guard) for issue #10638. - Add (number? v) guard to cljs mth/finite? so strings are rejected - Redesign numeric-input last-value* to store number, not formatted string - Invalid-input fallback restores display without emitting on-change - Esc now fully discards typed text (resets raw-value* + dirty flag) - Token dedup by name instead of resolved value - Defense-in-depth: d/parse-double at 4 padding/gap handlers - Math tests (common), unit tests, Storybook play tests, Playwright E2E AI-assisted-by: deepseek-v4-flash Co-authored-by: Akshit Nassa <nassaakshit@gmail.com> Co-authored-by: Ulises Millán <ulises.millanguerrero@gmail.com>
This commit is contained in:
Andrey Antukh
GitHub
parent
3eb50ef50e
commit
972353eccb
@ -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
|
||||
|
||||
59
common/test/common_tests/math_test.cljc
Normal file
59
common/test/common_tests/math_test.cljc
Normal file
@ -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))))))
|
||||
@ -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
|
||||
|
||||
File diff suppressed because one or more lines are too long
@ -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([]);
|
||||
});
|
||||
|
||||
@ -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 }) => <NumericInput {...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);
|
||||
},
|
||||
);
|
||||
},
|
||||
};
|
||||
|
||||
@ -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))]
|
||||
|
||||
@ -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}}))
|
||||
|
||||
@ -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"}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user