Fix: SR Settings Security status messages not announced#82935
Fix: SR Settings Security status messages not announced#82935marufsharifi wants to merge 6 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
bot
requested review from
JmillsExpensify and
suneox
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const isUserValidated = account?.validated ?? false; | ||
| const {asset: ShieldYellow} = useMemoizedLazyAsset(() => loadIllustration('ShieldYellow' as IllustrationName)); | ||
| const announceStatus = (message: string) => { |
There was a problem hiding this comment.
❌ CONSISTENCY-1 (docs)
This function and the associated hidden <Text> element (lines 163-173) introduce multiple Platform.OS checks within the component to handle platform-specific accessibility behavior (iOS gets announceForAccessibility, web gets role="alert", Android gets accessibilityRole + accessibilityLiveRegion). The codebase already has an established pattern for this exact scenario using platform-specific file extensions — see useFormHelpMessageAccessibilityAnnouncement.ios.ts and its default .ts counterpart.
Consider extracting the accessibility announcement logic into a custom hook with platform-specific file variants (e.g., useAccessibilityAnnouncement.ios.ts, useAccessibilityAnnouncement.android.ts, useAccessibilityAnnouncement.ts) similar to the existing useFormHelpMessageAccessibilityAnnouncement pattern, or generalize that existing hook to be reusable for non-form contexts.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
style/maintainability suggestion, not required for correctness of this fix, and would add extra file churn.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd9b0efebb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| localFileDownload('two-factor-auth-codes', account?.recoveryCodes ?? '', translate); | ||
| setError(''); | ||
| setCodesAreCopied(); | ||
| announceStatus(translate('fileDownload.success.title')); |
There was a problem hiding this comment.
The new download handler announces fileDownload.success.title immediately on press, but localFileDownload is asynchronous and has an explicit failure path on Android (src/libs/localFileDownload/index.android.ts, .catch(() => FileUtils.showGeneralErrorAlert(...))). In permission-denied/storage-failure cases, screen-reader users will still hear a success message even though the download failed, which makes the accessibility feedback inaccurate.
Useful? React with 👍 / 👎.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No concerns from product
| if (Platform.OS === CONST.PLATFORM.IOS) { | ||
| AccessibilityInfo.announceForAccessibility(message); | ||
| } |
There was a problem hiding this comment.
On iOS doesn't work with latest main
ScreenRecording_02-28-2026.16-47-03_1.1.mp4
As the same mentioned on previous PR We need delay to announce on iOS
| Clipboard.setString(account?.recoveryCodes ?? ''); | ||
| setError(''); | ||
| setCodesAreCopied(); | ||
| announceStatus(translate('common.copied')); |
There was a problem hiding this comment.
Please verify that the expected result is Codes copied to clipboard and carefully review the test steps. and also update your recording
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #76928 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
I accidentally submitted an approval review it should have been Comment changes |
|
@marufsharifi could you please priortize this PR |
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
Hi @rushatgabhane, all the comments were already addressed in the latest commit before this was closed, and the PR should now be ready to merge. Would it be possible to reopen it and request a C+ review, please? Thanks. |
| style={styles.hiddenElementOutsideOfWindow} | ||
| role={Platform.OS === CONST.PLATFORM.WEB ? CONST.ROLE.ALERT : undefined} | ||
| accessibilityRole={Platform.OS === CONST.PLATFORM.ANDROID ? CONST.ROLE.ALERT : undefined} | ||
| accessibilityLiveRegion={Platform.OS === CONST.PLATFORM.ANDROID ? 'assertive' : undefined} |
There was a problem hiding this comment.
@marufsharifi not really, there are a couple of issues with this. Not following the guidelines for platform code
4 participants
Explanation of Change
Adds accessibility feedback to the 2FA recovery codes step.
When users tap Copy or Download, the page now updates a hidden live-region alert so screen readers announce success (Copied! / Downloaded!), including repeated taps of the same action.
Fixed Issues
$ #76928
PROPOSAL: #76928 (comment)
Tests
button/Downloadbutton and activate it.Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Screen.Recording.2026-02-23.at.2.38.32.PM.mov
Android: mWeb Chrome
Screen.Recording.2026-02-23.at.2.44.52.PM.mov
iOS: Native
Screen.Recording.2026-03-05.at.3.24.07.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-03-05.at.3.39.01.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-03-05.at.3.42.03.PM.mov