fix(Button): default htmlType to button to prevent accidental form submission#1141
fix(Button): default htmlType to button to prevent accidental form submission#1141munzzyy wants to merge 3 commits into
Conversation
Button and IconButton both accept an htmlType prop but never defaulted it, so the native button fell back to the browser default of type=submit. Any Button or IconButton inside a form submitted that form on click unless htmlType was set explicitly. Default htmlType to 'button' on both components. Consumers who want submit behavior still opt in with htmlType="submit". Closes ClickHouse#1092
🦋 Changeset detectedLatest commit: 792339d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
SplitButtonhas this prop as well https://github.com/ClickHouse/click-ui/blob/main/src/components/SplitButton/SplitButton.types.ts#L38- 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.
There was a problem hiding this comment.
Good catches, both fixed.
-
SplitButton now defaults
htmlTypeto'button'the same way, with the same three-test pattern (default doesn't submit, explicithtmlType="button"doesn't submit, explicithtmlType="submit"does). -
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.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@clickhouse/click-ui': patch | |||
There was a problem hiding this comment.
Bumped to major.
One thing worth flagging before you merge: this package is at 0.9.0, and .changeset/config.json has no pre-1.0 handling (no linked/fixed overrides, nothing about clamping major bumps). I ran yarn changeset version against it locally and a major changeset here takes the package straight to 1.0.0 - same as plain semver.inc('0.9.0', 'major'). There's no other major-bump changeset anywhere in this repo's history either, so this would be the first one and it makes this PR the trigger for your 1.0.0 release.
If that's fine, it's already done. If you'd rather this stay pre-1.0 and treat "major" as "minor" by convention until you're ready to cut 1.0 on purpose, say the word and I'll set it back to minor - the changeset body already documents the break either way.
Extends the Button/IconButton fix to SplitButton, which has the same submit-by-default gap. Bumped the changeset from patch to minor since defaulting htmlType is a behavior change for any consumer relying on the old default-submit fallback.
|
Good catches, both fixed:
Full suite (505 tests) and typecheck both pass locally. |
Ran the changeset through this repo's own version tool: bumping a
0.9.0 package with a 'major' changeset produces 1.0.0, not a clamped
0.10.0 - there's no pre-1.0 downgrade in .changeset/config.json, so
changesets applies semver.inc('0.9.0', 'major') straight. Went with
major anyway per review since minor undersold the risk, and called
that consequence out in the changeset body so it's not a surprise at
release time.
Rewrote the migration note to name who's affected (a Button,
IconButton, or SplitButton inside a form that relied on the old
implicit type=submit) and the one-line fix (pass htmlType="submit").
2 participants
Why?
A native
<button>without an explicittypedefaults totype="submit".Button,IconButton, andSplitButtonall accept anhtmlTypeprop, but it was never defaulted, so it stayedundefinedand the browser fell back tosubmit. Any of these rendered inside a<form>submits that form on click unless a caller remembers to passhtmlType="button". #1126 added thehtmlTypeescape hatch and fixed a bunch of internal button usages, but left this specific default gap open, and the current test suite documents it directly (Button.test.tsx/IconButton.test.tsxboth had a test literally titled "should not default to any type... thus behaving as type=submit").Closes #1092.
How?
htmlType = 'button'in the destructured props ofButton,IconButton, andSplitButton.Button/IconButton: the old test asserted the buggy submit-by-default behavior, now it asserts the button does not submit by default and thattype="button"is on the element. Added matching coverage toSplitButton.test.tsx, which had no test for the default case at all.htmlType="submit"still submits the form when set explicitly.FileTabs,Select'sInternalSelect,Table) — they already passhtmlType="button"explicitly, so this default doesn't change their behavior.patchtominorper @ariser's review: this changes runtime behavior for any consumer who was relying on the old default-submit fallback, so it's not safe to treat as a patch. Called it out explicitly in the changeset body.Tickets?
Contribution checklist?
buildcommand runs locallySecurity checklist?
dangerouslySetInnerHTMLPreview?
N/A — behavior-only change (default prop value), no visual change.
yarn test(full suite, 505 tests) andyarn typecheckboth pass locally.