[MenuButton] Create the MenuButtonUnstyled component

[michaldudak]

After creating demos for #30961 it became clear that leaving implementation of menu buttons to developers would force them to write a lot of code. We can create an abstraction for a button that triggers the appearance of a menu and responds to keyboard input (in a slightly different way than a normal button - pressing up/down arrow keys should also open the menu).

Bonus points for making it work with SelectUnstyled.

Note: make sure that clicking on the button when menu is open does not cause blinking, as it's currently the case in MenuUnstyled demos (see https://github.com/mui/material-ui/pull/32661#issue-1228340370, point 1)

[siriwatknp]

@michaldudak I am about to start the Menu for Joy UI as well. What do you think about using button prop to specify the menu button?

// The usage will be like this:
<MenuUnstyled button={<ButtonUnstyled>trigger</ButtonUnstyled>}>
  ...
</MenuUnstyled>

[michaldudak]

Why not follow the pattern we've got elsewhere with componentsProps?

Also, I was thinking about encapsulating all the menuButton related logic in a separate component instead of moving it in the Menu. This way we could have a standalone MenuButton component:

<MenuButton label="trigger">
  <MenuUnstyled>
  ...
  </MenuUnstyled>
</MenuButton>

What do you think?

[siriwatknp]

I like having MenuButton declare first (it is more intuitive because the MenuButton would be the first thing I want to see) but I don't think MenuUnstyled should be a child because when disablePortal: true, The menu renders inside the button which sounds incorrect.

I propose using a popup prop to pass a menu:

<MenuButton popup={<Menu>...</Menu>}>
  Trigger
</MenuButton>

By default, the MenuButton renders a button but if I want to change the component to IconButton I can do this:

const IconButton = styled('button')(...)

<MenuButton component={IconButton} popup={...}>
  <Icon />
</MenuButton>

This MenuButton could also be used with a Dialog:

<MenuButton popup={<Dialog>...</Dialog>}>
  Open
</MenuButton>

What do you think?

Implementation

// MenuButton
return (
  <React.Fragment>
    <button>...</button>
    <Context.Provider value={dataForTheMenu}>{popup}</Context.Provider>
  </React.Fragment>
)

@michaldudak Any thoughts on my proposal?

[siriwatknp]

@michaldudak Any thoughts on my proposal?

[michaldudak]

Sorry, it got lost among other notifications :/

I propose using a popup prop to pass a menu

One concern I have with this approach is that having a "large" JSX within a prop doesn't look well, and it's harder to read. Let's consider the following:

<MenuButton popup={(
  <Menu>
    <MenuItem>First</MenuItem>
    <MenuItem>Second</MenuItem>
    <MenuItem>Third</MenuItem>
  </Menu>})>
  Trigger
</MenuButton>

it feels slightly less readable and harder to write (because of nested parenthesis) than

<MenuButton label="Trigger">
  <Menu>
    <MenuItem>First</MenuItem>
    <MenuItem>Second</MenuItem>
    <MenuItem>Third</MenuItem>
  </Menu>
</MenuButton>

By default, the MenuButton renders a button but if I want to change the component to IconButton I can do this

This looks great.

This MenuButton could also be used with a Dialog

That's good, however, keep in mind that MenuButton and Menu need a "special connection" ;) For example, a menu should open and highlight its first element when Arrow Down is pressed while the menu button is focused. Similarly with Arrow Up. MenuUnstyled exposes the needed methods as an imperative handle, so MenuButton needs to have the Menu's ref.

Having written this, another idea of composition came to my mind - a controlled menu button

const [open, setOpen] = useState();

<MenuButton popupRef={menuRef} onPopupVisibleChange={setOpen}>Trigger</MenuButton>
<Menu ref={menuRef} open={open}>
  ...
</Menu>

