mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
92.39k stars 31.82k forks source link

TypeError using conditional rendering under ListSubheader #36889

Closed danielplewes closed 1 year ago

danielplewes commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/friendly-zhukovsky-i08cjd?file=/demo.js

Steps:

  1. ListSubheader is the first item in a MenuList
  2. Next item is {false} or {null}
  3. No items in list are selected

Current behavior 😯

TypeError: Cannot read properties of null (reading 'props')

Expected behavior 🤔

Invalid items should be ignored

Context 🔦

Conditional rendering otherwise works, it's just when the second item isn't a valid element.

The problem originates here, which is incrementing the activeItemIndex when a disabled or ListSubheader item is found. If the next item isn't valid activeItemIndex gets stuck and throws an error here.

The fix should be as simple as adding the same incrementing logic when the activeItemIndex is set to an invalid item. https://github.com/mui/material-ui/blob/66a69ececd29cafd55ba7d52a56442771aa46b6e/packages/mui-material/src/MenuList/MenuList.js#L213

    if (!React.isValidElement(child)) {
      if (activeItemIndex === index) {
        activeItemIndex += 1;
        if (activeItemIndex >= children.length) {
          // there are no focusable items within the list.
          activeItemIndex = -1;
        }
      }
      return;
    }

Your environment 🌎

npx @mui/envinfo ``` System: OS: Windows 10 10.0.19044 Binaries: Node: 14.17.1 - C:\Program Files\nodejs\node.EXE Yarn: 1.22.19 - C:\Program Files\nodejs\yarn.CMD npm: 6.14.13 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: Not Found Edge: Spartan (44.19041.1266.0), Chromium (103.0.1264.37) npmPackages: @emotion/react: ^11.10.6 => 11.10.6 @emotion/styled: ^11.10.6 => 11.10.6 @mui/base: 5.0.0-alpha.124 @mui/codemod: 5.11.14 @mui/core-downloads-tracker: 5.11.16 @mui/docs: 5.11.13 @mui/envinfo: 2.0.8 @mui/icons-material: 5.11.16 @mui/internal-waterfall: 1.0.0 @mui/joy: 5.0.0-alpha.74 @mui/lab: 5.0.0-alpha.125 @mui/markdown: 5.0.0 @mui/material: 5.11.16 @mui/material-next: 6.0.0-alpha.80 @mui/private-theming: 5.11.13 @mui/styled-engine: 5.11.16 @mui/styled-engine-sc: 5.11.11 @mui/styles: 5.11.16 @mui/system: 5.11.16 @mui/types: 7.2.3 @mui/utils: 5.11.13 @mui/x-data-grid: 6.0.0-alpha.14 @mui/x-data-grid-generator: 6.0.0-alpha.14 @mui/x-data-grid-premium: 6.0.0-alpha.14 @mui/x-data-grid-pro: 6.0.0-alpha.14 @mui/x-date-pickers: 6.0.0-alpha.14 @mui/x-date-pickers-pro: 6.0.0-alpha.14 @mui/x-license-pro: 6.0.0-alpha.14 @types/react: ^18.0.26 => 18.0.26 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 styled-components: 5.3.9 typescript: ^4.9.5 => 4.9.5 ```
ZeeshanTamboli commented 1 year ago

Could you share some use-case/context on rendering a menu item conditionally?

github-actions[bot] commented 1 year ago

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

danielplewes commented 1 year ago

@ZeeshanTamboli we have a menu that will conditionally include items based on permissions, something like:

          <MenuButton icon={MoreHorizIcon}>
            <ListSubheader disableSticky={true} className={classes.listSubHeaderRoot}>
              <FormattedMessage {...messages.dropdownActions} />
            </ListSubheader>
            {hasPermission(permissions.DELETE) && (
              <MenuItem onClick={() => delete()}>
                <FormattedMessage {...messages.dropdownDelete} />
              </MenuItem>
            )}
           {hasPermission(permissions.DUPLICATE) && (
              <MenuItem onClick={() => duplicate()}>
                <FormattedMessage {...messages.dropdownDuplicate} />
              </MenuItem>
            )}

Everything works fine for the duplicate block, it's hidden based on permission. But for delete since it's the first item it will crash if the user doesn't have the DELETE permission.

It works fine on 5.11.10, but broke when we updated to 5.11.11, so we're stuck on the old version until this is resolved.