fix: add a props to make the menu focus the trigger when closing #355
Conversation
Deploying compound-web with
|
| Latest commit: |
8748045
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6bd3b04d.compound-web.pages.dev |
| Branch Preview URL: | https://florianduros-focus-menu.compound-web.pages.dev |
florianduros
requested review from
dbkr and
robintown
and removed request for
a team
| @@ -123,9 +129,11 @@ export const Menu: FC<Props> = ({ | |||
| align={align} | |||
| sideOffset={8} | |||
| onCloseAutoFocus={(event) => { | |||
| // https://www.radix-ui.com/primitives/docs/components/dropdown-menu#content => onCloseAutoFocus | |||
| // Prevent the default behavior of focusing the trigger when the menu closes | |||
There was a problem hiding this comment.
Returning focus is the wai-aria specified behaviour, when would we want focusTriggerOnClose=false?
There was a problem hiding this comment.
It was for the case of TAC: #147
Maybe it make more sense to put the props to true by default
There was a problem hiding this comment.
Looks like the TAC is not accessible then, I just tried in EW and closing the tac resets my tab position to the start of the page which is very much a no-no. The tooltip showing again is a nit at best vs a major accessibility failure. Might want to special-case the focus return for keyboard accessibility vs pointer interaction if hiding the tooltip on close is important, or a way to not show the tooltip on menu close somehow.
There was a problem hiding this comment.
In succinct terms, I think focusTriggerOnClose is misled and needs to be forced to true generally, including for the TAC.
There was a problem hiding this comment.
Okay, I'll revert then #147
There was a problem hiding this comment.
I think as a safe interim solution changing it to not apply for keyboard interaction might be saner, to not regress the general UX, the more intricate solution can always come later
There was a problem hiding this comment.
Maybe I'm wrong but radix doesn't offer this kind of precision in its event. It is firing a custom event without info if it's from a keyboard or pointer interaction (for both escape or menu item selection)
There was a problem hiding this comment.
That's daft. radix-ui/primitives#2248 (comment) is the issue with a potential workaround
There was a problem hiding this comment.
I read the issue and I think it's different of our use case. We don't have a tooltip inside a popover but on the popover trigger. Stopping propagation makes the first row of the tac unfocused, the user can't interact with it.
I don't see any good solution where UX and a11y are ok. I'll remove the #147 to make the TAC accessible.
Due to #147
We want to have the focus to be back on the trigger when the menu closes.