[data grid] Fix longText expand button submitting surrounding form#22450
Conversation
Add type="button" to the longText expand and collapse buttons so clicking them does not trigger submission of an enclosing <form>. Fixes mui#22382
Deploy previewhttps://deploy-preview-22450--material-ui-x.netlify.app/ Bundle size
Check out the code infra dashboard for more information about this PR. |
There was a problem hiding this comment.
Pull request overview
Fixes an HTML default behavior bug in the Data Grid longText cell expand/collapse controls: when rendered inside a <form>, the expand/collapse buttons should not implicitly submit the form.
Changes:
- Set
type="button"on thelongTextexpand and collapse buttons to prevent implicit form submission. - Add a regression test ensuring clicking the expand button does not trigger a surrounding form’s
onSubmit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/x-data-grid/src/components/cell/GridLongTextCell.tsx | Adds type="button" to the expand/collapse button elements used by the longText cell UI. |
| packages/x-data-grid/src/tests/cells.DataGrid.test.tsx | Adds a regression test for form submission behavior when clicking the longText expand button. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
LukasTy
left a comment
There was a problem hiding this comment.
Nice fix. 👍
Looks like Claude found more instances of the same bug, WDYT about fixing them?
Code Review: [data grid] Fix longText expand button submitting surrounding form (mui-x#22450)
Summary
Small, correct, well-explained fix. The root cause is accurate: GridLongTextCellCornerButton is styled('button', ...) (packages/x-data-grid/src/components/cell/GridLongTextCell.tsx:72), and per HTML spec a bare <button> inside a <form> defaults to type="submit". event.stopPropagation() in handleExpandClick does not prevent the browser-level submit triggered by the button's default type, so the canonical fix is to set type="button" on both render sites. Both the expand button (line 225) and the in-popper collapse button (line 284) get the attribute, plus a regression test that mounts the grid inside a <form> and asserts onSubmit is not fired.
Correctness
The fix is right on the merits, applied at both call sites, and placed before the {...slotProps?.expandButton} / {...slotProps?.collapseButton} spread so a consumer can still override type explicitly if they really want a submit button via slot props. That ordering matches the rest of the existing prop layout in this file and is consistent between the two sites.
The collapse button fix is also worth keeping (not just defensive): the popper sets container: cellRef.current?.closest('[role="row"]') (line 253), so the popper content is portalled into the row, not to document.body, and therefore stays inside any surrounding <form>. Without type="button" on the collapse button, clicking it would submit too.
Suggestions
-
Test coverage is asymmetric. The new test only exercises the expand button. Given the collapse button stays inside the form (see the container note above), a parallel assertion would close the loop on the second fix. Something like: open the popup, click the collapse button, assert
handleSubmit.callCountis still0. The existing "should close popup when clicking collapse button" test at packages/x-data-grid/src/tests/cells.DataGrid.test.tsx:300 already shows the pattern for reaching the collapse button. Not blocking. -
Out-of-scope but worth filing as a follow-up: the same latent bug exists on other
styled('button', ...)components in the codebase that have no explicittype:packages/x-data-grid-premium/src/components/chartsPanel/chart/GridChartsPanelChart.tsx:54 GridChartTypeButton packages/x-data-grid-premium/src/components/chartsPanel/GridChartsPanel.tsx:57 GridChartsPanelChartSelection packages/x-data-grid-premium/src/components/prompt/GridPrompt.tsx:166 PromptChangesToggle packages/x-data-grid-premium/src/components/collapsible/CollapsibleTrigger.tsx:26 CollapsibleTriggerRootAny of these inside a consumer's
<form>will submit it on click. A separate sweep to addtype="button"to all of them would prevent the same report from recurring.
What looks good
- PR description spells out why
event.stopPropagationis the wrong tool here. Useful context for future readers. - The regression test sits in the right
describeblock, follows the surrounding style (getCell,user.click,button[aria-haspopup="dialog"]selector), and defensivelypreventDefaults in the submit spy. - Diff is two attribute additions plus one test. No drive-by changes, no scope creep.
- Links the originating issue and includes the StackBlitz manual-repro step in the test plan.
Verdict
Approve. Optional: add a symmetric collapse-button assertion to the regression test, and open a follow-up for the other untyped styled('button') declarations in x-data-grid-premium.
…e-button form test
|
@LukasTy I'll separate the fix in another PR for premium, is that okay? |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…-submit # Conflicts: # packages/x-data-grid/src/tests/cells.DataGrid.test.tsx
Summary
longTextcolumn's expand and collapse buttons are rendered from astyled('button', …)element without atypeattribute. Per HTML spec, a<button>inside a<form>defaults totype="submit", so clicking the expand caret submits the parent form.type="button"to both render sites so the buttons stop being implicit submit buttons.event.stopPropagationin the click handler was not enough — it only stops React bubbling, not the browser's native submit triggered by the button's default type.<form>and assertsonSubmitis not fired when the expand button is clicked.Closes #22382
Test plan
pnpm test:unit --project "x-data-grid" --run -t "column type: longText"— all green, including the newshould not submit a surrounding form when clicking the expand buttoncase.pnpm eslint packages/x-data-grid/src/components/cell/GridLongTextCell.tsx packages/x-data-grid/src/tests/cells.DataGrid.test.tsxpnpm --filter "@mui/x-data-grid" run typescript