fix(Card): indicate card selectivity and status if using a screen reader #7091
Conversation
|
Preview: https://patternfly-react-pr-7091.surge.sh A11y report: https://patternfly-react-pr-7091-a11y.surge.sh |
wise-king-sullyman
force-pushed
the
6798
branch
from
4a6f78d to
b2bd756
Compare
wise-king-sullyman
force-pushed
the
6798
branch
from
c51f752 to
b6a3cd6
Compare
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Looks (and sounds) good! I think the only thing is that the first Card, which is the "add card" one, is being announced as selectable when it actually isn't. Removing the isOption prop from it resolves that, but initially upon entering the listbox there doesn't seem to be any actual context for that Card other than "You are currently on an article."
Using aria-describedby on that card and linking it to its <Title> element kind of works, but still a bit iffy. Going to see if there's anything I could come across tomorrow, or whether anyone else has an idea.
|
@thatblindgeye looks like having non options in a listbox isn't allowed. I think either the card-adding card will need to be an option or the actually selectable cards will need to be placed in a separate div to serve as the listbox, and I don't like either of those ideas. Any thoughts for a better approach than what I've come up with? |
The second option is the best of the two, but based on how this demo is meant to be yeah neither option is really ideal. Depending on the feedback from anyone else, perhaps the best option right now is to have all those cards as |
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
I agree with @thatblindgeye
Adding back the option role to the first card is probably the closest we can get it to ideal without more dramatically altering the demo structure - which we'd want to do first in Core i think.
Once that and my note about the example including information about 'listbox', i'll approve.
|
|
||
| return ( | ||
| <React.Fragment> | ||
| <div role="listbox" aria-label="Selectable cards"> |
There was a problem hiding this comment.
I think that the example should include one sentence above it that describes why you needed to add the role 'listbox' to the div surrounding it.
| Note: The listbox role on the containing div of this examples is needed because the cards have the option role via their `isOption` prop, and options are required to be inside a listbox. See the [option role documentation](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/option_role) from MDN for more information. | ||
|
|
||
| The cards need to be options in order to effectively communicate that they're selectable (and what their current selection state is) for those using screen readers. | ||
|
|
There was a problem hiding this comment.
Nit: The final line could maybe be updated from "for those using screen readers" to "for those using assistive technologies (such as screen readers)." Train of thought is mainly inclusivity.
There was a problem hiding this comment.
Good callout 🔥
There was a problem hiding this comment.
This looks pretty good - definitely an improvement! The only odd thing that I noticed is that I'm not sure how to use the kebab anymore inside of the first card with VO. I don't seem to be able to get to it. @thatblindgeye or @nicolethoen can you check to see if you're able to get to that first card's action menu by VO?
|
@jessiehuff Hmm yeah I was unable to get to it as well. When VO was active, when I clicked the kebab with my mouse I saw focus briefly on the kebab but then it snapped focus back to the Card itself. |
|
@jessiehuff @thatblindgeye with VO active I can get to the kebab by hitting tab once while focused on a card, is that acceptable? |
|
One issue with that is whether a user using VO would know there's a kebab to Tab to; if they primarily use the VO + arrow keys to navigate, there may be no indication of a kebab. That said, I'm wondering if that would be considered a separate issue from this, as per the original issue it does look resolved (as best as can be at this moment), or if the inability to use VO navigation to get to the kebab would block this update. |
|
Closing as it was decided that this would not be the path forward for this issue. |
5 participants
What: Closes #6798
Additional issues:
(I believe) I've implemented all of Nicole's suggestions from the issue and applied them to the card view demo.
If everything looks like it works well enough I'll update the other places Card is being used to the same pattern used here, then convert this to a real PR.Direct link to the card view demo for convenience: https://patternfly-react-pr-7091.surge.sh/demos/card-view/react-demos/card-view/