[ButtonGroup][material-ui] Renders incorrectly with conditional elements in 5.14.9

[Thomas101]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://stackblitz.com/edit/stackblitz-starters-nykpki

Steps:

Starting with version 5.14.9, the rounded corners fail to render when the last element is undefined, for example...

<ButtonGroup variant="contained" aria-label="outlined primary button group">
    <Button>One</Button>
    <Button>Two</Button>
    {false
      ? <Button>Three</Button>
      : undefined/* returning undefined here presents the issue */}
</ButtonGroup>

Current behavior 😯

The rounded corners on the last element are missing...

Expected behavior 🤔

Rounded corners should be visible on the last element. This works correctly in 5.14.8 and earlier

Context 🔦

Conditionally rendering elements in the ButtonGroup. Without this they need to be rendered into an array and filtered before being passed as children

Your environment 🌎

npx @mui/envinfo

System:
    OS: macOS 13.4.1
  Binaries:
    Node: 19.4.0 - ~/.nvm/versions/node/v19.4.0/bin/node
    Yarn: Not Found
    npm: 9.2.0 - ~/.nvm/versions/node/v19.4.0/bin/npm
  Browsers:
    Chrome: 116.0.5845.187
    Edge: 114.0.1823.67
    Safari: 16.5.2
  npmPackages:
    @emotion/react: 11.11.1 => 11.11.1
    @emotion/styled: 11.11.0 => 11.11.0
    @mui/base: 5.0.0-beta.15 => 5.0.0-beta.15
    @mui/core-downloads-tracker:  5.14.9
    @mui/icons-material: 5.14.9 => 5.14.9
    @mui/material: 5.14.9 => 5.14.9
    @mui/private-theming:  5.14.9
    @mui/styled-engine:  5.14.9
    @mui/system:  5.14.9
    @mui/types:  7.2.4
    @mui/utils:  5.14.9
    @types/react: 18.2.21 => 18.2.21
    react: 18.2.0 => 18.2.0
    react-dom: 18.2.0 => 18.2.0
    typescript: 5.2.2 => 5.2.2

[Thomas101]

Possibly this commit 95e41fd but not confirmed

[ZeeshanTamboli]

@Thomas101 Thanks for reporting. It's a regression from #38520.

We used React.Children.count to count the number of React children but it also counts empty nodes resulting in incorrect styling. I have created PR #38989 to fix it.

[Thomas101]

Thanks! I appreciate the fast response 👍

[DevRockyFish]

@Thomas101 Thanks for reporting. It's a regression from #38520.

We used React.Children.count to count the number of React children but it also counts empty nodes resulting in incorrect styling. I have created PR #38989 to fix it.

There is still a problem even with that PR. If things are wrapped in a fragment, it always renders it incorrectly. Previous to 5.14.9, this was not the case.

This is a problem for us because we have some items inside the button group that are memoized.

This can be seen in the above example by wrapping everything in the button group in a fragment.

[ZeeshanTamboli]

@DevRockyFish Why do you want to wrap elements inside the ButtonGroup in a React Fragment? Isn't that unnecessary? Do you mean like this? The React.Fragment is not needed there.

[DevRockyFish]

@ZeeshanTamboli It is not needed there, but we have some memoized toolbar buttons that are rendered conditionally, and they all get returned out of the memo as children. So since they are children we return them out using a fragment, so some of the buttons inside our button group are in the button group like in the example, and some are returned out of a useMemo and put into the button group as additional.

Alternatively we could create an array of JSX and render it that way.

[ZeeshanTamboli]

@DevRockyFish Yes, creating an array of JSX allows for dynamic button rendering without the need for Fragments, which was incorrectly used within the ButtonGroup earlier.

[DevRockyFish]

@DevRockyFish Yes, creating an array of JSX allows for dynamic button rendering without the need for Fragments, which was incorrectly used within the ButtonGroup earlier.

So fragments won't work inside of button groups anymore is the plan? Just confirming.

[ZeeshanTamboli]

So fragments won't work inside of button groups anymore is the plan? Just confirming.

Yes.

[AlexShukel]

@ZeeshanTamboli
There is still a bug when rendering a Button conditionally from custom component. Please, see this example: https://codesandbox.io/s/hopeful-rubin-yskwss?file=/src/App.js

[ZeeshanTamboli]

@ZeeshanTamboli There is still a bug when rendering a Button conditionally from custom component. Please, see this example: https://codesandbox.io/s/hopeful-rubin-yskwss?file=/src/App.js

I checked it, but I'm uncertain about how to resolve it. The problem is that we rely on checking valid JSX elements within the button group, and the custom Button component is considered valid, which disrupts the styles. In the past, we depended on the elements in the DOM prior to version 5.14.9, but we made fixes for cases where different HTML elements needed to be supported (see issues #31210, #29514, #29224). I'd like to use the :first-child, :last-child CSS selector solution, but it's not supported in Emotion. We might move away from Emotion JS in version 6. However, I consider the issue you mentioned a regression because it worked before version 5.14.9. @AlexShukel, could you please open a new issue so that we can track it?

[AlexShukel]

@ZeeshanTamboli There is still a bug when rendering a Button conditionally from custom component. Please, see this example: https://codesandbox.io/s/hopeful-rubin-yskwss?file=/src/App.js

I checked it, but I'm uncertain about how to resolve it. The problem is that we rely on checking valid JSX elements within the button group, and the custom Button component is considered valid, which disrupts the styles. In the past, we depended on the elements in the DOM prior to version 5.14.9, but we made fixes for cases where different HTML elements needed to be supported (see issues #31210, #29514, #29224). I'd like to use the :first-child, :last-child CSS selector solution, but it's not supported in Emotion. We might move away from Emotion JS in version 6. However, I consider the issue you mentioned a regression because it worked before version 5.14.9. @AlexShukel, could you please open a new issue so that we can track it?

Ok, for now it is fine for us to revert to version 5.14.8, I will create a new issue soon.

UPDATE: #39488