search

patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
772 stars 350 forks source link

Let's get stricter about (not) usage of properties that help accessibilty #5397

Open KKoukiou opened 3 years ago

KKoukiou commented 3 years ago

Very often there are instructions to use aria-label or similar accessibility attributes as part of the component properties. My feeling is that one can very easily miss them.

See the button example: https://www.patternfly.org/v4/components/button#button

In the property section it exposes the aria-label, which is actually not necessary for buttons having string type children, but rather for icons buttons it is a must.

For most of the consumers I would bet this is not clear at all, in Cockpit we have most of the icons buttons not accessible in fact, because we just were not so familiar with the accessibility rules 1 year ago. So how about going over all these accessibility related properties, and throw some warnings/errors if they are not used when they should?

CC @evwilkin @nicolethoen @tlabaj

We should do an audit of our components (listed below) and determined whether:

Components:

tlabaj commented 3 years ago

cc @jessiehuff

jessiehuff commented 3 years ago

This is definitely something that's been considered and talked about before. I think it definitely can be difficult to look at a component and understand what accessibility attributes should be added to make that component accessible depending on your use case. It's been something in our documentation that I've been wanting to update and improve for awhile, but it can be difficult to know the right way to do it that won't be overwhelming or confusing.

Do you have any ideas of how we can make this clearer in the documentation? I've considered suggesting an Accessibility Tab to the component documentation where we could include an accessibility table of the different properties and attributes that can or should be added depending on your use case. However, when put into practice, others were worried that it was rather confusing. What are your thoughts on something like that? Would that help in this situation?

In terms of warnings, I know there's been hesitation about that in the past. Technically we haven't made accessibility required in the past so that one can be debatable about whether products would find it helpful or frustrating. I also wonder the proper way to handle that. For example, in your situation, if you're using a button, would you get a warning that lets you know about the times/use cases when you would need an aria-label regardless? Would there be some way we'd be able to determine the way products are using it to give that warning? I imagine that if someone is using a button with visible text and they got that warning, it might confuse them. They might even add an aria-label to a button that already has visible text and unknowingly override the text. I'm not opposed to the idea of warnings if we put a good system in place though and products find it helpful! 🙂

Definitely interested in others' opinions here!

KKoukiou commented 3 years ago

@jessiehuff thanks for taking some time for this! I was more thinking of checking the properties passed to check if they are sufficient for accessibility; some components partially support such errors already! See https://github.com/patternfly/patternfly-react/blob/master/packages/react-core/src/components/TextInput/TextInput.tsx#L88

So I would stick to this pattern and make sure that all components are correctly consumed. So in this case of the progress bar, I would check if title is not passed, I would require the aria-label property.

For the button, if children is not of type string I would require aria-label again. And this can be extended to all components but not in a generic way of course.