Fix accessibility bug of missing accessible name on Dialog #2210
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 4a59f8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
khiga8
force-pushed
the
kh-add-missing-label
branch
from
6b1a1b9
to
ec170fb
Compare
khiga8
force-pushed
the
kh-add-missing-label
branch
from
63e711a
to
867fe44
Compare
TylerJDev
approved these changes
Aug 16, 2023
jonrohan
approved these changes
Aug 16, 2023
|
I am not authorized so please merge in, thank you! |
keithamus
approved these changes
Aug 17, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
When I was doing unrelated work in dotcom, I noticed that
Primer::Alpha::Dialogis not rendering with an accessible name resulting in an Axe failure of aria-dialog-name:This is unexpected since having an accessible name is also clearly documented in the docs.
There's currently no corresponding behavioral test written for this component. It's likely we're not running an axe scan when the Dialog is an open state, causing us to miss this error.
This PR fixes the bug and adds a behavioral system test.
List the issues that this change affects.
Closes https://github.com/github/primer/issues/2556
Risk Assessment
What approach did you choose and why?
Fix bug and add missing behavioral test.
Accessibility
Merge checklist
Questions to reviewers
📣 Are behavioral components (or components that render with JS) identified in any way? I couldn't find anything that would indicate a component has JS associated with it. I was thinking it might be worth having a check that ensures all behavioral components have a corresponding
system_testwritten.This is beneficial because it would ensure that we have checks in place for how a component should behave, and would also make sure that axe is ran against various states of the component (e.g. when dialog is open). This is also extra important because we don't seem to have custom element tests. This would help surface more accessibility or behavioral issues at the testing stage.