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

Audit `focusableWhenDisabled`

Open vladmoroz opened this issue 1 year ago • 4 comments

  • Remove focusableWhenDisabled from Menu.Trigger
  • Make sure Switch and Checkbox aren't focusable when disabled
  • Review any other usage of the prop

vladmoroz avatar Sep 27 '24 10:09 vladmoroz

We decided at one point to allow menuitems and other components that are inside widgets (following on ARIA guidelines), to remain focusable when disabled. But other standalone components would not be focusable when disabled.

colmtuite avatar Oct 07 '24 11:10 colmtuite

Hi @michaldudak, good morning!

I’ve been following this issue and have already begun working on it. I’m planning to open a draft PR later today. Apologies for not informing you sooner—please let me know if you’d prefer me to proceed with the PR or if you’d like to take it forward instead. I’m happy with either approach.

Thanks, and I look forward to your guidance!

cc: @vladmoroz

onehanddev avatar Oct 08 '24 06:10 onehanddev

@onehanddev feel free to continue with the PR.

Our current idea for this is:

  • Every component that has a roving tabindex (so currently menu items and radio buttons and tabs) should be focusable when disabled
  • Buttons, checkboxes, switches, and other "standalone" components should not be focusable when disabled.
  • This behavior should not be customizable (at least for now; we can revisit this if users need it)

michaldudak avatar Oct 08 '24 10:10 michaldudak

@michaldudak Thanks for the detailed information on this issue. I have a quick question regarding point 1. You mentioned that radio buttons should be focusable when disabled, but the default behaviour of radio buttons doesn’t allow for that functionality. When we focus on a single radio button, it automatically selects the option. Did you mean that we should be able to focus on a radio group when it’s disabled?

onehanddev avatar Oct 08 '24 11:10 onehanddev

I was wondering, if we allow focus on a disabled ‘Tab’, what content should be displayed beneath it? Should it show the content from the previously selected tab, or should there be no content at all? If there is no content, would that cause a layout shift issue?

Is it okay if I start by creating a small PR for changes that don’t require much thought, such as:

1.	Removing focusableWhenDisabled from Menu.Trigger
2.	Ensuring that Switch and Checkbox components aren’t focusable when disabled

@michaldudak @colmtuite

onehanddev avatar Oct 13 '24 07:10 onehanddev

Could you please review the PR at this link? I plan to work on components with roving tabindexes in a separate PR once I gain a clearer understanding of how to approach it (please refer to my earlier comments).

I would appreciate any suggestions or feedback regarding my work, as this is my first time contributing to open source, and I want to ensure I’m on the right track. Thank you!

onehanddev avatar Oct 13 '24 08:10 onehanddev

Hi! Good point about radio buttons and tabs in the activateOnFocus mode. The ARIA spec doesn't mention anything about disabled items in radio groups. I'd say we can skip them. @colmtuite, @vladmoroz - any thoughts on this?

michaldudak avatar Oct 23 '24 09:10 michaldudak

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.

github-actions[bot] avatar Oct 30 '24 15:10 github-actions[bot]