Skip to content
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

fix: a11y omnibus package #2819

Merged
merged 29 commits into from
Jun 20, 2024
Merged

fix: a11y omnibus package #2819

merged 29 commits into from
Jun 20, 2024

Conversation

werdnanoslen
Copy link
Contributor

@werdnanoslen werdnanoslen commented Mar 12, 2024

Summary

Fixes a bunch of a11y issues that Playwright surfaced (322 of 553 tests failed):

Source changes:

  • Required aria-hidden or aria-label or aria-labelledby on all Icons, defaulting to the former being true in storybook. Removed part of a test to this end.
  • Changed CardHeader element from header to div to avoid a11y issues that Storybook helpfully demonstrated, and it's how USWDS implements it anyway (probably for the same reason). This might be breaking in case anyone used css to target the rendered semantic header element. If so, please mark as breaking.
  • Hid Pagination, SocialLinks, and SearchButton's decorative svgs from screen readers
  • Replaced presentation role with aria-label to fix a11y issue, and per USWDS implementation
  • Used aria-controls on LanguageSelector only if specified, and for this component only uses it when appropriate

Storybook-only changes:

  • Added labels to all DatePicker, InputPrefix, InputSufffix, RangeInput, Select, Textarea, and TextInput stories to fix a11y warnings there, and tidied up a bit
  • Changed Tag story colors to meet contrast requirements
  • Hid IconList icons from screen readers
  • Changed Footer's h3s to ps since class already handled styling and use of a heading seemed unwarranted
  • Tidied Pagination's stories
  • Disabled 'skip-link' rule for ExtendedNav due to false-positive in Storybook

Related Issues or PRs

Resolves #2811
Resolves #2809

How To Test

If you have Playwright installed locally, run yarn test-storybook. Otherwise, check the Accessibility tab of all the stories

Screenshots (optional)

@werdnanoslen werdnanoslen linked an issue Mar 12, 2024 that may be closed by this pull request
@werdnanoslen werdnanoslen changed the title fix: Miscellaneous a11y fixes fix: a11y omnibus package Mar 15, 2024
@werdnanoslen werdnanoslen marked this pull request as ready for review March 15, 2024 03:54
@werdnanoslen werdnanoslen requested review from a team as code owners March 15, 2024 03:54
@werdnanoslen werdnanoslen changed the base branch from an-storybooka11y to main March 15, 2024 03:58
@werdnanoslen werdnanoslen added the a11y Relates to accessibility standards and practices label Mar 15, 2024
@werdnanoslen
Copy link
Contributor Author

werdnanoslen commented Mar 15, 2024

FYI everything is passing checks except for the a11y CI step I added, because it can't run here 🙃 Sooo, maybe we shouldn't run this as a storybook test runner, since it requires a deployed storybook (sorta like how USWDS does it on their PRs). Thoughts on whether to merge this as just a11y updates, with an alternative CI for a11y, or can we still make this work?

Nvm, I booted it to another branch/PR

@@ -20,9 +20,9 @@ Source: https://designsystem.digital.gov/components/tag/
export const DefaultTag = (): React.ReactElement => <Tag>My Tag</Tag>

export const CustomBg = (): React.ReactElement => (
<Tag background="#e44608">My Tag</Tag>
<Tag background="#d83933">My Tag</Tag>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally a nit, but this could be a different color (that's also accessible of course). Right now this is the same as bg-secondary so there's no visual difference looking at the CustomBg and CustomClass stories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember doing that on purpose, but I forget why 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think because it looked like bg-secondary, but it wasn't, so I was trying to keep it within the palette

@kleinschmidtj
Copy link
Contributor

yarn test-storybook

@werdnanoslen I looked at the code and it looks good to me. I just wanted to run this testing command, but I don't see it the command in package.json. Is this new? Should this have been added as a new script?

@werdnanoslen
Copy link
Contributor Author

yarn test-storybook

@werdnanoslen I looked at the code and it looks good to me. I just wanted to run this testing command, but I don't see it the command in package.json. Is this new? Should this have been added as a new script?

oh whoops, I'll remove it. I started this ticket by seeing if we should run playwright as a GH task, but I didn't figure out how and pivoted to just fixing the issues it found from running locally.

Copy link
Contributor

@kleinschmidtj kleinschmidtj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@werdnanoslen werdnanoslen merged commit 758cd74 into main Jun 20, 2024
8 checks passed
@werdnanoslen werdnanoslen deleted the an-storybooka11y-fixes branch June 20, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Relates to accessibility standards and practices
Projects
None yet
2 participants