[PM-2432] Desktop: Modify switch account dropdown/dialog for accessibility #5529
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
- if it opens a dialog, it should advertise this with `aria-haspopup="dialog"`, not `aria-haspopup="menu"` - if it opens a dialog, the `aria-expanded` is pointless (as the user will never get back out into the underlying page to check if it's expanded or collapsed, since it's for a dialog not a disclosure widget or menu)
as it's a dialog that opens, not a disclosure, this is irrelevant
just use existing single button, but add visually hidden extra "Switch account" to accName
750742b to
24e413a
Compare
|
Thank you for your contribution! We've added this to our internal Community PR board for review. |
24e413a to
98e73b7
Compare
* Take out the confusing "Logged in as..." a11y text * Use visible button text (with a few extra `sr-only` parts, for readability) as the button's accName * Add the "Switch account" context to each of the buttons to make clear what they do
98e73b7 to
1fb80ac
Compare
rr-bw
approved these changes
Oct 11, 2023
shane-melton
approved these changes
Oct 23, 2023
|
QA approved, thanks @patrickhlauke! |
BlackDex
pushed a commit
to BlackDex/bitwarden-clients
that referenced
this pull request
Nov 21, 2023
…ility (bitwarden#5529) * Tweak account switcher button - if it opens a dialog, it should advertise this with `aria-haspopup="dialog"`, not `aria-haspopup="menu"` - if it opens a dialog, the `aria-expanded` is pointless (as the user will never get back out into the underlying page to check if it's expanded or collapsed, since it's for a dialog not a disclosure widget or menu) * Make two variants for button to sort out `aria-label` on logged-in case * Remove `aria-controls` for button as it's a dialog that opens, not a disclosure, this is irrelevant * Fix `overlayPostition` typo * Simplify approach just use existing single button, but add visually hidden extra "Switch account" to accName * Tweak account switch buttons in dialog/dropdown * Take out the confusing "Logged in as..." a11y text * Use visible button text (with a few extra `sr-only` parts, for readability) as the button's accName * Add the "Switch account" context to each of the buttons to make clear what they do
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.
Type of change
Objective
Correct confusing/incomplete switch account menu button and the account dropdown/dialog to actually make sense when using a screen reader. Closes #5525
When already in an account (the menu button shows the current email address), the button is now announced as "[current email address] (Switch account)". The individual account buttons in the dropdown/modal are now announced as "Switch account: [email address] (Locked)".
Code changes
aria-controlsandaria-expanded, as they make no sense for a button that opens a dialog (rather than a disclosure widget). for same reason, changearia-haspopup="menu"toaria-haspopup="dialog"(as it opens a dialog, not a menu). add "(Switch account)" to the accessible name of the button (when already using an account). corrects the accessible name for the buttons to switch to other accounts: simply uses the visible text (with a few minorsr-onlyadditions for readability), instead of the "Logged in as ..." (which currently also doesn't convey locked/unlocked status)Screenshots
Video of Bitwarden on Windows, with NVDA running. Note the voice output, compared to the video in #5525
bitwarden-issue5525-fix.mp4
Before you submit