codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
26 stars 25 forks source link

Improve accessibility of labels #448

Closed milofultz closed 9 months ago

milofultz commented 9 months ago

Improve accessibility of labels

Resolves no existing Github issue, only minor fixes. First PR, so let me know if I should be making issues before I make a small PR like this to follow protocol.

1. Removes unnecessary aria-labels. 2. Improves existing aria-labels. 3. Use standard capitalization instead of all caps and rely on styling for capitalization.

The files this PR effects:

Components

Tests

Other Files

Screenshots (if applicable):

No visual changes.

xscottxbrownx commented 9 months ago

@milofultz I'm still learning here, and wanted to ask you about when a component has multiple buttons how do we test now without aria-labels? Seems you've been using getByRole('button') and curious how do we do this when multiple buttons.

milofultz commented 9 months ago

@milofultz I'm still learning here, and wanted to ask you about when a component has multiple buttons how do we test now without aria-labels? Seems you've been using getByRole('button') and curious how do we do this when multiple buttons.

Hey @xscottxbrownx , what I have done in the past has been use other parts of the element that are less prone to change. For instance, maybe the overall structure of the tested component remains the same (e.g. there is always one button), or the accessible name associated with it is fairly static (e.g. a button contains text "submit").

Let's say we have a login field with username and password fields. To get the username field, I could use one of these methods:

The former is good because you can use the accessible name, which aids in accessibility and doesn't rely solely on an aria-label. YOu can see how it's calculated below the "Terminology" section in this doc: https://www.w3.org/TR/accname-1.2/#mapping_additional_nd_te

I sometimes use the latter if the structure is almost assuredly unchanging, but I should probably use the former in those cases anyway.

Let me know if I can add more clarity here. Thank you!

xscottxbrownx commented 9 months ago

Hey @xscottxbrownx , what I have done in the past has been use other parts of the element that are less prone to change. For instance, maybe the overall structure of the tested component remains the same (e.g. there is always one button), or the accessible name associated with it is fairly static (e.g. a button contains text "submit").

Let's say we have a login field with username and password fields. To get the username field, I could use one of these methods:

So in your example of a button with the text submit, is that what you put as name? Like: getByRole('button', {name: 'submit'})


And in your 3 in the PR description... are you saying that the screen readers/assistive technology will read what we have in our codebase vs the styling?
Example: you changed CANCEL button text to Cancel and said if we wanted to all-caps it, to do so through CSS basically. Is this just a standard/best-practice, or having all-caps in the codebase causes issues?

xscottxbrownx commented 9 months ago

This all looks good. I'm just asking questions to better my understanding and so when we write more tests (tons of components are missing test), I don't need to rely on checking for aria-label of elements.

bingeboy commented 9 months ago

Is there an accessibility plugin you recommend to verify these types of changes? I've look at the code and the changes seem good to me but I'm not an accessibility expert. I did a little research and read a few posts about why these aria-labels shouldn't be needed. Very informative. 👍

I use to use Wave but it's been a few years :)

andycwilliams commented 9 months ago

Is there an accessibility plugin you recommend to verify these types of changes? I've look at the code and the changes seem good to me but I'm not an accessibility expert. I did a little research and read a few posts about why these aria-labels shouldn't be needed. Very informative. 👍

I'm not familiar with any but I'd be surprised if there weren't any.

Could you link to these posts please?

bingeboy commented 9 months ago

@andycwilliams https://wave.webaim.org/ and https://www.freecodecamp.org/news/web-accessibility-common-aria-mistakes-to-avoid/

milofultz commented 9 months ago

So in your example of a button with the text submit, is that what you put as name? Like: getByRole('button', {name: 'submit'})

And in your 3 in the PR description... are you saying that the screen readers/assistive technology will read what we have in our codebase vs the styling? Example: you changed CANCEL button text to Cancel and said if we wanted to all-caps it, to do so through CSS basically. Is this just a standard/best-practice, or having all-caps in the codebase causes issues?

Yeah, the example you would use submit or any text match object, so regex would work, too, like getByRole('button', {name: /submit/i}), if you wanted to make sure it matched without case sensitivity.

For the capitalization, there isn't any WCAG guidance on it, but generally, screen readers can interpret all capital series of letters as acronyms and CSS is essentially always ignored. So "SUBMIT" in the HTML could be read as "ESS EWE BEE EM EYE TEE" instead of "submit". Using <...>Submit</...> with text-transform: uppercase (which I think MUI is already doing by default) will result in a better user experience.

milofultz commented 9 months ago

Is there an accessibility plugin you recommend to verify these types of changes? I've look at the code and the changes seem good to me but I'm not an accessibility expert. I did a little research and read a few posts about why these aria-labels shouldn't be needed. Very informative. 👍

I use to use Wave but it's been a few years :)

The easiest tool to set up and use for the most basic stuff (certainly not all accessibility issues) is Microsoft Accessibility Insights. It's Chrome/Edge only, unfortunately, but it is very good at quick passes for the super low-hanging fruit.

For verification and other more nuanced accessibility stuff, there are essentially no tools that can fully verify anything due to a variety of factors. If any tools say they can, they are lying (see accessibility overlays). Things like Deque's axe (which can be integrated into Jest and Vitest, but I haven't tested it yet) are a great way to stop common regressions and find slightly more complex issues. But at the end of the day, it's just becoming familiar with the dialect of assistive technologies and their quirks.

milofultz commented 9 months ago

In general, re: when aria-label is/isn't needed, check out how accessible names are resolved and use the Chrome/Firefox Devtools's Accessibility pane to see how it's calculated.

Firefox example of the "Update branch" button's accessible name:

Screen Shot 2023-10-12 at 8 53 48 AM

Chrome example of the "Sign up" button (note that it's actually a link:

Screen Shot 2023-10-12 at 8 57 08 AM

Chrome's gives a little more info on how it calculates, which is great.

milofultz commented 9 months ago

If taking out the label in Home.jsx, should adjust the component it was being passed to HomeSection.jsx.

And maybe switch the {title.toUppercase()} to {title} in ConfirmationButton.jsx, as it does seem you are correct - MUI seems to have default uppercase styling on buttons within modals.

Got them in there. Thanks!