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

Audit focusableWhenDisabled #656

Closed
vladmoroz opened this issue Sep 27, 2024 · 8 comments · Fixed by #734
Closed

Audit focusableWhenDisabled #656

vladmoroz opened this issue Sep 27, 2024 · 8 comments · Fixed by #734
Assignees
Labels

Comments

@vladmoroz
Copy link
Contributor

vladmoroz commented Sep 27, 2024

  • Remove focusableWhenDisabled from Menu.Trigger
  • Make sure Switch and Checkbox aren't focusable when disabled
  • Review any other usage of the prop
@github-actions github-actions bot added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 27, 2024
@vladmoroz vladmoroz changed the title Remove focusableWhenDisabled from Menu.Trigger Audit focusableWhenDisabled Sep 27, 2024
@colmtuite
Copy link
Contributor

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 colmtuite added this to Base UI Oct 7, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Base UI Oct 7, 2024
@colmtuite colmtuite moved this from Backlog to Selected in Base UI Oct 7, 2024
@onehanddev
Copy link
Contributor

onehanddev commented Oct 8, 2024

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

@michaldudak
Copy link
Member

@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 michaldudak removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 8, 2024
@onehanddev
Copy link
Contributor

@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
Copy link
Contributor

onehanddev commented Oct 13, 2024

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 pushed a commit to onehanddev/base-ui that referenced this issue Oct 13, 2024
- Removed focusableWhenDisabled from Menu trigger
- Passed down disabled prop to the component renderer to enable the disabled state to the DOM element
onehanddev added a commit to onehanddev/base-ui that referenced this issue Oct 13, 2024
onehanddev added a commit to onehanddev/base-ui that referenced this issue Oct 13, 2024
@onehanddev
Copy link
Contributor

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!

@michaldudak
Copy link
Member

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?

Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants