-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
accessibility: improve screen reader announcements for emoji reaction picker buttons #7922
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
Open
UlisesMilani
wants to merge
2
commits into
signalapp:main
Choose a base branch
from
UlisesMilani:accessibility-reactions
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+15
−9
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
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.
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.
We actually have an API that does a better job localizing the emoji name:
I think we can get rid of the
titleattribute. The aria guide for toggle buttons specify that you shouldn't change the label based on the state:https://www.w3.org/WAI/ARIA/apg/patterns/button/
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.
Thanks for the review, @jamiebuilds-signal!
I tested using
Emoji.getDisplayLabel(emoji)locally with screen readers (NVDA/JAWS), but the returned text labels are very simplified compared to delegating the raw Unicode character. For example, for the crying face emoji it announces just "llorar" (cry) instead of the full descriptive name "cara llorando", and for the red heart emoji it just says "corazón" (heart) instead of "corazón rojo" (red heart).There is no need to worry about adding translation strings in our codebase for this: by passing the raw Unicode emoji character as the
aria-label, the browser/OS delegates the translation directly to the screen reader's native CLDR engine. The screen reader automatically translates and pronounces the emoji based on the active system language of the user. I verified this behavior across multiple languages (including English, Spanish, French, Portuguese, and Italian) and it works flawlessly, sounding much more natural in every single one of them.Therefore, I've updated the full emoji picker cells (in the "More" panel) to use the raw emoji Unicode character as well, so it matches the suggested reactions' behavior and gets the correct full screen reader name.
You are completely right. I've respected the W3C ARIA toggle button guidelines and simplified the button labels to be static (always the emoji itself), letting
aria-pressed={isSelected}convey the selection/active state instead of changing the label dynamically.I also added a check so that the long-press skin tone description ("press and hold to select skin tone") is only announced for emojis that actually support skin tone variants.
I've pushed these updates to this branch. Let me know if you have any further feedback!
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.
Yeah, there are definitely many cases where the official unicode title is a better accessible description of the emoji, although there are also cases where the official title doesn't really match how emojis get used by people or are wordy to the point of being disruptive in the middle of text.
There is also a gap between the languages supported by screen readers (and operating systems or the Unicode CLDR) and the languages Signal supports. Which you aren't going to see as much only looking at very broadly supported locales like English/Spanish/French/etc). Language support can also vary on different operating systems, and where possible we try to rely on our own translations because the app locale can differ from the system locale.
I am going to bring it up with our localization team to see if we could review and get accessible descriptions for every emoji. That is gonna take some time though.
In the mean time, I wonder if it would be an improvement to use the short name from
Emoji.getDisplayLabel()as thearia-labeland provide the emoji inaria-descriptionto fallback to whatever Unicode name is available.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.
Ha, apparently our localization team was already working on getting translations together for the CLDR annotations dataset.