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 focus states on disabled components #734

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

onehanddev
Copy link

@onehanddev onehanddev commented Oct 13, 2024

Changes -

  • Removed focusableWhenDisabled from Menu trigger
  • The disabled prop has been passed down to the component renderer, allowing the Checkbox and Switch components to appropriately reflect their disabled state in the DOM elements.

[] I have followed (at least) the PR section of the contributing guide.

hakim-toptal and others added 3 commits October 13, 2024 13:05
- Removed focusableWhenDisabled from Menu trigger
- Passed down disabled prop to the component renderer to enable the disabled state to the DOM element
@mui-bot
Copy link

mui-bot commented Oct 13, 2024

Netlify deploy preview

https://deploy-preview-734--base-ui.netlify.app/

Generated by 🚫 dangerJS against 8ce05b3

@onehanddev onehanddev changed the title Audit focus states disabled components Audit focus states on disabled components Oct 13, 2024
@onehanddev onehanddev marked this pull request as ready for review October 13, 2024 08:35
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! With a few tweaks this will be good to merge.

@@ -98,6 +98,7 @@ const CheckboxRoot = React.forwardRef(function CheckboxRoot(
extraProps: {
...otherProps,
...otherGroupProps,
disabled,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing disabled here, it should be set by useCheckboxRoot's getButtonProps. You can remove aria-disabled and have just disabled prop returned from there.
Same with the Switch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out! 🙏 I’ve updated the code.
Please let me know if there are any further changes needed. 😊

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that removing aria-disabled is causing CircleCI’s browser tests to fail. I suspect this might be an accessibility (a11y) issue. Do you think we should reintroduce aria-disabled alongside the disabled prop?

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

Successfully merging this pull request may close these issues.

5 participants