-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[a11y] Search: Search Icon Label Logic Update #13648
Conversation
With the currently implementation, an accessibility violation occurs when the Search Icon's container doesn't have a role. The current implementation removes the `role` attribute when the `onExpand` property is undefined, but it should also remove the `aria-labelledby`. This update will avoid the accessibility violation: https://www.w3.org/TR/wai-aria-1.1/#roles --- The error can be see in the `@carbon/react` Storybook UI. Visit the [Search component](https://react.carbondesignsystem.com/?path=/story/components-search--default) and click on the 'Accessibility' tab
DCO Assistant Lite bot All contributors have signed the DCO. |
✅ Deploy Preview for carbon-components-react ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
I have read the DCO document and I hereby sign the DCO. |
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed no longer see this violation:
@sjbeatle looks like some tests are failing other than that LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me also. Let's fix the tests and it will be ready to merge.
Thank you
Great! I'll work on that right away. Thanks for the speedy response! 😁 |
@guidari @francinelucca I think this one should be all set, now. Please let me know if there's anything else you'd like me to address. Cheers! |
packages/react/src/components/DataTable/__tests__/__snapshots__/DataTable-test.js.snap
Show resolved
Hide resolved
@sjbeatle Hey Steven just waiting on a small fix on your end and we can get this in :) |
Hello @andreancardona! Thank you, but I'm not confident that the fix is trivial. Please see my recent comment here. Thank you. |
…3648) * [a11y] Search: Search Icon Label Logic Update With the currently implementation, an accessibility violation occurs when the Search Icon's container doesn't have a role. The current implementation removes the `role` attribute when the `onExpand` property is undefined, but it should also remove the `aria-labelledby`. This update will avoid the accessibility violation: https://www.w3.org/TR/wai-aria-1.1/#roles --- The error can be see in the `@carbon/react` Storybook UI. Visit the [Search component](https://react.carbondesignsystem.com/?path=/story/components-search--default) and click on the 'Accessibility' tab * fix(tests): snapshots updated after Search a11y update --------- Co-authored-by: Francine Lucca <40550942+francinelucca@users.noreply.github.com> Co-authored-by: Alison Joseph <alison.joseph@us.ibm.com> Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
With the current implementation, an accessibility violation occurs when the Search Icon's container doesn't have a role. The current implementation removes the
role
attribute when theonExpand
property is undefined, but it should also remove thearia-labelledby
.This update will avoid the accessibility violation: https://www.w3.org/TR/wai-aria-1.1/#roles
Testing / Reviewing
The error can be see in the
@carbon/react
Storybook UI. Visit the Search component and click on the 'Accessibility' tabThanks!