Accessibility: Improve HelpModal markup #83171
Conversation
|
Should we write/design something that makes it easier to understand if you should press the keys at the same time or in order? |
tskarhed
requested review from
Clarity-89 and
JoaoSilvaGrafana
and removed request for
a team
tskarhed
changed the title
| <span className={styles.shortcutTableKey}> | ||
| <Text variant="code">{children}</Text> | ||
| </span> |
There was a problem hiding this comment.
I tried using a combination of aria-hidden and aria-label here to get the screen reader to say h keyetc, but it always appended with empty group. I am looking into if it is something I can fix.
There was a problem hiding this comment.
I don't think it is a big deal to announce key. I think it is pretty obvious in context that it is reading out the keyboard keys that need to be hit. I do have a note in the other comment about the keys that need to be hit together, though.
There was a problem hiding this comment.
To be honest, I'm not that precious about this but I'd probably say I'd rather have a table over the Definition List. Two main reasons why:
- Tables actually work really well with screen readers - in particular that you can easily navigate up and down columns which I think would be pretty desirable when as a user you want to see what shortcuts are available, then look at what the shortcut is if you want to use it (for this reason I'd also think about changing the design and put the command first then the keys second -- again minor and not sure if design will sign off on that change or not but all the other sites I've spot-checked do it this way.)
I should have made this clearer in the filed issue but the current implementation's main problem is the way the table header is implemented. Neither columns have their own header, just a shared one that spans two columns, so it can be disorientating for screen reader users when entering the table.
- I think the screen reader implementation for definition lists is pretty annoying. There is clearly 8 commands which you have marked up correctly but the screen reader reads it as "16 items", as it considers each definition term and definition description as an item and doesn't think they are related 🤷 You have to go through all of the commands and keys associated before finding out what the next command is.
There was a discussion about separation the keys with modifiers, but I don't think we should do it, as it highlights that they should be pressed at the same time as opposed to in succession.
I never noticed that they could be hit one after the other until this comment, I wasn't sure what you meant until I tested it 😅 So on this note, the problem with them listed like this is they are treated separately with VoiceOver (when I tested with NVDA they are read together, though, so it is platform specific -- might also be because of the settings I use for my screen readers).
You could go the route of rendering a grouped text version that is hidden visually (e.g. className="sr-only") and putting aria-hidden on the visible keys so they aren't read out twice.
Lastly -- for screen reader users Esc is read out as e - s - c rather than escape, (same with c - m - d). The few keys that are capitals also aren't clear to screen reader users. It might be worth specifically mentioning it is d shift + c as an example.
| {category} | ||
| </Text> | ||
| </div> | ||
| <dl className={styles.keysAndDescriptions}> |
There was a problem hiding this comment.
You could add an aria-labelledby to this and point at the h3 but it's not that big of a deal.
There was a problem hiding this comment.
Using a caption for the table
| const Key = ({ children }: KeyProps) => { | ||
| const styles = useStyles2(getStyles); | ||
| return ( | ||
| <span className={styles.shortcutTableKey}> |
There was a problem hiding this comment.
Screen readers don't recognise it so this is also very minor but you could change the element to <kbd> here.
As a side note for another day, I'd actually love if this Key component was available in @grafana/ui as I've had to reimplement it in some plugins 😅
There was a problem hiding this comment.
Could you link to/tell me whichever those plugins are?
| <span className={styles.shortcutTableKey}> | ||
| <Text variant="code">{children}</Text> | ||
| </span> |
There was a problem hiding this comment.
I don't think it is a big deal to announce key. I think it is pretty obvious in context that it is reading out the keyboard keys that need to be hit. I do have a note in the other comment about the keys that need to be hit together, though.
|
I added specific characters for command and shift. I am doing sr-only for ctrl and esc. Writing the whole word seems a bit excessive, especially since there is a convention around it. |
|
/deploy-to-hg |
|
|
|
One little trick that might help the layout look a bit better would be to reorder the sections. So I'd also change them to all to sentence case ( |
|
Having dashboard shortcuts at the bottom really makes it better. I just want a final signoff from @ckbedwell |
There was a problem hiding this comment.
Left one minor comment about the srOnly implementation to make it meet the official standard.
I also noticed that when I am zoomed in a bit I can't scroll up and down the content using just my keyboard. @eledobleefe looked at this problem recently and we agreed the best solution is to get modal authors to add tabIndex={0} to content containers (in this case the Grid component in the HelpModal) with no focusable elements.
One last thing I want to highlight so that if this ever gets flagged in a future audit we can say we are aware of it and have chosen to ignore this best practice (or maybe add it to be thought about in the future for these kind of designs? cc @amy-super).
So it is best practice that when using a table, that the table headers are visible. (Quick side node -- the way our .sr-only class is written Deque's automated dev tools think it is visible...). As previously discussed, this design could be implemented as a definition list, or just without table markup at all but we've chosen not to do that because we believe the experience is worse for users that way.
|
|
||
| return key.replace( | ||
| displayName, | ||
| `<span aria-label="${srName}"><span aria-hidden="true" role="none">${displayName}</span></span>` |
There was a problem hiding this comment.
TIL: that in the official specification that aria-label is only meant to be used for interactive elements.
So really you want to change this to <span className="sr-only">${srName}</span><span aria-hidden="true" >${displayName}</span>.
There is an annoying trade-off doing it this way which in the grand scheme of things is very minor and wouldn't block me approving this PR but worth knowing and Microsoft flagged it up with us in their audit: the screen reader cursor is no longer representative of the actual element that is being 'focussed'. You have to do a fair amount of jiggery pokery to get it to line up with non-interactive elements.
|
Okay, I've done this:
From my understanding, you are okay with the table headers not showing. |
What is this feature?
Removes the table in the HelpModal to make it more accessible for screen readers. With the help of the
Textcomponent, the contrast issue for the light theme is addressed. Also made it readable for extra small devices, because why not?Before:
After:
Why do we need this feature?
It is very heard to read for screenreaders.
Who is this feature for?
Screen reader users
Which issue(s) does this PR fix?:
Fixes #79516
Special notes for your reviewer:
I used a definition list and tried it with the screen reader, but I am not convinced. It did work well with the CSS grid though.
There was a discussion about separation the keys with modifiers, but I don't think we should do it, as it highlights that they should be pressed at the same time as opposed to in succession.
Please check that: