Skip to content

[mergeProps] Fix multi-argument event handler forwarding#4598

Merged
atomiks merged 2 commits intomui:masterfrom
atomiks:codex/merge-props-forward-all-handler-args
Apr 20, 2026
Merged

[mergeProps] Fix multi-argument event handler forwarding#4598
atomiks merged 2 commits intomui:masterfrom
atomiks:codex/merge-props-forward-all-handler-args

Conversation

@atomiks
Copy link
Copy Markdown
Contributor

@atomiks atomiks commented Apr 13, 2026

Fixes #4597

mergeProps only forwarded the first argument when merged handlers were called, so cases like mergeProps({ onOpenChange }, { onOpenChange }) were already dropping secondary arguments. PR #4330 added preventBaseUIHandler() runtime wrapping for lone and first-position handlers, which widened the same behavior to cases that had previously worked, such as mergeProps({}, { onOpenChange }).

This change forwards all handler arguments in both the merged-handler path and the runtime wrapper path, while still only treating the first argument as the synthetic event for preventBaseUIHandler().

Changes

  • Forward all arguments from mergeEventHandlers().
  • Forward all arguments from wrapEventHandler().
  • Keep preventBaseUIHandler() behavior tied to the first argument.
  • Add regression tests for merged callbacks, lone wrapped callbacks, and synthetic handlers with extra arguments.

@atomiks atomiks added type: bug It doesn't behave as expected. type: regression A bug, but worse, it used to behave as expected. scope: public utils labels Apr 13, 2026 — with ChatGPT Codex Connector
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 13, 2026

commit: ba85b95

@code-infra-dashboard
Copy link
Copy Markdown

code-infra-dashboard bot commented Apr 13, 2026

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+54B(+0.01%) 🔺+13B(+0.01%)

Details of bundle changes

Performance

Total duration: 1,416.08 ms 🔺+59.39 ms(+4.4%) | Renders: 53 (+0) | Paint: 2,172.02 ms +45.22 ms(+2.1%)

Test Duration Renders
Select open (500 options) 57.76 ms 🔺+10.55 ms(+22.3%) 14 (+0)
Select mount (200 instances) 165.95 ms 🔺+27.17 ms(+19.6%) 3 (+0)
Menu open (500 items) 81.86 ms 🔺+10.99 ms(+15.5%) 12 (+0)
Scroll Area mount (300 instances) 98.82 ms 🔺+12.71 ms(+14.8%) 3 (+0)
Menu mount (300 instances) 153.46 ms 🔺+11.66 ms(+8.2%) 2 (+0)

...and 7 more. View full report

Details of benchmark changes


Check out the code infra dashboard for more information about this PR.

@netlify
Copy link
Copy Markdown

netlify bot commented Apr 13, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit ba85b95
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/69e5e317395b730008bbb795
😎 Deploy Preview https://deploy-preview-4598--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@atomiks atomiks marked this pull request as ready for review April 13, 2026 15:02
@cgatian
Copy link
Copy Markdown
Contributor

cgatian commented Apr 13, 2026

@atomiks verified locally 🚀 . Thank you!

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented Apr 13, 2026

@cgatian surprisingly, it was already broken before (in a different way), as mergeProps({onOpenChange}, {onOpenChange}) already had this issue, while mergeProps({}, {onOpenChange}) worked

I'm not sure what the blast radius of the regression is like in terms of how commonly this was used, or if your case was somewhat unique

@cgatian
Copy link
Copy Markdown
Contributor

cgatian commented Apr 14, 2026

@atomiks Hmm we were using it with what would seem like a pretty common use case

export const ConfirmDialogRoot: React.FC<ConfirmDialogRoot.Props> = (rest) => {
  const handleOnOpenChange: AlertDialog.Root.Props['onOpenChange'] = (_open, eventDetails) => {
     // code omitted for brevity
  };

  return (
      <AlertDialog.Root {...mergeProps(rest, { onOpenChange: handleOnOpenChange })} />
  );
};

@oliviertassinari oliviertassinari removed the type: bug It doesn't behave as expected. label Apr 18, 2026
@atomiks atomiks mentioned this pull request Apr 20, 2026
@atomiks atomiks merged commit 783e530 into mui:master Apr 20, 2026
24 checks passed
@atomiks atomiks deleted the codex/merge-props-forward-all-handler-args branch April 20, 2026 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: public utils type: regression A bug, but worse, it used to behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[mergeProps] secondary arguments are undefined

4 participants