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

[docs] Use focus-visible instead of focus for Menu demos #36847

Merged
merged 2 commits into from Apr 28, 2023

Conversation

michaldudak
Copy link
Member

As spotted in #36400 (review).
This PR changes the :focus to :focus-visible (or .Mui-focusVisible) styles on menu button.

@michaldudak michaldudak added docs Improvements or additions to the documentation component: menu This is the name of the generic UI component, not the React module! labels Apr 11, 2023
@mui-bot
Copy link

mui-bot commented Apr 11, 2023

Netlify deploy preview

https://deploy-preview-36847--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against c301ab3

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 24, 2023
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Looks great! I still find it a bit unclear when to use focus vs focus-visible... 🤔 I know the latter is explicitly for when the component is focused through tabbing, right? But it seems to also do the same thing as just the focus (which would be focusing through click/tap). Is this a correct interpretation? Just for the sake of learning.

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

Yup, focus-visible is only applied when an element receives focus as an effect of user interaction with the keyboard. In the case of Menu demos, we had the weird state where you clicked the trigger button, the menu opened and was focused, you clicked on a menu item that closed the menu, and only then you could see the focus ring on the button.
Having the ring applied only on focus-visible does look more consistent.

@michaldudak michaldudak merged commit 8687f89 into mui:master Apr 28, 2023
18 checks passed
@michaldudak michaldudak deleted the menu-demos-focus branch April 28, 2023 15:06
binh1298 pushed a commit to binh1298/material-ui that referenced this pull request May 17, 2023
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! docs Improvements or additions to the documentation
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants