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

More autocompletion accessibility fixes #16526

Merged
merged 2 commits into from
Apr 14, 2023
Merged

Conversation

sergiou87
Copy link
Member

Closes https://github.com/github/accessibility-audits/issues/3287
Closes https://github.com/github/accessibility-audits/issues/3246
Closes https://github.com/github/accessibility-audits/issues/3244

Description

This work is a follow up of #16335 to address some accessibility issues that were reported after the PR was merged:

  • The Co-Authors label right before the input should be a label element instead of a div.
  • On Windows, NVDA wouldn't read the aria-live region with the number of suggestions the first time the autocomplete list is shown. I'd argue this is NVDA not behaving correctly, but the workaround was clean and easy enough: just move the aria-live region out of the auto-complete list and make it a sibling of the input, so it's always there.

Release notes

Notes: [Fixed] NVDA reads number of suggestions when an autocompletion list shows up

This hopefully fixes a glitch with NVDA which doesn't read the aria-live content the first time the element is added.
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Just a small question

@@ -207,6 +211,7 @@ export class AuthorInput extends React.Component<
private renderAuthors() {
return (
<div
id="added-authors"
className="added-author-container"
ref={this.authorContainerRef}
aria-labelledby="author-input-label"
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't having a label with for mean that this is redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

You would think so, but VoiceOver didn't read it, and I couldn't set a label without its for or otherwise we get a linter error 😞

Copy link
Member

Choose a reason for hiding this comment

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

🤦

@sergiou87 sergiou87 merged commit ebf4fcb into development Apr 14, 2023
@sergiou87 sergiou87 deleted the more-co-author-fixes branch April 14, 2023 13:09
Copy link

@Sindhuja365 Sindhuja365 left a comment

Choose a reason for hiding this comment

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

Approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants