[material-ui][ButtonGroup] Compute per-button position classes via context #46712

theo-sim-dev · 2025-08-11T03:42:20Z

Summary

Restore correct first, middle and last styles in ButtonGroup with custom, wrapped, or conditionally rendered Buttons by using runtime registration via context (no JSX/DOM heuristics, no Emotion :first/last-child dependence).

Changes

ButtonGroup: add register, unregister, getPosition context; ensuring not to leak these internal props to DOM.
Button: register actual root, apply position class from context, merge classes.grouped.

Tests

Position classes: single, multiple, wrapped, and hidden children.
Button integration: grouped class merge, position class, no unknown-prop warnings.

Others

I have followed (at least) the PR section of the contributing guide.

…OM/JSX - Add register/unregister/getPosition to ButtonGroupContext - Button registers its root and applies first/middle/last classes from context - Merge grouped class from context; no :first-child/DOM-type heuristics - Works with wrapped/conditional children and custom Button components - Avoid leaking runtime props to the DOM - Add unit tests for single child, wrapped children, and dynamic visibility - Fixes regression since v5.14.9 where custom Buttons broke grouped styles - Fixes mui#39488

mui-bot · 2025-08-11T03:43:33Z

Netlify deploy preview

https://deploy-preview-46712--material-ui.netlify.app/

Bundle size report

