[material-ui][ButtonGroup] Compute per-button position classes via context by theo-sim-dev · Pull Request #46712 · mui/material-ui

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

+    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

+    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

  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

+      .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

+    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

+  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

+
+  // 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

+    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

+      // 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

  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.

mj12albert · 2025-09-05T14:46:30Z


  const classes = useUtilityClasses(ownerState);

+  // Live set of buttons, in mount/render order


I think this isn't 100% reliable, and it's necessary to sort all the registered Buttons by document position

You could have a look at how floating UI does this with one state: https://github.com/floating-ui/floating-ui/blob/master/packages/react/src/components/FloatingList.tsx#L58-L81

ZeeshanTamboli · 2025-11-07T05:35:38Z

It's decided to not fix this. See #39488 (comment). Closing this PR.

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

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

Closed

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

mj12albert reviewed Sep 5, 2025

View reviewed changes

ZeeshanTamboli closed this Nov 7, 2025

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

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


		const classes = useUtilityClasses(ownerState);

		// Live set of buttons, in mount/render order

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

ZeeshanTamboli commented Nov 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

theo-sim-dev commented Aug 11, 2025 •

edited

Loading

mui-bot commented Aug 11, 2025 •

edited

Loading

ZeeshanTamboli commented Aug 29, 2025 •

edited

Loading