[A11y] Add button role to PressableWithDelayToggle for screen readers#84304
Conversation
…bility
Default accessibilityRole to CONST.ROLE.BUTTON in PressableWithDelayToggle so
screen readers announce the element as a button. Remove unused accessible={false}
props from TFA page callers. Replace blanket eslint-disable with targeted
eslint-disable-next-line on the inner PressableWithoutFeedback. Improve
accessibilityLabel fallback to use visible text when tooltip text is empty.
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…sites
The PressableProps type requires either accessible={false} or an
accessibilityLabel string. These call sites were missing it, causing
TypeScript errors.
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Fixed the failing Added |
|
@MelvinBot Mark it as ready for review |
|
PR has been marked as ready for review. |
melvin-bot
Bot
requested review from
trjExpensify and
truph01
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| text={tooltipTexts} | ||
| shouldRender | ||
| > | ||
| {/* eslint-disable-next-line react-native-a11y/has-valid-accessibility-descriptors */} |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line comment suppresses the react-native-a11y/has-valid-accessibility-descriptors rule but does not include an explanatory comment justifying why the disable is needed. Every ESLint disable should have an accompanying reason so future readers understand the exception.
Add a justification after the rule name using the -- separator:
{/* eslint-disable-next-line react-native-a11y/has-valid-accessibility-descriptors -- Inner pressable is intentionally non-accessible (accessible={false}) since the outer PressableView handles accessibility */}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| styles={[styles.button, styles.buttonMedium, styles.twoFactorAuthCopyCodeButton]} | ||
| textStyles={[styles.buttonMediumText]} | ||
| accessible={false} | ||
| accessibilityLabel={translate('twoFactorAuth.copy')} |
There was a problem hiding this comment.
@MelvinBot It looks like accessibilityLabel is never used in PressableWithDelayToggle, right? If that’s the case, passing accessibilityLabel={translate('twoFactorAuth.copy')} would be redundant.
There was a problem hiding this comment.
You're correct — accessibilityLabel is not used by PressableWithDelayToggle at runtime.
Here's what happens:
-
Not destructured: The component destructures 18 specific props by name at
PressableWithDelayToggle.tsx:72-91, butaccessibilityLabelis not among them, and there's no...restspread. The caller's value is silently dropped. -
Computed internally: At
PressableWithDelayToggle.tsx:135, the component builds its ownaccessibilityLabel:accessibilityLabel={tooltipTexts || (!isActive && textChecked ? textChecked : text) || ''}
This derives from
tooltipTexts,textChecked, andtext— never from the caller-supplied prop. -
But required by TypeScript:
PressableWithDelayTogglePropsextendsPressableProps, which uses a discriminated union (RequiredAccessibilityLabelinGenericPressable/types.ts:12-39) requiring eitheraccessibilityLabel: stringoraccessible: false. Removing the prop would cause a compilation error.
So the prop is redundant at runtime but required by the type system. The proper fix would be to update PressableWithDelayToggleProps to override the RequiredAccessibilityLabel constraint (since the component always computes its own label), so callers don't need to pass a value that gets discarded. I'll address this in the next commit.
| onPress={updatePressState} | ||
| accessibilityLabel={tooltipTexts} | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- Fallback to visible text when tooltip is empty for screen readers | ||
| accessibilityLabel={tooltipTexts || (!isActive && textChecked ? textChecked : text) || ''} |
There was a problem hiding this comment.
@MelvinBot Is this out of scope for this issue?
Screen readers (e.g. JAWS + Chrome) announce the TFA Copy and Download buttons without a button role because PressableWithDelayToggle does not default its accessibilityRole to button. This violates WCAG 4.1.2 (Name, Role, Value).
There was a problem hiding this comment.
The accessibilityLabel fallback is directly related to this issue. The issue is about screen readers not properly announcing the TFA buttons — without the fallback, if tooltipTexts is empty the screen reader would announce nothing at all. Adding the visible text (text/textChecked) as a fallback ensures screen readers always have a meaningful label, which is part of satisfying WCAG 4.1.2 (Name, Role, Value).
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-03-06.at.19.39.21.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-06.at.18.38.13.mov |
|
No, this is not out of scope — it's the core of the issue. The primary reproduction steps in Expensify/App#74861 (steps 1–7) are specifically about the TFA Copy and Download buttons being announced without a button role. The "Actual Result" section also directly references these controls. The fix in The issue does list additional "Other Occurrences" (Contact methods, Company cards, Track Distance), but those likely involve different components and would be separate fixes. The TFA buttons are the primary reported problem. |
| onPress={updatePressState} | ||
| accessibilityLabel={tooltipTexts} | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- Fallback to visible text when tooltip is empty for screen readers | ||
| accessibilityLabel={tooltipTexts || (!isActive && textChecked ? textChecked : text) || ''} |
There was a problem hiding this comment.
@MelvinBot Instead of keeping this inline:
accessibilityLabel={tooltipTexts || (!isActive && textChecked ? textChecked : text) || ''}
Consider creating a separate variable, such as processedAccessibilityLabel, and then using it like this:
accessibilityLabel={processedAccessibilityLabel}
This would improve readability and make the code easier to maintain.
There was a problem hiding this comment.
Done — extracted the expression into a processedAccessibilityLabel variable and moved the eslint-disable comment alongside it. See 26cd360.
…tyLabel variable and add eslint-disable justification Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #74861 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
MarioExpensify
deleted the
claude-addButtonRoleToPressableWithDelayToggle
branch
|
🚧 @MarioExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MarioExpensify in version: 9.3.33-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.33-5 🚀
|
4 participants
Explanation of Change
Screen readers (e.g. JAWS + Chrome) announce the TFA Copy and Download buttons without a button role because
PressableWithDelayToggledoes not default itsaccessibilityRoletobutton. This violates WCAG 4.1.2 (Name, Role, Value).This PR fixes the issue by:
accessibilityRoletoCONST.ROLE.BUTTONinPressableWithDelayToggle, so all usages automatically getrole="button"on web unless overridden.accessible={false}props from the TFA page callers (CopyCodesPage.tsx,VerifyPage.tsx) — these were accepted by the type system but never forwarded to the inner pressable, so they had no effect.eslint-disable react-native-a11y/has-valid-accessibility-descriptorsat the top ofPressableWithDelayToggle.tsxwith a targetedeslint-disable-next-lineon just the innerPressableWithoutFeedbackwhich legitimately usesaccessible={false}.accessibilityLabelfallback to use visible text (text/textCheckedprops) when tooltip text is empty, so screen readers always announce a meaningful label.Fixed Issues
$ #74861
PROPOSAL: #74861 (comment)
Tests
Offline tests
N/A — This is a UI accessibility fix (HTML role attributes). No network requests are involved.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — Accessibility role changes only affect web screen readers.
Android: mWeb Chrome
N/A — Accessibility role changes only affect web screen readers.
iOS: Native
N/A — Accessibility role changes only affect web screen readers.
iOS: mWeb Safari
N/A — Accessibility role changes only affect web screen readers.
MacOS: Chrome / Safari
N/A — Accessibility role changes primarily affect screen reader announcements. The visual appearance is unchanged.