[pm-41938] fix: Bank Account accessibility fixes - #2985
Conversation
github-actions
Bot
added
app:password-manager
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## pm-41934-bank-account-behavior-fixes #2985 +/- ##
=======================================================================
Coverage ? 39.67%
=======================================================================
Files ? 362
Lines ? 16994
Branches ? 0
=======================================================================
Hits ? 6742
Misses ? 10252
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
92f03fd to
15f4a0d
Compare
morganzellers-bw
added
ai-review
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed the Bank Account VoiceOver batch against Code Review Details
|
15f4a0d to
a6074fb
Compare
github-actions
Bot
added
the
app:authenticator
a6074fb to
b68caa1
Compare
b68caa1 to
04c13c5
Compare
388d472 to
7543515
Compare
7543515 to
1213f77
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // any character boundary, but that same text is used as the accessibility label by | ||
| // default. Override it with the zero-width-space-free password so VoiceOver doesn't | ||
| // announce the value once from the label and again from `speechSpellsOutCharacters`. | ||
| .accessibilityLabel(isPasswordVisible ? password : hiddenPassword) |
There was a problem hiding this comment.
❓ QUESTION: Was the new accessibility label verified on the non-bank-account screens that also use PasswordText?
Details
colorCodedText(for:) inserts a zero-width space after every character, and until now that ZWSP-separated string was the implicit accessibility label for every visible PasswordText. This line replaces it with the raw value for all callers, but only the three bank-account fields opt into spellOutAccessibilityValue: true.
Callers that keep the default false and render visible values include:
ViewCardItemView.swift:47/:104— card number and security codeViewLoginItemView.swift:59— login passwordViewSSHKeyItemView.swift:32— SSH private keyViewPassportItemView.swift:141/:186,ViewDriversLicenseItemView.swift:102,ViewIdentityItemView.swift:74GeneratorView.swift:292,PasswordHistoryListView.swift:88,MasterPasswordGeneratorView.swift:48— always visible
The ZWSP separators are commonly what pushes VoiceOver into per-character reading, so a numeric value like a card number or a generated password may now be announced as a single number/word rather than digit by digit on those screens. If that regression is real, passing spellOutAccessibilityValue: true at those call sites (or defaulting it to true) would keep the previous behavior.
c75a618 to
6c6222a
Compare
71a508a to
f6c8262
Compare
…unt view toggles VoiceOver announced the Account Number, PIN, and IBAN reveal toggles on the Bank Account view screen with a generic "button" label because PasswordVisibilityButton accepted an accessibilityLabel but never applied it. Wires up field-specific announcements (e.g. "Account number is not visible, tap to show.") for each toggle.
…unt add/edit toggles VoiceOver announced the Account Number, PIN, and IBAN reveal toggles on the Bank Account add/edit screen with the same generic "Password is/is not visible" wording regardless of which field was being toggled. Adds an optional field-name parameter to BitwardenTextField so these three fields announce their own name (e.g. "PIN is visible, tap to hide.") while every other password-style field in the app keeps its existing generic wording.
…o VoiceOver VoiceOver was reading the Account Number, Routing Number, Branch Number, PIN, SWIFT Code, and IBAN values on the Bank Account view screen as whole numbers or words instead of spelling out each character, since no explicit accessibilityValue was set for these fields. Adds an opt-in spellOutAccessibilityValue flag to PasswordText and BitwardenTextValueField, backed by a new String.spellingOutCharacters() helper, and wires it up only for the six affected Bank Account fields so other consumers of these shared components are unaffected.
…ows to VoiceOver The row's more options button was visually tappable but unreachable by VoiceOver because the row combines its content into a single accessibility element, which swallows the nested button's own announcement and activation. Adds a named accessibility action so the overflow menu remains announced and accessible, matching the pattern already used for PasswordHistoryListView's combined row.
…o icon to VoiceOver The info button was nested inside BitwardenToggle's title content, so VoiceOver collapsed the whole row into one element and swallowed the button's own label and tap target. Move it into BitwardenToggle's accessory slot, which keeps it independently reachable, and add an "External link" hint, mirroring the Fill Assist toggle fix (PM-41097).
.accessibilityValue(value.spellingOutCharacters()) was being added on
top of a Text whose own content is already exposed as its
accessibility label, so VoiceOver would likely announce the value
twice: once normally, once spelled out. Use speechSpellsOutCharacters
instead, which tells VoiceOver to spell out the label itself rather
than adding a redundant value. Drops the now-unused
String.spellingOutCharacters() helper.
Also replaces a hand-rolled Task { await ... } in an
accessibilityAction with the existing accessibilityAsyncAction helper
for consistency.
ViewInspector can't inspect speechSpellsOutCharacters directly, so the
affected tests now verify the flag is passed through to PasswordText/
BitwardenTextValueField instead of the rendered accessibility value;
the actual VoiceOver announcement should be confirmed on-device.
The Add/Edit Bank Account screen's Account Type menu defaulted to displaying "Checking" even when the user had never opened the picker, because DefaultableType<BankAccountType>'s default-case label reused the "Checking" option name instead of a blank placeholder.
f6c8262 to
663bb65
Compare
| .if(spellOutAccessibilityValue && isPasswordVisible) { textView in | ||
| textView.accessibilityValue(password.spellingOutCharacters()) | ||
| } | ||
| .speechSpellsOutCharacters(spellOutAccessibilityValue) |
There was a problem hiding this comment.
❓ QUESTION: Was the duplicate-announcement behavior re-checked after re-adding accessibilityValue alongside speechSpellsOutCharacters?
Details
Commit 70b20eb removed .accessibilityValue(password.spellingOutCharacters()) for exactly this reason ("VoiceOver would likely announce the value twice: once normally, once spelled out") and replaced it with speechSpellsOutCharacters. The latest commit re-adds the value while keeping the modifier, so the element now has both a label (the rendered text) and a spelled-out value, with spell-out applied to both.
If on-device testing showed the modifier alone wasn't spelling out here — plausible, since colorCodedText(for:) inserts a zero-width space after every character — then this is the right fix and the question is just whether the value is now read once or twice. BitwardenTextValueField.swift:72 still uses the modifier alone, so the two components take different approaches.
Two smaller notes on the same lines:
speechSpellsOutCharactersis no longer gated onisPasswordVisible(it was in70b20eb), so when a bank account field is masked the••••••••placeholder is now spelled out one bullet at a time.- The
replacingOccurrences(of: String.zeroWidthSpace, with: "")added tospellingOutCharacters()never fires on this path:passwordis the raw value, and the ZWSPs only exist in theAttributedStringbuilt bycolorCodedText(for:).
1 participant
🎟️ Tracking
📔 Objective
Adds a batch of accessibility bug fixes under the Bank Account item type
📸 Screenshots
Click to reveal
PM-41938
showHideToggle.mov
PM-41939
addEditShowHideToggle.mov
PM-41940
individualCharacters.mov
PM-41941
overflowMenu.mov
PM-41942
mpRepromptInfoIcon.mov