-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(material/chips): Example violates axe accessibility tests #32679
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: main
Are you sure you want to change the base?
Conversation
tjshiu
left a comment
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.
So I like the change overall - but I do have a bit of a nit. The input is always on a new line here. Can we adjust it so that it isn't?
An alternative solution, but it would take more work: Add the role row and gridcell here and wrap the input. But I think it might not work because we wouldn't have the keyboard a11y for it (e.g. would still need to be able to arrow to the input). It would also pass the a11y checks but would be an issue for keyboard users since they are treated as separate tab stops.
|
I also dislike that the input is on a new line -- that was the exact functionality that led me to this example and to this bug. But that seems to be a fundamental limitation of the component. I do not think code listed as example code should do funky things with roles to try to get a11y to work -- it should either be supported by the component or it shouldn't, and it's not, so I'm fixing the example to reflect that. |
|
Deployed dev-app for 51c17d7 to: https://ng-dev-previews-comp--pr-angular-components-32679-dev-4g4jwctd.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
|
Deployed docs-preview for 51c17d7 to: https://ng-dev-previews-comp--pr-angular-components-32679-docs-r9dwwmwc.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
adolgachev
left a comment
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.
I think this change is fine as the goal here is to not have an example which fails the accessibility check and shows that something can not be actually done (without failing the checks).
We can explore doing a better example which does the right thing at a later point.
See b/475597223 for context as well.
|
Check the lint error: use material/chips for the PR and commit. |
Fixes #28067