fix(components): label icon-only controls for VoiceOver - #18
Conversation
The Swap and Coinbase screens in the wallet are largely accessible only
because they inherit this package's labels — their own files contain no
accessibility calls at all. The gaps here are inherited just as widely.
Toast's dismiss button, ConverterArrowBadge and the currency-select chevron
announced their asset or SF Symbol names ("diagonal-up-down", "icon copy
outline"), which sounds like a working label in a quick VoiceOver pass but
tells a blind user nothing. Each now carries a proper label, and the chevron
also carries an accessibilityValue naming the currency it currently holds —
a picker that never says what it is set to is unusable non-visually.
DualSwapAmountView's swap area was a bare container .onTapGesture: no button
trait, no label, and no way to activate it. It is now a single element with
both amounts in its label and an accessibilityAction, so double-tap swaps.
DashButton gains an optional accessibilityLabel. Constructing it with an icon
and no text produced a button whose only announcement was the asset name, with
no way for a caller to override — an API that made accessibility impossible
rather than merely absent. The parameter defaults to nil and sits before
`action:`, so every existing call site compiles unchanged.
The static rules the wallet uses do not catch the chevron: its label is
DashIcon.Common.chevronDownCurrencySelect.image rather than a literal Image,
so the rule sees no bare image. Found by reading, not by tooling.
Verified with swift build and swift build --build-tests.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Approving.
Additive only — no logic or signature changes. Checked the parts that could bite:
DashButton.accessibilityLabelsits before the trailingaction:with anildefault, so every existing call site (labelled and trailing-closure) compiles unchanged. Theif letinbodyis fine under the implicit@ViewBuilder.DualSwapAmountView's.accessibilityElement(children: .combine)is safe:SwapAmountViewrenders the amounts asText, notTextField, so nothing editable gets swallowed, and the chevronButtondoes survive as a custom action as the comment states.- The
.accessibilityHidden(true)on the non-interactivearrow-downbadge is the right call — it's decorative and was announcing its asset name.
One nit, non-blocking: accessibilityDescription's doc comment says "in visual order (primary first)", but AnimatedSwapLayout swaps the A/B rows via .offset when isPrimaryLarge == false, so the label order stops matching the visual order in that state. The announcement is still unambiguous since each amount carries its currency name — worth fixing the comment rather than the code.
Agree with the note that the on-device VoiceOver pass (dual-swap activation, picker value) still needs doing.
…ibes From @llbartekll's review of #18. The doc comment said the two amounts are listed "in visual order (primary first)", but AnimatedSwapLayout offsets the A and B rows past each other, so when isPrimaryLarge is false the secondary amount is the one drawn on top and the two orders diverge. The order in the label is logical, not visual, and the announcement is still unambiguous because each amount carries its own currency name — so the comment was the thing that was wrong, not the code. Reworded to say that, and to name the case where the two differ. swift build clean.
|
Fixed the doc comment in b76f6b3 — you were right that it was the comment, not the code. Confirmed the divergence in Nothing else changed; On the two you checked and cleared — the |
3 participants
Issue being fixed or feature implemented
Dash Wallet ticket 32072: "Please add support for screenreaders. The tabs at the bottom, as well as buttons, need to be labeled so that screenreaders can read them."
An accessibility audit of the wallet found that its Swap and Coinbase screens are ~88% accessible purely by inheriting this package's labels — the app-side files for those flows contain zero accessibility calls of their own. That cuts both ways: a gap here is inherited just as widely as a fix is.
A companion PR fixes the app's own shared components (dashpay/dashwallet-ios#…); this one closes the design-system half.
What was done?
Toast.swift— the icon-only dismiss button had no label.ConverterArrowBadge.swift— the swap-direction badge announced its SF Symbol name ("diagonal-up-down"). Now "Swap direction". The non-interactivearrow-downvariant is marked.accessibilityHidden(true), since it is decorative and was also announcing its asset name.SwapAmountView.swift— the currency-select chevron had no label and, more importantly, never announced which currency is currently selected. A picker that does not say what it currently holds is unusable non-visually, so it now carries both anaccessibilityLabel("Select currency") and anaccessibilityValue(the row's currency symbol, or "Dash" for a Dash row). Both call sites — the static primary row and the animated B row — are covered.DualSwapAmountView.swift— the swap area was a bare container.onTapGesture: invisible to VoiceOver, no button trait, and no way to activate it at all. It is now a single element combining both amounts, with the.isButtontrait, a hint describing what activation does, and an.accessibilityActionso double-tap actually swaps.DashButton.swift— adds an optionalaccessibilityLabel. Constructing the button with an icon andtext: nilproduced a control whose only announcement was the asset name, with no way for a caller to supply a label. That is an API that makes accessibility impossible rather than merely absent, which is worse than a missing call.Worth noting: the static rules the wallet now runs in CI do not catch the chevron — its label is
DashIcon.Common.chevronDownCurrencySelect.imagerather than a literalImage(...), so the rule sees no bare image. It was found by reading the code. The tooling under-reports; it is a ratchet, not a proof of correctness.How Has This Been Tested?
swift build— Build complete.swift build --build-testsalso passes; this is the real check that the package still compiles for its consumer.scripts/a11y_audit.pyfrom dashwallet-ios, run against this package) reported 3 findings forA11Y004/A11Y012before and 0 after. Its remaining findings here are allA11Y007(Dynamic Type), which is separate work.One finding from the audit turned out not to be a real defect: the
.onTapGesturereported atSwapAmountViewsits inside#if DEBUGpreview code and never ships. It was fixed anyway so the preview demonstrates the correct pattern.Breaking Changes
None. One public API addition:
DashButton.accessibilityLabel: String?, as both a property and an init parameter, defaulted toniland placed before the trailingaction:parameter — every existing call site, labelled or trailing-closure, compiles unchanged. No signatures were renamed or removed.The package has no localization catalog (no
.strings/.xcstrings;Package.swiftdeclaresdefaultLocalization: "en"). New strings follow the existing convention ofNSLocalizedString(_, bundle: .module, comment: "DashUIKit")relying on the key as its English fallback, exactly asNavigationBar,SearchBarandAddressFieldViewalready do. Keys introduced: "Select currency", "Swap direction", "Switches which currency you enter the amount in". If a catalog is ever added, these need entries.Checklist:
For repository code-owners and collaborators only