Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MenuUnstyled] Create MenuUnstyled and useMenu #30961

Merged
merged 35 commits into from
Mar 7, 2022

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Feb 7, 2022

This PR introduces the unstyled menu component and friends: MenuUnstyled, MenuItemUnstyled, useMenu and useMenuItem.
This is a rewrite of the components used in Material UI and it's based on the new useListbox hook.

The unstyled Menu differs from its Material counterpart as it contains only the list of items, without the element that triggers the appearance of the list. It's more similar to Material UI's MenuList in this matter. This helps developers make it more customizable. An example of creating a button triggering a menu is included in the docs.

This PR does not enable support for nested menus. This is to be implemented in a separate PR, taking into account work done in #20591 (cc @EsoterikStare).

Preview - https://deploy-preview-30961--material-ui.netlify.app/base/react-menu/

@mui-bot
Copy link

mui-bot commented Feb 7, 2022

Details of bundle changes

@material-ui/unstyled: parsed: +6.72% , gzip: +5.36%

Generated by 🚫 dangerJS against ad71ebf

@michaldudak michaldudak requested a review from a team February 16, 2022 12:04
@michaldudak michaldudak added component: menu This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Feb 16, 2022
@michaldudak michaldudak marked this pull request as ready for review February 16, 2022 12:06
@mnajdova
Copy link
Member

Shouldn't we skip disabled items when using the keyboard? I don't think this is a valid state:
image


#### CSS classes

The MenuUnstyled does not receive any state classes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The MenuUnstyled does not receive any state classes.
The MenuUnstyled does not set any state classes.

The components sets these classes no?


The MenuUnstyled does not receive any state classes.

The MenuItemUnstyled can receive the following classes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

border-bottom: none;
}

&:focus-visible {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that only the technology preview of Safari supports this at the moment. We should recommend the polyfill in the meantime.

Generally, the demos should have the same support matrix the library has in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I forgot to implement the focus-visible polyfill for menu items. Will fix.

</TriggerButton>
<Popper
placement="bottom-start"
open={isOpen && buttonRef.current != null}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reading a ref during render which is generally not safe. This is why we have the pattern for anchorEl that puts the element in React state instead of using a ref.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks!

if (isOpen) {
contentRef.current?.focus();
}
}, [isOpen, contentRef]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be doing what you think it does. contentRef has no impact on the effect behavior while the effect is not synced with contentRef.current

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was exactly my intention. contentRef isn't really needed in the dependency array.

@eps1lon
Copy link
Member

eps1lon commented Feb 21, 2022

It's a bit odd to me that the default pattern for a Menu does not use it as a popup. In my perception Menus are always in a popup.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 24, 2022
@michaldudak
Copy link
Member Author

michaldudak commented Feb 24, 2022

I am thinking about using the useMenuItem hook inside the Joy's ListItemButton.

@siriwatknp Sure! Hooks are meant to be composed to make higher-level components. In case of useMenuItem it's just a small utility to register the item within the Menu to be able to be notified about whether it should be selected.

From the design perspective, I think the content in the MenuPopup is a bunch of list components with keyboard support

Indeed! Just note that the menu implements a roving focus (only the currently selected item has tabIndex: 0 and the rest has -1), so it's not possible to navigate through items with the Tab key - not sure if this is something you want.

useMenuItem({ component: 'button', ref }) // what's component for?

The useButton hook (which is used by useMenuItem needs to know if it's applied on a button element or something else. Props such as role, disabled/aria-disabled, and event handlers depend on it.
I am considering changing this API as I don't really like how it's used. I will refactor the Joy implementation if I do so.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 25, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2022
@mnajdova
Copy link
Member

mnajdova commented Mar 1, 2022

Some problems found after an a11y review:

  • esc should close the menu and focus the trigger element (implemented in Material-UI)
  • you should be able to select the element by typing the first letter (implemented in Material-UI)
  • the button opening the Menu should have the corresponding aria attributes: aria-haspopup, aria-controls (to reference the menu it opens), aria-expanded (when the menu is opened) (implemented in the demos for Material-UI)
  • the button opening the Menu should open the menu on Down/Up arrow (not implemented in Material-UI)

References:

};

const handleButtonKeyDown = (event) => {
if (event.key === 'ArrowDown' || event.key === 'ArrowUp') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the ArrowUp is pressed, the last element should be foced when the menu opens.

@michaldudak
Copy link
Member Author

esc should close the menu and focus the trigger element (implemented in Material-UI)

Implemented in the demos. I'll consider creating an unstyled MenuButton (or DropdownButton) that has this functionality built-in. It could also be used for a Select.

you should be able to select the element by typing the first letter (implemented in Material-UI)

This makes more sense in cases where items are ordered alphabetically (which is rarely the case in a menu). I am going to implement it anyway in Listbox (which is what Select and Menu use internally).

the button opening the Menu should have the corresponding aria attributes: aria-haspopup, aria-controls (to reference the menu it opens), aria-expanded (when the menu is opened) (implemented in the demos for Material-UI)

This is now fixed

the button opening the Menu should open the menu on Down/Up arrow (not implemented in Material-UI)
If the ArrowUp is pressed, the last element should be foced when the menu opens.

👍 Working on it

@michaldudak
Copy link
Member Author

the button opening the Menu should open the menu on Down/Up arrow (not implemented in Material-UI)
If the ArrowUp is pressed, the last element should be foced when the menu opens.

@mnajdova this should work as expected now.

event.preventDefault();
setAnchorEl(event.currentTarget);
if (event.key === 'ArrowUp') {
menuActions.current?.highlightLastItem();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we need to use useImperativeHandle here. Is there a different option? One additional reason to have the MenuButton as a standalone component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to this:

      <TriggerButton
        type="button"
        onClick={handleButtonClick}
        onKeyDown={handleButtonKeyDown}
        ref={buttonRef}
        aria-controls={isOpen ? 'simple-menu' : undefined}
        aria-expanded={isOpen || undefined}
        aria-haspopup="menu"
      >
        Language
      </TriggerButton>

Has too much boilerplate which is important to not be missed. I would propose to tackle this in the next PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I absolutely agree it's too much code. I will create the MenuButton for sure.
As for useImperativeHandle - I realize it's not quite in the spirit of React, but I couldn't think of a better option that would work with the current MenuUnstyled design. I'm happy to discuss alternatives.

Comment on lines +121 to +122
<Popper {...popperProps} ref={forwardedRef}>
<Listbox {...listboxProps}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, we talked about merging these one into one DOM element right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if we had a usePopper hook, we could apply the popup behavior to the listbox itself and avoid creating a wrapper div.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior looks good. Next steps would be:

  • create MenuButton component
  • merge the two dom elements we have in the MenuUnstyled - root and listbox into one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants