radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.85k stars 821 forks source link

There is no way to control the hover, focus, and focus-visible styles for DropdownMenuItem #1164

Closed KelvinOm closed 2 years ago

KelvinOm commented 2 years ago

Bug report

There is no possibility to set styles for hover, focus, and focus-visible states separately for DropdownMenuItem

Current Behavior

The focus is set automatically and because of this, even just when hovering, the elements are in focus.

Expected behavior

It is possible to apply different styles for different states Example: https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html#

Reproducible example

Here is an attempt to apply different styles for DropdownMenuItem. Please take a look at lines 71-73 https://codesandbox.io/s/naughty-chebyshev-bhnv3f?file=/App.js

benoitgrelard commented 2 years ago

Hi @KelvinOm,

This is really by design. Have a read through this similar issue where I responded with more details: https://github.com/radix-ui/primitives/issues/956#issuecomment-961819460

KelvinOm commented 2 years ago

Hi @benoitgrelard. Thank you so much for such a quick response.

I understand your arguments about one focused element and agree with them. But what about making different styles for mouse and keyboard. I mean different styles for :focus and :focus-visible. This doesn't contradict your arguments and gives flexibility in customization.

benoitgrelard commented 2 years ago

The problem with :focus-visible is that it relies on heuristics applied by the browser to "guess" the input modality. In most browsers (if not all), a programatic focus will be dimmed "visible" thus, both mouse and keyboard will appear "visible".

KelvinOm commented 2 years ago

I got you. Thanks! 👍

KelvinOm commented 2 years ago

@benoitgrelard Could you please add this information to the docs? I believe it can save some time for consumers of Radix.

KelvinOm commented 2 years ago

And one more question: Could you tell please why the same approach as in the menu is not used in Toolbar?

Screenshot 2022-02-20 at 23 11 41
benoitgrelard commented 2 years ago

And one more question: Could you tell please why the same approach as in the menu is not used in Toolbar?

Screenshot 2022-02-20 at 23 11 41

Because this is "mixed" pattern, ie. a composition of potentially lots of different controls put together so there isn't a "single" concept for "highlighted". This here really is focus moving around and it wouldn't make sense to move focus whilst hovering.

I do agree with you though that this could present similar issues depending on how you decide to style your hover states. Personally in a toolbar, I would probably forego the hover states alltogether to avoid the issue.

KelvinOm commented 2 years ago

I understand your position.

But for me as a consumer, there is no way to make consistent behavior for the dropdown menu and for the toolbar. Only if I fork dropdown menu, which is completely wrong in my opinion.

I still think it's not very right not to give consumers the opportunity to be able to adjust the state of the hover in the dropdown menu. As a library, you can give recommendations on best practices, and from my perspective, this is the best approach. This will allow the library to be more flexible.

benoitgrelard commented 2 years ago

But for me as a consumer, there is no way to make consistent behavior for the dropdown menu and for the toolbar.

What do you mean by that? The DropdownMenu styling will be consistent whether inside or outside a toolbar no?

KelvinOm commented 2 years ago

I mean, I don't have the opportunity to make such picture.

Screenshot 2022-02-21 at 15 48 32

In the DropdownMenu I can use either box-shadow or background but not both at once.

Imagine that we open the menu by clicking on the button in Toolbar. It turns out that we have one behavior in the Toolbar and completely different behavior in the DropdownMenu. I suppose from the point of UX, this is not very good.

benoitgrelard commented 2 years ago

I mean, I don't have the opportunity to make such picture. Screenshot 2022-02-21 at 15 48 32

In the DropdownMenu I can use either box-shadow or background but not both at once.

Imagine that we open the menu by clicking on the button in Toolbar. It turns out that we have one behavior in the Toolbar and completely different behavior in the DropdownMenu. I suppose from the point of UX, this is not very good.

But from the POV of the dropdown menu, it will be consistent overall though. It's up to you then whether you want to use hover states on the toolbar items or not.

I believe I have already explained why it has to be this way for menus, so I don't think we can do anything else here really.

KelvinOm commented 2 years ago

It's up to you then whether you want to use hover states on the toolbar items or not.

I can decide I want to use hover or not in Toolbar, but I can't do this in DropdownMenu because the library does not give me such an opportunity.

I don't think we can do anything else here really.

You could give consumers the ability to customize states.

Anyway, thanks a lot for the answers and for the excellent library! :+1:

benoitgrelard commented 2 years ago

…but I can't do this in DropdownMenu because the library does not give me such an opportunity.

I guess the point I'm trying to make is that it's not really just us deciding we should do it this way. It's a well established UX/accessibility pattern that has been designed/used by many other people far more experienced than us.

As an example, here are native menus (dropdown and menubar) and native select in MacOS:

https://user-images.githubusercontent.com/1539897/154947621-a160cc67-7bb6-463f-b03d-6c59fec1133d.mov

https://user-images.githubusercontent.com/1539897/154947630-13b19894-8496-450f-ab40-2ddf7719af54.mov

https://user-images.githubusercontent.com/1539897/154947647-f5c4210e-3593-4267-a217-4ff1ac6c45a1.mov

KelvinOm commented 2 years ago

@benoitgrelard I understand all this and accept it. I'm just trying to say that if the library provides this opportunity then it would become a little more flexible. We cannot know all the cases of using the component and sometimes it may be necessary to do something custom.

KelvinOm commented 2 years ago

Another case. If the DropdownMenu is open and I click on the address bar, the focus will stop working. But I can still hover over a menu item and select something. The hover in such a situation would work out fine.

https://user-images.githubusercontent.com/11555074/155072981-ecea1b46-fa40-46c0-b996-a3d5ca7d4bba.mov

jjenzz commented 2 years ago

@KelvinOm That will be fixed by the following proposed change https://github.com/radix-ui/primitives/issues/956#issuecomment-963085494

ivanbanov commented 2 years ago

@jjenzz is it expected to have the proposed change being released soon?

jjenzz commented 2 years ago

@ivanbanov i'm not on the modulz team anymore i'm afraid so perhaps @benoitgrelard or @andy-hook can help?

benoitgrelard commented 2 years ago

I am closing this issue now as I have captured the request for the enhancement in a new issue: #1289

zecka commented 1 year ago

If we need more flexibility about when use focus style, maybe we can use library such as what-input in addition of radix-ui.

aditya22314 commented 2 months ago

@benoitgrelard Also there is no option to open the DropMenuContent while hovering it .In the docs dropdown works only when clicking the DropMenuTrigger Items .