material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Select] - First options receives .Mui-focusVisible always

Open orest22 opened this issue 4 years ago • 15 comments

  • [x] The issue is present in the latest release.
  • [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

There is some issue with :focus-visible for Select. For the first time when there is no value selected .Mui-focusVisible class is getting applied to the first item in a list. This is not the case for the Menu component though. Also, this happens only in Chrome.

Expected Behavior 🤔

Same behaviour as the Menu component has.

Steps to Reproduce 🕹

Two components side by side:https://codesandbox.io/s/material-ui-issue-forked-udm04?file=/src/Demo.js

Steps:

Case 1:

  1. Open Select via mouse click
  2. The first item is highlighted with a red border meaning the focusVisible class was added

Case 2:

  1. Refresh a page
  2. Open Menu via mouse click everything works as expected

Context 🔦

Want have focus applied on Select only when it was focused with a keyboard. The options should also stay without focus if the keyboard wasn't used

Your Environment 🌎

Tech Version
Material-UI v5.0.0.alpha.17
React 17
Browser Chrome
TypeScript
etc.

orest22 avatar Nov 27 '20 21:11 orest22

Screen Recording 2020-11-27 at 04 07 06 PM

orest22 avatar Nov 27 '20 21:11 orest22

@orest22 Interesting, it's because of this line:

https://github.com/mui-org/material-ui/blob/5115f08e29babb11481a190d68e7fd045f3ce804/packages/material-ui-utils/src/useIsFocusVisible.js#L105

You can reproduce using the native CSS feature: https://codesandbox.io/s/material-ui-issue-forked-mk5wi?file=/src/Demo.js. I suspect a bug in Chrome.

oliviertassinari avatar Nov 28 '20 10:11 oliviertassinari

Thanks for the detailed report.

Definitely needs some investigation how this happens. Only then can we know whether this is a chrome bug or Material-UI bug.

eps1lon avatar Nov 28 '20 23:11 eps1lon

@eps1lon could it be related to a question you have asked here https://bugs.chromium.org/p/chromium/issues/detail?id=1127875 ?

orest22 avatar Dec 03 '20 02:12 orest22

This bug happens for me on a clean install on latest MUI version, both on chrome and firefox. Here is a sandbox.

fxlemire avatar Oct 19 '21 02:10 fxlemire

This is still a problem btw. Anyway to fix this?

CarlosAmaral avatar Jul 13 '22 12:07 CarlosAmaral

I'm currently experiencing this on Chrome using MenuUnstyled and MenuItemUnstyled.

mraballard avatar Sep 29 '22 17:09 mraballard

Same here. I have a search input in my MenuUnstyled component and auto-focus on that is getting overridden and comes to the first element in the list. Anyway to fix this ?

stuthib avatar Nov 23 '22 03:11 stuthib

I also encountered the same event. I think the environment is the same as @orest22. Are there any plans to fix this?

hiroki0404508 avatar Nov 29 '22 03:11 hiroki0404508

Well this issue is not related to the MUI. It is something related to :focus-visible that matches on initial programmatic focus w3c :focus-visible And i think it is more related to the heurestic spec rule number 4 check the comment

  1. If the user interacts with the page via a pointing device, such that the focus is moved to a new element which does not support user input, the newly focused element should not match :focus-visible.

@orest22 i think you can reproduce the same thing for the menu with two possible implementations of Anchor that opens the menu:

  1. Button with mousedown and preventdefault this is the same implementation on Select in this PR to solve PERF issue.
  2. Change the Button to Div (click / mousedown)

Same behavior with vanilla js in this example

@oliviertassinari possible fix is to remove this line useIsFocusVisible the focus-visible should be applied only if the user uses keyboard no need to check focus-visible matches. For more details this article https://www.deque.com/blog/give-site-focus-tips-designing-usable-focus-indicators/ in MDN

SaidMarar avatar Jan 30 '23 12:01 SaidMarar

Has there been any progress for this?

allicanseenow avatar May 01 '23 07:05 allicanseenow

I am also experiencing same!!

qramit avatar May 24 '23 08:05 qramit

This issue is still there - I understand that according to spec, the first item gets the focus automatically... but so far MUI was good at applying the focusVisible class on focused elements only when user used keyboard. So the fix is to remove focusVisible class if the user interacted using mouse. Interestingly this issue is not there if user navigated from another screen in single page app.

harinair avatar Jun 16 '23 08:06 harinair

This issue can be tackled by using a MenuItem Component (as 1st Option) and setting it's display property to "none".

chirag-chan-dream11 avatar Jul 11 '23 08:07 chirag-chan-dream11

still the issue, but this workaround helped

This issue can be tackled by using a MenuItem Component (as 1st Option) and setting it's display property to "none".

davidko5 avatar May 01 '24 17:05 davidko5

Today I leaned about this option: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus#focusvisible / https://html.spec.whatwg.org/multipage/interaction.html#focus-management-apis:

.focus({ focusVisible: false });

that we could maybe used to solve this issue and dodge https://github.com/w3c/csswg-drafts/issues/5885. Now, I imagine that we can't systematically use this, first the false option isn't supported, second, we would need to bring #42467 back or actually, simpler, just look at the event type that triggered the programmatic focus cc @DiegoAndai.

oliviertassinari avatar Jul 14 '24 10:07 oliviertassinari

IMO the path forward for focus visible handling in Material UI should rely on :focus-visible. That would mean deprecating and eventually removing the .Mui-focusVisible classes, as well as our usage of them for styling, in favor of :focus-visible. This aligns better with current web development practices and technologies. What do you think @aarongarciah? If you agree, I'll create an issue to track this work.

DiegoAndai avatar Jul 24 '24 20:07 DiegoAndai

Agree with @DiegoAndai. If possible, we should try to simplify and move work to CSS. I don't have the right context to know if :focus-visible is sufficient in our case or if we need a more advanced JS-centric solution (we're probably don't need something as advanced as React Aria https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/useFocusVisible.ts)

aarongarciah avatar Jul 25 '24 06:07 aarongarciah