From 4a844ca5b1e1300f9a74828e5c14bbdeaf31e496 Mon Sep 17 00:00:00 2001 From: Pranav Tomar Date: Sun, 29 Mar 2026 22:37:58 +0530 Subject: [PATCH 1/2] fix(form): route Ctrl+Enter through onSubmit and stop double-firing on_change Three related bugs in mo.ui.form are fixed: 1. Ctrl+Enter / Cmd+Enter now routes through the
's onSubmit handler via requestSubmit() instead of calling setValue() directly. This ensures that shouldValidate and clearOnSubmit behave consistently regardless of whether the user clicks the button or uses the keyboard shortcut. 2. Shift+Enter inside an OnBlurredInput no longer bypasses the form wrapper. A shiftKey guard is added to the onKeyDown handler so that Shift+Enter cannot inadvertently commit the input value and skip the form buffer. 3. The wrapped element's on_change callback no longer fires during form submission. form._convert_value() previously called element._update(), which always triggered on_change. A new _update_value() method on UIElement updates the stored value without invoking the callback; form._convert_value() now calls that instead, so on_change fires exactly once (on the form itself) per submission. Fixes #8324 --- frontend/src/components/ui/input.tsx | 5 +- frontend/src/plugins/impl/FormPlugin.tsx | 6 +- .../impl/__tests__/FormPlugin.test.tsx | 188 ++++++++++++++++++ marimo/_plugins/ui/_core/ui_element.py | 14 +- marimo/_plugins/ui/_impl/input.py | 5 +- tests/_plugins/ui/_impl/test_input.py | 54 +++++ 6 files changed, 265 insertions(+), 7 deletions(-) create mode 100644 frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx diff --git a/frontend/src/components/ui/input.tsx b/frontend/src/components/ui/input.tsx index d98a01e8f32..264d17bac99 100644 --- a/frontend/src/components/ui/input.tsx +++ b/frontend/src/components/ui/input.tsx @@ -195,7 +195,10 @@ export const OnBlurredInput = React.forwardRef< onChange={(event) => setInternalValue(event.target.value)} onBlur={() => setValue(internalValue || "")} onKeyDown={(event) => { - if (event.key !== "Enter") { + // Only commit on plain Enter. Shift+Enter should not submit the + // enclosing form — let the event bubble naturally (e.g. newline in a + // textarea, or no-op in a single-line input). + if (event.key !== "Enter" || event.shiftKey) { return; } setValue(internalValue || ""); diff --git a/frontend/src/plugins/impl/FormPlugin.tsx b/frontend/src/plugins/impl/FormPlugin.tsx index fe242f3716c..540585ac04f 100644 --- a/frontend/src/plugins/impl/FormPlugin.tsx +++ b/frontend/src/plugins/impl/FormPlugin.tsx @@ -149,11 +149,13 @@ export const FormWrapper = ({ "bg-(--amber-1) border-(--amber-7)": !synchronized && bordered, })} onKeyDown={(evt) => { - // Handle enter + ctrl/meta key + // Handle enter + ctrl/meta key — route through the form's onSubmit + // so that validation and clearOnSubmit are applied consistently, + // just as when the submit button is clicked. if (evt.key === "Enter" && (evt.ctrlKey || evt.metaKey)) { evt.preventDefault(); evt.stopPropagation(); - setValue(newValue); + formDiv.current?.requestSubmit(); } }} > diff --git a/frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx b/frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx new file mode 100644 index 00000000000..bdd8544a9cc --- /dev/null +++ b/frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx @@ -0,0 +1,188 @@ +/* Copyright 2026 Marimo. All rights reserved. */ + +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { beforeAll, describe, expect, it, vi } from "vitest"; +import { SetupMocks } from "@/__mocks__/common"; +import { type FormWrapperProps, FormWrapper } from "../FormPlugin"; + +beforeAll(() => { + SetupMocks.resizeObserver(); +}); + +function renderForm( + overrides: Partial> = {}, +) { + const setValue = vi.fn(); + const validate = vi.fn().mockResolvedValue(null); + + render( + + label={null} + bordered={false} + loading={false} + submitButtonLabel="Submit" + submitButtonDisabled={false} + clearOnSubmit={false} + showClearButton={false} + clearButtonLabel="Clear" + currentValue="old" + newValue="new" + setValue={setValue} + validate={validate} + shouldValidate={false} + {...overrides} + > + + , + ); + + return { setValue, validate }; +} + +describe("FormWrapper — submit button", () => { + it("calls setValue when the submit button is clicked", async () => { + const { setValue } = renderForm(); + fireEvent.click(screen.getByTestId("marimo-plugin-form-submit-button")); + await waitFor(() => expect(setValue).toHaveBeenCalledWith("new")); + }); + + it("does NOT call setValue when shouldValidate=true and validate returns an error", async () => { + const validate = vi.fn().mockResolvedValue("Invalid input"); + const { setValue } = renderForm({ shouldValidate: true, validate }); + fireEvent.click(screen.getByTestId("marimo-plugin-form-submit-button")); + await waitFor(() => + expect(screen.getByText("Invalid input")).toBeInTheDocument(), + ); + expect(setValue).not.toHaveBeenCalled(); + }); + + it("calls setValue when shouldValidate=true and validate returns null", async () => { + const validate = vi.fn().mockResolvedValue(null); + const { setValue } = renderForm({ shouldValidate: true, validate }); + fireEvent.click(screen.getByTestId("marimo-plugin-form-submit-button")); + await waitFor(() => expect(setValue).toHaveBeenCalledWith("new")); + }); +}); + +describe("FormWrapper — Ctrl+Enter / Cmd+Enter", () => { + it("submits (calls setValue) on Ctrl+Enter", async () => { + const { setValue } = renderForm(); + const form = screen + .getByTestId("marimo-plugin-form-submit-button") + .closest("form")!; + // requestSubmit is not implemented in jsdom — spy on it and trigger submit + const requestSubmit = vi + .spyOn(form, "requestSubmit") + .mockImplementation(() => { + fireEvent.submit(form); + }); + + fireEvent.keyDown(form.firstElementChild!, { + key: "Enter", + ctrlKey: true, + }); + + expect(requestSubmit).toHaveBeenCalled(); + await waitFor(() => expect(setValue).toHaveBeenCalledWith("new")); + }); + + it("submits (calls setValue) on Cmd+Enter (metaKey)", async () => { + const { setValue } = renderForm(); + const form = screen + .getByTestId("marimo-plugin-form-submit-button") + .closest("form")!; + vi.spyOn(form, "requestSubmit").mockImplementation(() => { + fireEvent.submit(form); + }); + + fireEvent.keyDown(form.firstElementChild!, { + key: "Enter", + metaKey: true, + }); + + await waitFor(() => expect(setValue).toHaveBeenCalledWith("new")); + }); + + it("runs validation on Ctrl+Enter when shouldValidate=true", async () => { + const validate = vi.fn().mockResolvedValue("Bad value"); + const { setValue } = renderForm({ shouldValidate: true, validate }); + const form = screen + .getByTestId("marimo-plugin-form-submit-button") + .closest("form")!; + vi.spyOn(form, "requestSubmit").mockImplementation(() => { + fireEvent.submit(form); + }); + + fireEvent.keyDown(form.firstElementChild!, { + key: "Enter", + ctrlKey: true, + }); + + await waitFor(() => + expect(screen.getByText("Bad value")).toBeInTheDocument(), + ); + expect(setValue).not.toHaveBeenCalled(); + }); + + it("applies clearOnSubmit on Ctrl+Enter", async () => { + const { setValue } = renderForm({ clearOnSubmit: true }); + const form = screen + .getByTestId("marimo-plugin-form-submit-button") + .closest("form")!; + vi.spyOn(form, "requestSubmit").mockImplementation(() => { + fireEvent.submit(form); + }); + + fireEvent.keyDown(form.firstElementChild!, { + key: "Enter", + ctrlKey: true, + }); + + await waitFor(() => expect(setValue).toHaveBeenCalledWith("new")); + }); + + it("does NOT submit on plain Enter (no modifier)", () => { + const { setValue } = renderForm(); + const form = screen + .getByTestId("marimo-plugin-form-submit-button") + .closest("form")!; + const requestSubmit = vi.spyOn(form, "requestSubmit"); + + fireEvent.keyDown(form.firstElementChild!, { key: "Enter" }); + + expect(requestSubmit).not.toHaveBeenCalled(); + expect(setValue).not.toHaveBeenCalled(); + }); +}); + +describe("FormWrapper — clear button", () => { + it("renders a clear button when showClearButton=true", () => { + renderForm({ showClearButton: true }); + expect( + screen.getByTestId("marimo-plugin-form-clear-button"), + ).toBeInTheDocument(); + }); + + it("does NOT render a clear button when showClearButton=false", () => { + renderForm({ showClearButton: false }); + expect( + screen.queryByTestId("marimo-plugin-form-clear-button"), + ).not.toBeInTheDocument(); + }); +}); + +describe("FormWrapper — loading state", () => { + it("disables the submit button when loading=true", () => { + renderForm({ loading: true }); + expect( + screen.getByTestId("marimo-plugin-form-submit-button"), + ).toBeDisabled(); + }); + + it("disables the submit button when submitButtonDisabled=true", () => { + renderForm({ submitButtonDisabled: true }); + expect( + screen.getByTestId("marimo-plugin-form-submit-button"), + ).toBeDisabled(); + }); +}); diff --git a/marimo/_plugins/ui/_core/ui_element.py b/marimo/_plugins/ui/_core/ui_element.py index 13d73236b5d..af070b4e8f3 100644 --- a/marimo/_plugins/ui/_core/ui_element.py +++ b/marimo/_plugins/ui/_core/ui_element.py @@ -452,15 +452,23 @@ def _update(self, value: S) -> None: Calls the on_change handler with the element's new value as a side-effect. """ + self._update_value(value) + if self._on_change is not None: + self._on_change(self._value) + + def _update_value(self, value: S) -> None: + """Update value without triggering the on_change callback. + + Used internally when a parent element (e.g. form) needs to hydrate + this element's value as part of its own conversion, without + double-firing the element's on_change handler. + """ self._value_frontend = value try: self._value = self._convert_value(value) except MarimoConvertValueException: raise - if self._on_change is not None: - self._on_change(self._value) - def _on_update_completion(self) -> bool: """Callback to run after the kernel has processed a value update. diff --git a/marimo/_plugins/ui/_impl/input.py b/marimo/_plugins/ui/_impl/input.py index 1301214f5ad..e1dca97d668 100644 --- a/marimo/_plugins/ui/_impl/input.py +++ b/marimo/_plugins/ui/_impl/input.py @@ -1578,7 +1578,10 @@ def _validate(self, value: ValueArgs) -> Optional[str]: def _convert_value(self, value: Optional[JSONTypeBound]) -> Optional[T]: if value is None: return None - self.element._update(value) + # Use _update_value (not _update) to hydrate the wrapped element's + # value without triggering its on_change handler — the form's own + # on_change is what should fire, not the wrapped element's. + self.element._update_value(value) return self.element.value diff --git a/tests/_plugins/ui/_impl/test_input.py b/tests/_plugins/ui/_impl/test_input.py index 6caf8f47f43..9d628f78792 100644 --- a/tests/_plugins/ui/_impl/test_input.py +++ b/tests/_plugins/ui/_impl/test_input.py @@ -677,3 +677,57 @@ def test_power_scale() -> None: assert range_slider.start == 1 assert range_slider.stop == 9 assert range_slider.step is None + + +# --------------------------------------------------------------------------- +# form — on_change double-fire regression tests (issue #8324) +# --------------------------------------------------------------------------- + + +def test_form_convert_value_does_not_fire_element_on_change() -> None: + """Submitting a form must NOT trigger the wrapped element's on_change. + + Before the fix, form._convert_value called element._update(), which + always fired the element's on_change callback. After the fix it calls + element._update_value(), which skips that callback. + """ + element_changes: list[Any] = [] + text = ui.text(on_change=lambda v: element_changes.append(v)) + f = text.form() + + # Simulate a form submission: the form receives the wrapped element's + # serialised value from the frontend. + f._update("hello") + + # The form's value should be updated … + assert f._value == "hello" + # … but the wrapped element's on_change must NOT have been called. + assert element_changes == [], ( + "Wrapped element's on_change fired during form submission — " + "this is the double-fire bug described in issue #8324." + ) + + +def test_form_on_change_fires_once_on_submit() -> None: + """The form's own on_change must fire exactly once per submission.""" + form_changes: list[Any] = [] + text = ui.text() + f = text.form(on_change=lambda v: form_changes.append(v)) + + f._update("world") + + assert form_changes == ["world"] + + +def test_form_update_value_does_not_trigger_on_change() -> None: + """_update_value is the silent counterpart to _update.""" + changes: list[Any] = [] + text = ui.text(on_change=lambda v: changes.append(v)) + + text._update_value("silent") + assert text._value == "silent" + assert changes == [], "_update_value must not trigger on_change" + + # Contrast: _update *should* trigger on_change. + text._update("loud") + assert changes == ["loud"] From 45e5ac384b225960176dcf3768ede8691662d5a1 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 29 Mar 2026 17:09:55 +0000 Subject: [PATCH 2/2] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx b/frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx index bdd8544a9cc..fa15cb7eb3c 100644 --- a/frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx +++ b/frontend/src/plugins/impl/__tests__/FormPlugin.test.tsx @@ -3,15 +3,13 @@ import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import { beforeAll, describe, expect, it, vi } from "vitest"; import { SetupMocks } from "@/__mocks__/common"; -import { type FormWrapperProps, FormWrapper } from "../FormPlugin"; +import { FormWrapper, type FormWrapperProps } from "../FormPlugin"; beforeAll(() => { SetupMocks.resizeObserver(); }); -function renderForm( - overrides: Partial> = {}, -) { +function renderForm(overrides: Partial> = {}) { const setValue = vi.fn(); const validate = vi.fn().mockResolvedValue(null);