This would decouple definitions of menu and menubutton, but require more work from developers (it won't just work upon placing it on the page).

[siriwatknp]

and it's harder to read.

I think it is better than confusion compare to:

<MenuButton label="Trigger">
  <Menu>...</Menu>
</MenuButton>

In JSX, this means Menu is a child of the MenuButton which is wrong. The Menu should be a sibling.

2: The other way is to provide a MenuProvider similar to Chakra UI:

<MenuProvider>
  <MenuButton>trigger</MenuButton>
  <Menu>...</Menu>
</MenuProvider>

3: Or back to my first proposal, add a button prop to the Menu (it looks intuitive to me) which does not require a new component.

// The usage will be like this:
<Menu button={<Button>trigger</Button>}>
  ...
</Menu>

That's good, however, keep in mind that MenuButton and Menu need a "special connection" ;)

Fair enough, let's keep the MenuButton for the menu then.

a controlled menu button

🤔 I rather not go this way.

[michaldudak]

Another idea - how about including the button in the Menu component itself (both Unstyled and Joy)? It would remove the confusion and make things simpler for users. For cases where a button is not required (like a context menu), components.Button could allow null.

Then we would have:

<Menu label="Trigger">
  <MenuItem>...</MenuItem>
  ...
</Menu>

or

<Menu components={{ Button: null }}>
...

[siriwatknp]

Another idea - how about including the button in the Menu component itself (both Unstyled and Joy)? It would remove the confusion and make things simpler for users. For cases where a button is not required (like a context menu), components.Button could allow null.

Then we would have:

<Menu label="Trigger">
  <MenuItem>...</MenuItem>
  ...
</Menu>

or

<Menu components={{Button: null}}>
...

But then what is the root slot? the button or the popper?

<Menu label="Trigger" data-testid="">
</Menu>

// becomes
<button data-testid="">...</button>
<Popper>...</Popper>

I lean toward having the button being root to have a similar experience to the Select.

For cases where a button is not required (like a context menu), components.Button could allow null.

Using button as a root slot would not work well 🥲.

Overall, I still favor MenuButton as a separate component (I might extract Select into Select and SelectButton as well because the current customization experience is not that good).

[michaldudak]

Using button as a root slot would not work well

Perhaps the root slot could be a Fragment by default.

I've taken a look at what the other libraries are doing. Chakra, Mantine, and Radix (with Select, as they don't have the menu yet) use the context approach:

<Menu>
  <MenuButton />
  <MenuList>
    <MenuItem />
  </MenuList>
</Menu>

React Spectrum has another solution:

<MenuButton>
  <button />
  <Menu>
    ...
  </Menu>
</MenuButton>

and FluentUI uses a completely different approach:

<Button menuProps={…} />

[mnajdova]

<Menu>
  <MenuButton />
  <MenuList>
    <MenuItem />
  </MenuList>
</Menu>

This looks most intuitive API to me. And, in addition, we can also support it with the components API:

<Menu
  components={{
    Button: {},
    List: {}.
  }}
/>

[siriwatknp]

@mnajdova @michaldudak I think the resulting API should weigh in:

• MenuButton should be a feature, not breaking changes (fine for Joy and Base but not for Material UI)
• Single DOM per component (this would make customization a lot easier)
• Favor explicit code over too much encapsulation. I think it is better to write a little bit more code that gives me full control of the customization rather than writing a few lines of code but I have to dig into the implementation when customization is needed.

Here is the comparison for all options:

Option 1:

encapsulating all the menuButton related logic in a separate component instead of moving it in the Menu. This way we could have a standalone MenuButton component

Playground: #35671

<MenuButton label="trigger">
  <MenuUnstyled>
  ...
  </MenuUnstyled>
</MenuButton>

Cons

• Might be confused because the result is not the same as JSX. MenuUnstyled is not inside the MenuButton.

Option 2:

Playground: #35719

Having MenuButton declare first but use a prop (popup) to open the Menu:

<MenuButton popup={(
  <Menu>
    <MenuItem>First</MenuItem>
    <MenuItem>Second</MenuItem>
    <MenuItem>Third</MenuItem>
  </Menu>})>
  Trigger
</MenuButton>

Cons

• having a "large" JSX within a prop doesn't look well, and it's harder to read.

Option 3:

Playground: #35778

<Menu label="Trigger">
  <MenuItem>...</MenuItem>
  ...
</Menu>

Cons

• what is the root slot? the button or the popper?
• customization seems harder

  // before
  <Menu variant="...">...</Menu>
  
  // with option 3
  <Menu label="Trigger componentsProps={{ listbox: { variant: '...' } }}>
  </Menu>
  

Option 4:

Similar to Chakra UI, Menu becomes a provider.

