mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.28k stars 32.12k forks source link

[material-ui][Menu] After opening Menu, it is not possible to refresh page by ctrl + R #43215

Closed JaniecMichal closed 2 weeks ago

JaniecMichal commented 1 month ago

Steps to reproduce

Link to live example: (required)

Steps:

  1. Open Menu
  2. Choose on keyboard ctrl + r
  3. Shortcut is igonored, page has not refreshed

Current behavior

This is weird case because we using this component in our apps and we have some places when ctrl + r after open Menu don't works. We have also places when the same component but rendered for different users ones works and ones not works

Expected behavior

Refreshing by keyboard shortcut should works always?

Context

Maybe it is caused by internal feature of Menu component that after click into key is searching item which match (by first letter)

Your environment

Output from `npx @mui/envinfo` goes here. System: OS: Linux 6.5 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish) Binaries: Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node npm: 10.7.0 - ~/.nvm/versions/node/v20.10.0/bin/npm pnpm: 8.15.1 - ~/.nvm/versions/node/v20.10.0/bin/pnpm Browsers: Chrome: 127.0.6533.99

Search keywords: Menu, MenuItem, refresh, ctr + r

ZeeshanTamboli commented 1 month ago

Could you provide a minimal reproduction? A live example would be ideal. You can start with this StackBlitz template.

It works on the MUI site next.mui.com:

https://github.com/user-attachments/assets/b84a020d-dd46-4518-a90a-23b0397b5825

github-actions[bot] commented 3 weeks 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.

JaniecMichal commented 3 weeks ago

@ZeeshanTamboli I just explored reason of this behavior. Looks that if MenuItem have content which start from R letter or any other letter then using shortcut with this letter won't work. You can see this issue also if you use attached by you this StackBlitz template.

MateuszGroth commented 3 weeks ago

https://stackblitz.com/edit/react-bdf3td?file=Demo.tsx

ZeeshanTamboli commented 3 weeks ago

@JaniecMichal The MenuItem component has focus when menu is opened, and the Menu listens for key events, such as typing the first letter of an item. This can prevent the default browser action, like refreshing the page with Ctrl + R.

You can stop the event from reaching the Menu by calling event.stopPropagation() in MenuItem's handleKeyDown when Ctrl is pressed. This prevents the Menu from intercepting the event, allowing the browser to handle the page refresh as expected.

See the example here: https://stackblitz.com/edit/react-bdf3td-pldymk?file=Demo.tsx.

MateuszGroth commented 3 weeks ago

@ZeeshanTamboli The fix does not work on mac - probably need to check for the metaKey instead of the ctrlKey

ZeeshanTamboli commented 3 weeks ago

@ZeeshanTamboli The fix does not work on mac - probably need to check of metaKey instead of the ctrlKey

Yup.

MateuszGroth commented 3 weeks ago

Shouldn't it work like that by default tho? I can imagine this being an issue for more people

ZeeshanTamboli commented 3 weeks ago

Shouldn't it work like that by default tho?

Alright. I compared the Menu component with other libraries like React Aria, Radix UI, and even Base UI, which all support this feature. Maybe we should too, or wait until v7. I'd like to hear others' opinions. cc @aarongarciah @DiegoAndai @michaldudak

I can imagine this being an issue for more people

From what I found, no one has reported this issue since the Material UI Menu component was introduced.

aarongarciah commented 3 weeks ago

I'd consider this a bug. With the migration to Base UI in the horizon, I'm not sure we should be spending time on improving Menu.

I'll get back to you soon.

aarongarciah commented 3 weeks ago

I added the bug label. Contributions are more than welcome! This is the part of the code that handles the typeahead: https://github.com/mui/material-ui/blob/f1a5b88304b750aba3a1620549f08b16bac7ffc7/packages/mui-material/src/MenuList/MenuList.js#L168-L195

We should not prevent events that include modifier keys such as ctrl, meta, alt. For reference, here's the useTypeahead implementation from Floating UI: https://github.com/floating-ui/floating-ui/blob/master/packages/react/src/hooks/useTypeahead.ts#L155-L166

DiegoAndai commented 2 weeks ago

Added to the "Material UI with Base UI" milestone in case we don't get a contribution before it.

github-actions[bot] commented 2 weeks ago

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] We value your feedback @JaniecMichal! How was your experience with our support team? If you could spare a moment, we'd love to hear your thoughts in this brief Support Satisfaction survey. Your insights help us improve!