Bundle	Parsed size	Gzip size
@mui/material	🔺+724B^(+0.14%)	🔺+285B^(+0.19%)
@mui/lab	🔺+931B^(+0.69%)	🔺+256B^(+0.62%)
@mui/system	0B^(0.00%)	0B^(0.00%)
@mui/utils	0B^(0.00%)	0B^(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against def448d

ZeeshanTamboli

@theo-sim-dev The implementation looks promising, and I confirmed it works: https://stackblitz.com/edit/prgp5bra.
I’ve left a few initial comments. Let’s also hear what @siriwatknp thinks.

ZeeshanTamboli · 2025-08-18T12:18:00Z

packages/mui-material/src/ButtonGroup/ButtonGroup.js

+    const id = Symbol('button-group-item');
+    orderCounter.current += 1;
+    itemsRef.current.set(id, { node, order: orderCounter.current });
+    setVersion((v) => v + 1);


Let's use full words as variable names. This improves code readability. Same in multiple other places.

ZeeshanTamboli · 2025-08-18T12:18:35Z

packages/mui-material/src/ButtonGroup/ButtonGroup.js

+    return { id };
+  }, []);
+
+  const unregister = React.useCallback((h) => {


Suggested change

const unregister = React.useCallback((h) => {

const unregister = React.useCallback((handler) => {

ZeeshanTamboli · 2025-08-18T12:23:44Z

packages/mui-material/src/ButtonGroup/ButtonGroup.js

@@ -284,6 +282,43 @@ const ButtonGroup = React.forwardRef(function ButtonGroup(inProps, ref) {

  const classes = useUtilityClasses(ownerState);

+  // Live set of buttons, in mount/render order
+  const itemsRef = React.useRef(new Map()); // Map<symbol, { node, order }>


Why itemsRef is not an object, but a Map?

ZeeshanTamboli · 2025-08-18T12:26:31Z

packages/mui-material/src/ButtonGroup/ButtonGroup.js

+      .map(([id, v]) => ({ id, ...v }))
+      .sort((a, b) => a.order - b.order);
+
+    if (arr.length <= 1) {


Shouldn’t this just check for exactly one button?

Suggested change

if (arr.length <= 1) {

if (arr.length === 1) {

In what situation would the group have zero registered buttons? It seems that once a ButtonGroup renders, there’s always at least one child registered; otherwise the group wouldn’t be useful. If 0 ever happened, it would only be during the brief unmount of all children, which isn’t a case that needs position styling anyway.

So arr.length === 1 should be enough.

ZeeshanTamboli · 2025-08-18T12:27:43Z

packages/mui-material/src/Button/Button.js

+    register: groupRegister,
+    unregister: groupUnregister,
+    getPosition: groupGetPosition,
+    version: groupVersion,


Is the version state only meant to force re-renders? Also, I don’t see groupVersion actually being used inside Button.

ZeeshanTamboli · 2025-08-18T12:35:17Z

packages/mui-material/src/Button/Button.js

+  const rootRef = React.useRef(null);
+  const combinedRef = useForkRef(ref, rootRef);
+
+  // Opaque handle returned by ButtonGroupContext.register(...)


What do you mean by Opaque handle? What does it mean?

ZeeshanTamboli · 2025-08-18T12:36:08Z

packages/mui-material/src/Button/Button.js

+
+  // Register on mount, unregister on unmount.
+  // Passive effect avoids parent setState during layout-unmount -> re-entrancy loops in dev.
+  React.useEffect(() => {


I think we should use useEnhancedEffect from utils.

ZeeshanTamboli · 2025-08-18T12:37:57Z

packages/mui-material/src/Button/Button.js

+    if (!groupRegister || !groupUnregister) {
+      // Not inside a ButtonGroup, so no need to register
+      return;
+    } // not inside a ButtonGroup


Above comment is enough.

Suggested change

} // not inside a ButtonGroup

}

ZeeshanTamboli · 2025-08-18T12:47:46Z

packages/mui-material/src/Button/Button.test.js

+      // position class from getPosition() should be applied after re-render
+      expect(btn).to.have.class(buttonGroupClasses.firstButton);
+      expect(btn).not.to.have.class(buttonGroupClasses.middleButton);
+      expect(btn).not.to.have.class(buttonGroupClasses.lastButton);


I’m not clear on this—if there’s only one Button, why does it still get the firstButton class?

ZeeshanTamboli · 2025-08-18T12:49:45Z

packages/mui-material/src/ButtonGroup/ButtonGroupContext.ts

@@ -12,6 +15,15 @@ interface ButtonGroupContextType {
  fullWidth?: boolean;
  size?: ButtonGroupProps['size'];
  variant?: ButtonGroupProps['variant'];
+
+  // Child button calls this once when it mounts
+  register?: (node: HTMLElement | null) => ButtonGroupItemHandle;


Why to support null?

Suggested change

register?: (node: HTMLElement | null) => ButtonGroupItemHandle;

register?: (node: HTMLElement) => ButtonGroupItemHandle;

siriwatknp · 2025-08-25T07:34:24Z

Thank you for submitting the PR but the issue is related to styles. I think we should try to find a solution using CSS first.

Now that we have :has and CSS variables, I think it's highly possible to solve without JavaScript.

ZeeshanTamboli · 2025-08-29T06:57:34Z

@siriwatknp We support Firefox v115, which doesn't support :has selector.

theo-sim-dev mentioned this pull request Aug 11, 2025

[material-ui][ButtonGroup] Renders incorrectly when rendering conditionally from custom component. #39488

Open

2 tasks

zannager added the component: ButtonGroup The React component. label Aug 11, 2025

zannager requested a review from DiegoAndai August 11, 2025 13:30

DiegoAndai requested review from siriwatknp and removed request for DiegoAndai August 13, 2025 19:17

Merge branch 'master' into issue-39488-buttongroup-position-classes

def448d

ZeeshanTamboli reviewed Aug 18, 2025

View reviewed changes

ZeeshanTamboli added scope: button Changes related to the button. type: regression A bug, but worse, it used to behave as expected. labels Aug 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[material-ui][ButtonGroup] Compute per-button position classes via context #46712

[material-ui][ButtonGroup] Compute per-button position classes via context #46712

theo-sim-dev commented Aug 11, 2025 •

edited

Loading

Uh oh!

mui-bot commented Aug 11, 2025 •

edited

Loading

Uh oh!

ZeeshanTamboli left a comment

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

ZeeshanTamboli Aug 18, 2025

Uh oh!

siriwatknp commented Aug 25, 2025

Uh oh!

ZeeshanTamboli commented Aug 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

	const unregister = React.useCallback((h) => {
	const unregister = React.useCallback((handler) => {

	register?: (node: HTMLElement \| null) => ButtonGroupItemHandle;
	register?: (node: HTMLElement) => ButtonGroupItemHandle;

Uh oh!

[material-ui][ButtonGroup] Compute per-button position classes via context #46712

Are you sure you want to change the base?

[material-ui][ButtonGroup] Compute per-button position classes via context #46712

Conversation

theo-sim-dev commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Tests

Others

Uh oh!

mui-bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Bundle size report

Uh oh!

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siriwatknp commented Aug 25, 2025

Uh oh!

ZeeshanTamboli commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

theo-sim-dev commented Aug 11, 2025 •

edited

Loading

mui-bot commented Aug 11, 2025 •

edited

Loading

ZeeshanTamboli commented Aug 29, 2025 •

edited

Loading