This option is similar to Option 5, please use next playground to evaluate it.

<Menu>
  <MenuButton />
  <MenuList>
    <MenuItem />
  </MenuList>
</Menu>

Cons:

• Is this considered a breaking change for Material UI?
• Menu does not render anything (it is a provider), would it confuse people who are familiar with Material UI?

Option 5:

Quite similar to option 4 but uses a provider name so that it does not break Material UI's Menu component

Playground: #35782

<MenuProvider>
  <MenuButton />
  <Menu>
    <MenuItem />
  </Menu>
</MenuProvider>

Cons:

• Need to import the Provider which looks strange if I want to apply this approach to Select

  <SelectProvider>
     <SelectButton />
     <Select>
       <SelectItem />
     </Select>
   </SelectProvider>
  

Option 6:

use hook, usePopup instead of MenuButton. (Is it possible to make the hook generic with other components e.g. dialog?)

const { getButtonProps, getMenuProps } = usePopup();

<Button {...getButtonProps()}>
<Menu {...getMenuProps()}>...</Menu>

Cons

• adding custom onClick to the button must be placed in getButtonProps({ onClick }).

Option 7:

Same as #9893 final proposal which uses React.cloneElement(). It does not introduce breaking change.

Playground: #35784

<Menu button={<Button>Open</Button>}>
  <MenuItem>...</MenuItem>
</Menu>

---

Even though I think option 4 is intuitive but I guess it would be a breaking change for Material UI (if yes, I rather go with either option 2, 5, or 6 instead)

[siriwatknp]

@oliviertassinari @danilo-leal @samuelsycamore feel free to jump into the discussion!

[michaldudak]

As for Option 4 and your worry about a breaking change - MenuUnstyled has already departed from what was in Material UI. Its implementation was completely rewritten, so we have to expect some breaking changes anyway. Now that I see all the options together, I think I like this one the most (as a user of the library, not necessarily a contributor ;))

[mnajdova]

+1 on Option 4 once again :D

[michaldudak]

Just for the record, as we've discussed in one of the meetings - @siriwatknp is going to prepare a POC in Joy, and once we consider the API good enough, we will create a proper implementation in MUI Base and base the Joy version on it.

[oliviertassinari]

From what I understand, this problem has been discussed a couple of times over the history of the project, e.g. https://github.com/mui/material-ui/issues/9893. See the difference options explored on that issue.

[siriwatknp]

After reading #9893 a couple of times, I would like everyone to reconsider the candidate options again. The discussion of the issue consists of 2 topics, the popper behavior, and the menu button. I will focus only on the menu button. The main thing @ryancogswell brought up that triggered me to reconsider is the breaking changes.

This is the API that he proposed (same as my first proposal):

<Menu button={<Button>Open Menu</Button>} variant="dropdown">
  <MenuItem>Item 1</MenuItem>
  <MenuItem>Item 2</MenuItem>
  <MenuItem>Item 3</MenuItem>
</Menu>

He already summarized all the options in https://github.com/mui/material-ui/issues/9893#issuecomment-491477259.

---

The main reason I would like everyone to reconsider is the breaking changes. The MenuUnstyled will cause the behavior breaking changes for Material UI but it does not mean that the MenuButton has to be a breaking change.

In terms of usage and customization, the only difference between button prop and Menu as a provider is how you use it:

<Menu button={<Button></Button>}>

<Menu>
  <MenuButton></MenuButton>
</Menu>

There are no differences in terms of customization because both approaches have an explicit button. However, the Menu as a provider will create a lot of work for us (creator), library maintainers (e.g. material-ui-popper-state), and developers (direct users of Material UI).

The breaking changes are not just for Material UI but for Joy UI and MUI Base as well. We will have to rewrite the Menu, create a migration codemod, rewrite docs, and update all the demos. Do we have that much time for this in v6?

cc @michaldudak @oliviertassinari @mnajdova

---

Here is the POC version of adding button prop to the Menu. No breaking changes 😀.

[mnajdova]

I still like the JSX like API better, but considering everything I would be ok if we adopt the prop based API too. This would mean that we can add this in all packages immediately right?

[oliviertassinari]

A few thoughts:

