-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Improve accessibility of AutocompletingTextInput
component
#16324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @sergiou87. The one thing that feels weird to me here is that you have to hit arrow-down to autocomplete to the first match, i.e. no more typing @serg
and hitting Tab to complete which is the convention that I'm most familiar with (in fact I can't think of an autocomplete that doesn't work like that). Could we make it autocomplete the top hit on Tab/Enter even if nothing is selected or would that make it fail the a11y criteria?
Something like
diff --git a/app/src/ui/autocompletion/autocompleting-text-input.tsx b/app/src/ui/autocompletion/autocompleting-text-input.tsx
index fa3f5003d..c6b5fadc9 100644
--- a/app/src/ui/autocompletion/autocompleting-text-input.tsx
+++ b/app/src/ui/autocompletion/autocompleting-text-input.tsx
@@ -520,7 +520,10 @@ export abstract class AutocompletingTextInput<
event.key === 'Enter' ||
(event.key === 'Tab' && !event.shiftKey)
) {
- const item = currentAutoCompletionState.selectedItem
+ const item =
+ currentAutoCompletionState.selectedItem ??
+ currentAutoCompletionState.items.at(0)
+
if (item) {
event.preventDefault()
Also, while technically out of scope for this work I noticed how obnoxious it is hearing that first 'colon' announced on each emoji. It seems dotcom just strips the leading and trailing :
when autocompleting emojis. Could we do the same?
I share that sentiment, but as far as I understand it fails the a11y criteria because a screen reader user wouldn't know what they'd be autocompleting before hitting the tab/enter key. I also considered selecting the first item and making the screen reader read it, but then it's like moving the focus out of the input text box, which is where the screen reader user is expecting to be.
Sounds good! |
@niik I've improved readability of the emoji autocomplete list as you suggested, and I also improved readability of the issue autocompletion elements by adding a space between the issue number and the title (before this, the screen reader would read both the number and the first word of the title as if they were one). I also used the secondary text color for the issue number, which is what dotcom seems to do too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ Thank you for all your work on this.
Description
This PR improves accessibility of the
AutocompletingTextInput
with these changes:aria-live
hidden component with the number of suggestionsI have more changes from the co-author-accessibility-fixes branch that are much more specific to the Co-Authors input, so I will leave those for another PR.
Screenshots
CleanShot.2023-03-15.at.16.38.07.mp4
Release notes
Notes: [Improved] Make autocompletion suggestions more accessible for screen readers.