-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[SvgIcon] Remove wrong role #20307
[SvgIcon] Remove wrong role #20307
Conversation
I wonder if we shouldn't introduce https://github.com/validator/validator in our CI. |
Details of bundle changes.Comparing: 3d01aa0...a31a040 Details of page changes
|
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.
That one has annoyed me a lot. Good first step though it's for validation purposes only. The element will be removed from the a11y tree anyway if titleAccess
is truthy. We should also rethink the aria-hidden at some point (some tools will forcefully remove aria-hidden elements from the tree for visually impaired. This makes the UI less usable if you have icon-only buttons).
I wonder if we shouldn't introduce validator/validator in our CI.
We can test it, sure.
The problem with these tools it that they are intended for HTML that is authored i.e. something you wrote manually. For libraries they oftentimes produce a lot of false alarms e.g. referencing a non-existent element in an IDREF attribute (such as aria-controls) is perfectly fine. But many of these tools will throw because they assume you meant to reference an existing one.
@eps1lon Great point on the tradeoff taken by these tools. My concern is that if our components output warnings/errors, it will prevent the developers that author websites/apps to see relevant information. In sum, I would be cautious with the SNR ratio. |
Took me 30s due to message filtering. This is an essential part of these tools. If they don't have proper message filtering then they are useless because of the high false-positive/true-positive ratio (there was a proper term but I forget; accuracy? correctness?) Edit: The message filtering is especially important for library code since you likely have repeated errors rather than different ones. |
The point I want to make is that you're right that false alarms are bad. But it's better to fix these in the tools (because it helps more people) rather than the validated code. Oftentimes this would require a lot of branching from our side which makes code unreadable without fixing an actual problem. |
Definitely, I think that it can go both ways. In the case of WAVE, I agree that some of the warnings and errors are questionable. In the case of this validator, I haven't yet seen false positives. But it's not impossible, I guess we will find some if we use the tool extensively. |
The possible roles are:
https://www.w3.org/TR/html-aria/#svg
This fix:
https://validator.w3.org/nu/?doc=https%3A%2F%2Fmaterial-ui.com%2F