1. There are different types of menus. Having a single component called Menu is maybe an oversimplification. For example, in Radix we have Context Menu, Navigation Menu, Dropdown Menu, Select. We also talked about having a https://github.com/mui/mui-x/issues/511 and https://github.com/mui/mui-x/issues/514
2. The Menu is not necessarily the ideal abstraction to implement all of these components. In Radix, only Context Menu, and Dropdown Menu are built on top of the Menu primitive, I don't know 🤷‍♂️.
3. It seems that we are trying to solve the problem of creating a new abstraction on top of the existing ones. So I don't see why we should change how the Menu behaves. Instead, we could create a new abstraction that goes beyond having a "menu", but to have a "menu button" or a dropdown. In this argument "so we have to expect some breaking changes anyway." I think that two breaking changes are x2 the amount of pain that one breaking change introduces, so if we can skip them, great. It makes me a bit think about the car example at 15:10 https://www.ted.com/talks/dan_gilbert_why_we_make_bad_decisions 😁.
4. react-spectrum API, makes me think about the Accordion:

https://github.com/mui/material-ui/blob/a52c566848b70b1a78314443010c170b390b8ecc/packages/mui-material/src/Accordion/Accordion.js#L151

It's close to option 5 but a bit different.

---

Regarding the options, personally, I think that: 5 > 1 > 4 > 3 > 2

[michaldudak]

There are different types of menus. Having a single component called Menu is maybe an oversimplification. For example, in Radix we have Context Menu, Navigation Menu, Dropdown Menu, Select.

This is a good point. Having a different component for menus triggered by a button and those without (i.e. context menus) makes sense. I'm for having at least a MenuButton and a ContextMenu.

In https://github.com/mui/material-ui/issues/32088#issuecomment-1219425128 "so we have to expect some breaking changes anyway." I think that two breaking changes are x2 the amount of pain that one breaking change introduces, so if we can skip them, great.

Sure, breaking changes aren't welcome, but if we can significantly simplify the API while introducing them, IMO they're worth considering.

Let's consider the code from https://mui.com/material-ui/react-menu/#positioned-menu. With Option 3, my current favorite, this:

export default function PositionedMenu() {
  const [anchorEl, setAnchorEl] = React.useState<null | HTMLElement>(null);
  const open = Boolean(anchorEl);
  const handleClick = (event: React.MouseEvent<HTMLElement>) => {
    setAnchorEl(event.currentTarget);
  };
  const handleClose = () => {
    setAnchorEl(null);
  };

  return (
    <div>
      <Button
        id="demo-positioned-button"
        aria-controls={open ? 'demo-positioned-menu' : undefined}
        aria-haspopup="true"
        aria-expanded={open ? 'true' : undefined}
        onClick={handleClick}
      >
        Dashboard
      </Button>
      <Menu
        id="demo-positioned-menu"
        aria-labelledby="demo-positioned-button"
        anchorEl={anchorEl}
        open={open}
        onClose={handleClose}
        anchorOrigin={{
          vertical: 'top',
          horizontal: 'left',
        }}
        transformOrigin={{
          vertical: 'top',
          horizontal: 'left',
        }}
      >
        <MenuItem onClick={handleClose}>Profile</MenuItem>
        <MenuItem onClick={handleClose}>My account</MenuItem>
        <MenuItem onClick={handleClose}>Logout</MenuItem>
      </Menu>
    </div>
  );
}

could become this:

export default function PositionedMenu() {
  return (
    <MenuButton label="Dashboard">
      id="demo-positioned-menu"
      anchorOrigin={{
        vertical: 'top',
        horizontal: 'left',
      }}
      transformOrigin={{
        vertical: 'top',
        horizontal: 'left',
      }}
    >
      <MenuItem onClick={handleClose}>Profile</MenuItem>
      <MenuItem onClick={handleClose}>My account</MenuItem>
      <MenuItem onClick={handleClose}>Logout</MenuItem>
    </MenuButton>
  );
}

It's much more simple to use. It's also familiar and consistent as it uses patterns similar to Select. A solution like Option 5, with a separate provider, will be less straightforward. Having four different components instead of two means more writing and being harder to remember.

The main thing @ryancogswell brought up that triggered me to reconsider is the breaking changes.

This is the API that he proposed (same as my first proposal):

