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

[base-ui][Switch] Add missing role attribute #40907

Merged
merged 9 commits into from
Feb 14, 2024
Merged

[base-ui][Switch] Add missing role attribute #40907

merged 9 commits into from
Feb 14, 2024

Conversation

KirankumarAmbati
Copy link
Contributor

@KirankumarAmbati KirankumarAmbati commented Feb 3, 2024

Related to #40615

Changes in this PR:

Added the missing accessibility features for the Switch component and useSwitch hook:

  • Root element's role should be switch
  • Root element should have aria-checked attribute

@KirankumarAmbati KirankumarAmbati marked this pull request as draft February 3, 2024 13:56
@KirankumarAmbati KirankumarAmbati marked this pull request as ready for review February 3, 2024 13:56
@mui-bot
Copy link

mui-bot commented Feb 3, 2024

Netlify deploy preview

https://deploy-preview-40907--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 8f4894c

@oliviertassinari oliviertassinari added the package: base-ui Specific to @mui/base label Feb 3, 2024
@@ -103,6 +103,9 @@ const Switch = React.forwardRef(function Switch<RootComponentType extends React.
elementType: Input,
getSlotProps: getInputProps,
externalSlotProps: slotProps.input,
additionalProps: {
role: 'switch', // Switch component should have the role "switch" to meet a11y guidelines
Copy link
Member

Choose a reason for hiding this comment

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

This should be applied directly in the JSX component, no?

Copy link
Contributor Author

@KirankumarAmbati KirankumarAmbati Feb 5, 2024

Choose a reason for hiding this comment

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

I tried adding the prop into useSwitch.getInputProps but useSwitch is being used by Radio, CheckBox along with Switch component. So, added it in additionalProps

Copy link
Member

Choose a reason for hiding this comment

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

This should be returned by the hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useSwitch hook is being consumed by Radio and Checkbox components, along with the Switch component. Adding role = switch to the hook returned value breaks those components. To make the hook work, we might have send another parameter to useSwitch, which I felt isnt an ideal solution. We have additionalProps, which came handy for the fix.

But, I'm open for suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

useSwitch hook is being consumed by Radio and Checkbox components,

This is a bit unfortunate (and temporary). I'd make the hook return the correct set of props for the Switch and override them if needed in Checkbox and Radio button.
We are going to add Checkbox and Radio Button to Base UI in the next batch of components (likely late Q2 or Q3 this year), so it won't be an issue anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for the update @michaldudak! Made changes as per the discussion

packages/mui-joy/src/Checkbox/Checkbox.tsx Outdated Show resolved Hide resolved
packages/mui-joy/src/Radio/Radio.tsx Outdated Show resolved Hide resolved
KirankumarAmbati and others added 2 commits February 9, 2024 19:04
Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Signed-off-by: Kirankumar Ambati <kiran.chinna12520@gmail.com>
Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Signed-off-by: Kirankumar Ambati <kiran.chinna12520@gmail.com>
@KirankumarAmbati
Copy link
Contributor Author

@michaldudak the build step test_e2e_website is failing but sems like unrelated to the PR. Please can you verify & retrigger the step, if that fixes that issue. Thanks!

@KirankumarAmbati
Copy link
Contributor Author

@michaldudak Triggered an empty build and pipeline is green now :)

@danilo-leal danilo-leal added the component: switch This is the name of the generic UI component, not the React module! label Feb 12, 2024
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.

Looks good! Please write a summary of changes in the description of this PR so it's easier to find in the future.

@KirankumarAmbati
Copy link
Contributor Author

Looks good! Please write a summary of changes in the description of this PR so it's easier to find in the future.

Done @michaldudak 👍

@michaldudak michaldudak changed the title [base-ui][Switch] Missing accessibility features [base-ui][Switch] Add missing role attribute Feb 14, 2024
@michaldudak michaldudak merged commit fb3dfb0 into mui:master Feb 14, 2024
20 checks passed
@michaldudak
Copy link
Member

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: switch This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[base-ui][Switch] Missing accessibility features
5 participants