Skip to content

fix(Button): default htmlType to button to prevent accidental form submission #1141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
munzzyy wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(Button): default htmlType to button to prevent accidental form submission #1141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
845e11d
fix(Button): default htmlType to button to stop accidental form submits
munzzyy Jul 5, 2026
8fade1c
fix(SplitButton): default htmlType to button; mark release as minor
munzzyy Jul 6, 2026
792339d
fix(changeset): bump htmlType default change to major
munzzyy Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/fix-button-htmltype-default.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@clickhouse/click-ui': major
---

Default `htmlType` to `"button"` on `Button`, `IconButton`, and `SplitButton`. All three already accepted an `htmlType` prop, but left it undefined by default, so the native `<button>` fell back to the browser's own default of `type="submit"`. Any of these rendered inside a `<form>` without an explicit `htmlType` would submit that form on click.

**Breaking change.** This affects any consumer with a `Button`, `IconButton`, or `SplitButton` inside a `<form>` that relied on the old default-submit behavior — i.e. you never passed `htmlType` and expected a click to submit the form anyway. To keep that behavior, pass `htmlType="submit"` explicitly on the affected button.
17 changes: 16 additions & 1 deletion src/components/Button/Button.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ describe('Button', () => {
});

describe('Button HTML types', () => {
it('should not default to any type when consumer does not specify it, thus behaving as type=submit', () => {
it('should default to type=button so it does not submit an enclosing form', () => {
const handleSubmit = vi.fn();

const { getByRole } = renderCUI(
Expand All @@ -132,8 +132,23 @@ describe('Button', () => {

const button = getByRole('button');

expect(button).toHaveAttribute('type', 'button');
fireEvent.click(button);
expect(handleSubmit).not.toHaveBeenCalled();
});

it('should submit an enclosing form when htmlType="submit" is set explicitly', () => {
const handleSubmit = vi.fn(e => e.preventDefault());

const { getByRole } = renderCUI(
<form onSubmit={handleSubmit}>
<Button htmlType="submit" />
</form>
);

const button = getByRole('button');
fireEvent.click(button);

expect(handleSubmit).toHaveBeenCalled();
});

Expand Down
2 changes: 1 addition & 1 deletion src/components/Button/Button.tsx
View file
Open in desktop

@ariser ariser Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. SplitButton has this prop as well https://github.com/ClickHouse/click-ui/blob/main/src/components/SplitButton/SplitButton.types.ts#L38
  2. This is a breaking change, we should not just merge it without a migration plan for consumers. We can't assume all use cases that expect default submit behavior are covered by tests. This can break silently.

@munzzyy munzzyy Jul 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catches, both fixed.

  1. SplitButton now defaults htmlType to 'button' the same way, with the same three-test pattern (default doesn't submit, explicit htmlType="button" doesn't submit, explicit htmlType="submit" does).

  2. Agreed on the breaking-change risk. Changeset is bumped to major now (see my note on that thread) with a migration note naming who's affected and the one-line fix.

I looked for a deprecation-warning precedent here before considering it as an alternative to a straight breaking change, but I don't think it fits this case. The existing @deprecated tags in this repo are all for props/types that keep working until a future removal - a runtime warning here would need to detect "this button has no explicit htmlType and is inside a form," which is a new kind of check, not something this codebase does anywhere. Happy to build that out if you'd rather ship it that way, just didn't want to assume it was wanted without asking.

Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
(
{
type = 'primary',
htmlType,
htmlType = 'button',
iconLeft,
iconRight,
align = 'center',
Expand Down
17 changes: 16 additions & 1 deletion src/components/IconButton/IconButton.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('Button', () => {
});

describe('Button HTML types', () => {
it('should not default to any type when consumer does not specify it, thus behaving as type=submit', () => {
it('should default to type=button so it does not submit an enclosing form', () => {
const handleSubmit = vi.fn();

const { getByRole } = renderCUI(
Expand All @@ -51,8 +51,23 @@ describe('Button', () => {

const button = getByRole('button');

expect(button).toHaveAttribute('type', 'button');
fireEvent.click(button);
expect(handleSubmit).not.toHaveBeenCalled();
});

it('should submit an enclosing form when htmlType="submit" is set explicitly', () => {
const handleSubmit = vi.fn(e => e.preventDefault());

const { getByRole } = renderCUI(
<form onSubmit={handleSubmit}>
<IconButton icon="user" htmlType="submit" />
</form>
);

const button = getByRole('button');
fireEvent.click(button);

expect(handleSubmit).toHaveBeenCalled();
});

Expand Down
2 changes: 1 addition & 1 deletion src/components/IconButton/IconButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const iconButtonVariants = cva(styles.iconbutton, {
});

export const IconButton = forwardRef<HTMLButtonElement, IconButtonProps>(
({ type = 'primary', htmlType, icon, size, disabled, className, ...props }, ref) => {
({ type = 'primary', htmlType = 'button', icon, size, disabled, className, ...props }, ref) => {
const iconName = icon ? icon.toString() : 'unknown icon';

return (
Expand Down
36 changes: 36 additions & 0 deletions src/components/SplitButton/SplitButton.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,24 @@ describe('SplitButton', () => {
}
);

it('defaults to type=button so it does not submit an enclosing form', () => {
const handleSubmit = vi.fn(e => e.preventDefault());
const { getByText } = renderCUI(
<form onSubmit={handleSubmit}>
<SplitButton menu={menuItems}>
<div>SplitButton Main Trigger</div>
</SplitButton>
</form>
);

const button = getByText('SplitButton Main Trigger').closest('button');
expect(button).toHaveAttribute('type', 'button');

fireEvent.click(getByText('SplitButton Main Trigger'));

expect(handleSubmit).not.toHaveBeenCalled();
});

it('does not submit an enclosing form when htmlType="button"', () => {
const handleSubmit = vi.fn(e => e.preventDefault());
const { getByText } = renderCUI(
Expand All @@ -218,5 +236,23 @@ describe('SplitButton', () => {

expect(handleSubmit).not.toHaveBeenCalled();
});

it('submits an enclosing form when htmlType="submit" is set explicitly', () => {
const handleSubmit = vi.fn(e => e.preventDefault());
const { getByText } = renderCUI(
<form onSubmit={handleSubmit}>
<SplitButton
menu={menuItems}
htmlType="submit"
>
<div>SplitButton Main Trigger</div>
</SplitButton>
</form>
);

fireEvent.click(getByText('SplitButton Main Trigger'));

expect(handleSubmit).toHaveBeenCalled();
});
});
});
2 changes: 1 addition & 1 deletion src/components/SplitButton/SplitButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { SplitButtonProps, Menu, ButtonType } from './SplitButton.types';

export const SplitButton = ({
type = 'primary',
htmlType,
htmlType = 'button',
disabled,
menu,
dir,
Expand Down