<Menu button={<Button>Open Menu</Button>} variant="dropdown">
  <MenuItem>Item 1</MenuItem>
  <MenuItem>Item 2</MenuItem>
  <MenuItem>Item 3</MenuItem>
</Menu>

This is a similar approach but doesn't use the customization patterns we have today in the library. We settled on using slots, so let's use them consistently.

<MenuButton label="Open Menu" slots={{ button: Button }}>
  <MenuItem>Item 1</MenuItem>
  <MenuItem>Item 2</MenuItem>
  <MenuItem>Item 3</MenuItem>
</MenuButton>

[oliviertassinari]

more writing and being harder to remember.

@michaldudak This point reminds me of a discussion with @joserodolfofreitas about how high-level the API of the data grid and date picker should be. I would like to defend the following interpretation of reality: boilerplate for our use case might not matter:

• Developers copy & paste demos, unless the use case is very common, they will never remember what's the API. Personally, even when I was building Material UI v1 at night and using it during the day, I could only really remember how to use the Button, TextField, makeStyles, theme. Everything else, I was copying & pasting from the docs. Tailwind UI's success is IMHO a strong signal that copy & paste is a superior DX for customization problems.
• Developers expect to be able to customize everything. MUI's market share is 25% in the space, what are the over 75% doing? Mostly in-house components, why? Control, you see the whole codebase. So how do we grow the adoption? IMHO it's by lowering the API. Developers can build higher level abstraction if they wish userland, most are already doing it.
• On the other hand, as a maintainer, I think that our incentive is to build APIs as high level as possible to maximize our freedom to fix bugs, and build new features without introducing breaking changes. So there is a balance to find, and the need to expose two different APIs (e.g. TextField + Input components) when the tension between these two is too strong.

We settled on using slots, so let's use them consistently.

From my perspective, the slots API is designed to solve the advanced customization problem, it's not designed for a default component use case. In the problem that we are considering, I think that the extra flexibility of React elements that can be manipulated in the render function (over React components/slots) is important.

[siriwatknp]

I understand that the proposal from @michaldudak leans toward Select component but honestly, my first intention is to make the Select lean toward Menu in Joy UI 😅

Seems like:

• Jun: push the components to be 1-to-1 mapping with the DOM.
• Michal: consolidate components to slots in a single component.

Or should MUI Base and Joy UI have different APIs because they target different audiences?

[michaldudak]

Perhaps we should ask the developers what API they'll prefer to use.

[oliviertassinari]

@michaldudak Sounds great 👍.

Also, some designers likes it when it's composable 😛 https://twitter.com/olivtassinari/status/1592951715463983104

[michaldudak]

Sure, if that's what our users want, I'm fine with it. But then, let's rethink other components' API as well because I wouldn't like to have many different customization patterns across the library.

[michaldudak]

@siriwatknp, @oliviertassinari One more question about options 4 and 5 - how do you envision setting the menu button or items' props conditionally, based on the state of the menu? For a concrete example, let's consider having a custom CSS class applied when a menu is open.

[siriwatknp]

@siriwatknp, @oliviertassinari One more question about options 4 and 5 - how do you envision setting the menu button or items' props conditionally, based on the state of the menu? For a concrete example, let's consider having a custom CSS class applied when a menu is open.

I don't think adding a custom CSS class is common for Material UI and Joy UI, otherwise, we would have seen the issues with other components already.

However, it will follow the convention using the class name with the sx prop:

<MenuButton sx={{ [`&.${menuButtonClasses.active}`]: { ... } }} />

or with the theming API:

{
  components: {
    MuiMenuButton: {
      stylesOverrides: {
        root: ({ ownerState }) => ({
           ...ownerState.active && { ... },
        })
      }
    }
  }
}

[michaldudak]

It is very important for Base components, though. And in this case, there is no possibility to use sx or a theme.

[siriwatknp]

It is very important for Base components, though. And in this case, there is no possibility to use sx or a theme.

Yeah, that's a valid point for MUI Base.

[siriwatknp]

https://github.com/mui/material-ui/pull/34997 reminds me that it is possible with:

<MenuButton slotProps={{
  root: (ownerState) => ({
    className: ownerState.active ? '...' : '...',
  })
/>

Would this work? MUI Base also support this right?

[michaldudak]

Having the "one DOM element per component" pattern makes slots and slotProps obsolete, IMO.