fix: The fields of code are not announced#85504
fix: The fields of code are not announced#85504truph01 wants to merge 11 commits intoExpensify:mainfrom
Conversation
|
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 |
|
@MelvinBot Please review this PR |
🤖 Code ReviewNice work adding screen reader announcements to 1. (High) Reuse the existing
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚧 @amyevans has triggered a test Expensify/App build. You can view the workflow run here. |
melvin-bot
bot
requested review from
aimane-chnaif and
joekaufmanexpensify
and removed request for
a team
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/components/MagicCodeInput.tsx
Outdated
| editIndex.current = index; | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The announcement value is purely derived from focusedIndex, maxLength, and translate -- it can be computed directly in the component body instead of using a useEffect + useState pair. The current approach introduces an unnecessary extra render cycle: first focusedIndex changes and the component renders with the stale announcement, then the effect fires, calls setAnnouncement, and triggers a second render with the correct value.
Replace the useState + useEffect with a derived value:
// Remove: const [announcement, setAnnouncement] = useState('');
// Remove: the useEffect that sets announcement
// Add: derive directly in the component body
const announcement = focusedIndex === undefined ? '' : translate('common.enterDigitLabel', {digitIndex: focusedIndex + 1, totalDigits: maxLength});Both consumers (useAccessibilityAnnouncement and the hidden <Text> live region) will work identically with the derived value, since they only read the string.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
src/components/MagicCodeInput.tsx
Outdated
| accessibilityLiveRegion="assertive" | ||
| > | ||
| {announcement} | ||
| </Text> |
There was a problem hiding this comment.
Won't it work without this hidden Text?
useAccessibilityAnnouncement is now supporting web thanks to #85166
🤖 Code Review (Updated)Good improvement — the updated PR now uses the existing 1. (Medium) Potential double-announcement on native platformsThe PR announces on native via two mechanisms simultaneously:
Suggestion: Either:
2. (Low) Redundant
|
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
No product review needed
![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: 9b447c35f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@truph01 gentle bump |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari
|
🤖 Code Review (Updated — v3)The reworked PR is a clear improvement: using the existing (High)
|
@truph01 what do you think of this concern? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cdb1050de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| accessibilityElementsHidden | ||
| importantForAccessibility="no-hide-descendants" |
There was a problem hiding this comment.
Hiding every digit cell here removes the only accessible copy of the code. The backing TextInput still uses value={input}, and setInputAndIndex() clears that state whenever the cursor moves, so after this change a screen reader can only hear the generic input label plus the new position alert. In OTP/PIN flows, users revisiting a slot can no longer tell whether it already contains a digit or what they are correcting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Do you have any suggestions on this?
Should we update the announcement parameter passed in useAccessibilityAnnouncement so that it also reflects the current digit?
|
Please address AI reviews. Otherwise looks good |
|
Yes — you can replace the entire inline platform logic with const announcement = focusedIndex !== undefined
? translate('common.enterDigitLabel', {digitIndex: focusedIndex + 1, totalDigits: maxLength})
: '';
useAccessibilityAnnouncement(announcement, focusedIndex !== undefined, {
shouldAnnounceOnNative: true,
shouldAnnounceOnWeb: true,
});This replaces the
Since the hook manages its own hidden DOM element on web, you can also remove the The What stays the same: The |
I removed aria-live="polite" logic totally. |
What was that fix for? |
@aimane-chnaif Previously, I added |
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
Pull main again once lint error is fixed
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #74882 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
Waiting for Eslint/Typescript errors to be fixed.. |
|
@truph01 pull main again. It's fixed now |
|
@aimane-chnaif @aldo-expensify All good now! |
|
I resolved conflicts |
6 participants
Explanation of Change
The magic code input fields (used for OTP / verification codes) were not announced by screen readers when the user navigated between digit positions. This made the component inaccessible — screen reader users had no way to know which digit they were editing.
Changes:
<Text>element withrole="alert"andaccessibilityLiveRegion="assertive"(maps toaria-live="assertive"in the DOM). A zero-width space toggle ensures re-announcement even when navigating back to the same digit.accessibilityLiveRegion="assertive"is natively supported by TalkBack.AccessibilityInfo.announceForAccessibility()is called explicitly, since iOS has no live-region equivalent.accessibilityElementsHiddenandimportantForAccessibility="no-hide-descendants"to each digit cell so the screen reader doesn't navigate to them individually. The hiddenTextInputremains the single focusable element, and the announcements provide the per-digit context.enterDigitLabeltranslation — Added the new accessibility string to all 10 language files (en, de, es, fr, it, ja, nl, pl, pt-BR, zh-hans).Fixed Issues
$ #74882
PROPOSAL:
Tests
Prerequisites: The user is signed in
Step 2 of 3: VerifyOffline tests
QA Steps
// 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-03-17.at.11.55.02.mov
https://meet.google.com/kwt-sgnz-ycohttps://meet.google.com/kwt-sgnz-ycoasd
Android: mWeb Chrome
Screen.Recording.2026-03-17.at.12.17.48.mov
iOS: Native
Screen.Recording.2026-03-17.at.12.28.05.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-03-17.at.11.42.44.mov