Add screen reader focus to emoji category shortcuts#84629
Conversation
When selecting an emoji category shortcut button, the screen reader now receives proper feedback: the button announces its selected state, and focus moves to the corresponding category heading in the emoji list. Co-authored-by: truph01 <truph01@users.noreply.github.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
The failing checks are unrelated to this PR's changes. Analysis: Both Android build failures ( Evidence: This PR only modifies TypeScript/TSX emoji picker files ( Re-running the failed builds should resolve this. |
|
@MelvinBot Please merge main |
…11yFocus Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Merged main into this branch. |
| } | ||
| pendingHeaderFocusIndexRef.current = null; | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| InteractionManager.runAfterInteractions(() => { |
There was a problem hiding this comment.
@MelvinBot
Why do we need InteractionManager.runAfterInteractions here? In the index.tsx version, we don’t use it.
Also, InteractionManager.runAfterInteractions is a deprecated method and should not be used in this app.
There was a problem hiding this comment.
Good catch — removed InteractionManager.runAfterInteractions from both usages:
-
focusHeaderAtIndex: Now callsAccessibilityInfo.sendAccessibilityEventdirectly. ThescheduleHeaderFocusretry mechanism (which retries up to 2 times withCONST.ANIMATED_TRANSITIONdelay) already handles the case where the ref isn't ready yet, so theInteractionManagerwrapper was unnecessary. -
updateEmojiList: Now usesrequestAnimationFramedirectly without theInteractionManagerwrapper, matching how the web version handles it.
Also removed the InteractionManager import since it's no longer used.
Replace with direct calls - AccessibilityInfo.sendAccessibilityEvent is called directly in focusHeaderAtIndex (the scheduleHeaderFocus retry mechanism already handles timing), and requestAnimationFrame is used directly in updateEmojiList without the InteractionManager wrapper. Co-authored-by: truph01 <truph01@users.noreply.github.com>
| onMomentumScrollEnd={() => { | ||
| if (pendingHeaderFocusIndexRef.current == null) { | ||
| return; | ||
| } | ||
| scheduleHeaderFocus(pendingHeaderFocusIndexRef.current); | ||
| }} |
There was a problem hiding this comment.
@MelvinBot In native version (index.native.tsx file), why do we need to call scheduleHeaderFocus in both onMomentumScrollEnd and scrollToHeader, meanwhile in web version (index.tsx file) we only call it in scrollToHeader?
|
The difference comes down to how scroll animations and FlashList virtualization behave on each platform. On native,
On web, the scroll animation is more predictable (CSS-based, no native momentum physics). The web version wraps In short: native needs both because the momentum scroll timing is unpredictable and virtualized items may not be mounted until the scroll fully settles. Web doesn't need the fallback because a simple |
|
@MelvinBot In Then, in This approach looks like a workaround. Why do we need two |
|
Good question. The two 1. The outer emojiListRef.current?.scrollToOffset({offset: calculatedOffset, animated: true});
setTimeout(() => {
scheduleHeaderFocus(headerIndex);
}, CONST.ANIMATED_TRANSITION);This waits for the animated scroll to finish before attempting to focus the header. 2. The inner function schedule(headerIndex, attempt = 0) {
if (focusHeaderAtIndex(headerIndex)) return;
if (attempt >= 2) return;
setTimeout(() => schedule(headerIndex, attempt + 1), CONST.ANIMATED_TRANSITION);
}Even after the scroll completes, FlashList's virtualization may not have rendered the target header yet (especially when scrolling to a distant category). If Why This is admittedly a pragmatic workaround. The core problem is that FlashList doesn't provide a "scroll completed and target item is rendered" callback on web. The alternatives considered:
If there's a preferred pattern in the codebase for handling this kind of "wait for virtualized item to render after scroll" scenario, I'm happy to adopt it instead. |
|
@MelvinBot Mark this PR as ready for review |
|
PR has been marked as ready for review. |
|
The failing Analysis: The Evidence: The error log shows: This occurred before any code review logic ran. The other failing check ( Recommendation: Re-run the |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-13.at.10.10.17.movAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-11.at.14.13.45.mov |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3226960b5c
ℹ️ 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".
| if (focusHeaderAtIndex(headerIndex)) { | ||
| return; |
There was a problem hiding this comment.
schedule() always attempts focusHeaderAtIndex(headerIndex) even if another shortcut was selected afterward, so a delayed retry from an earlier tap can steal screen-reader focus back to the old category. This is reproducible when a user activates category A and then category B before A’s retry fires: the stale retry has no pendingHeaderFocusIndexRef guard and may announce the wrong heading.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this was a real bug. If a user quickly taps category A then category B, the delayed retry from A's scheduleHeaderFocus could fire after B was selected and steal screen-reader focus back to the wrong heading.
Fixed by adding a staleness guard at the top of scheduleHeaderFocus: it now checks pendingHeaderFocusIndexRef.current === headerIndex before each attempt. When a new category is selected, scrollToHeader updates pendingHeaderFocusIndexRef to the new index, so any in-flight retries for the old index will bail out immediately.
Applied to both index.tsx and index.native.tsx.
There was a problem hiding this comment.
Fixed — added a staleness guard to scheduleHeaderFocus in both files. See my reply above for details.
Add a check at the top of scheduleHeaderFocus to bail out if pendingHeaderFocusIndexRef no longer matches the target headerIndex. This prevents a delayed retry from an earlier category tap from stealing screen-reader focus after the user has already selected a different category. Co-authored-by: truph01 <truph01@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #77490 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
conflicts 😬 |
|
@MelvinBot We have conflicts, please resolve it. |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Merged main and resolved conflicts in
|
|
The failing check ( Analysis: The 2 failing tests are in Evidence:
|
|
@Beamanator All good now! The test (job 8) isn't related to this PR. |
|
@MelvinBot merge main |
…11yFocus Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
|
Merged main into this branch. |
|
The failing check Analysis: The build fails with: This is an Android build infrastructure issue with expo autolinking — not related to the emoji picker accessibility changes in this PR. Evidence: The same |
|
The failing checks are unrelated to this PR's changes. Failing checks:
Why these are unrelated:
No action needed from the PR author. These builds should resolve once the infrastructure issue is fixed upstream. |
|
rerunning failing checks |
|
The failing check Analysis: The build fails with Evidence: The exact same error is occurring on the |
Beamanator
left a comment
There was a problem hiding this comment.
yeah i believe it, we can move forward
|
failing checks unrelated |
|
@Beamanator looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚧 @Beamanator 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/Beamanator in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
Explanation of Change
When a screen reader user selects an emoji category shortcut button (e.g., "Frequently used", "Smileys"), the emoji list scrolls visually but no screen reader feedback is provided. This PR fixes the accessibility gap by:
Viewelements now have proper accessibility roles (header/heading) and labels so screen readers can identify them as section headingsAccessibilityInfo.sendAccessibilityEvent; on web,.focus()viatabIndex={-1}accessibilityState={{ selected: true }}so screen readers announce the selected stateA retry mechanism handles the case where the target header may not be mounted yet in the virtualized FlashList when scrolling to a distant category.
Fixed Issues
$ #77490
PROPOSAL: #77490 (comment)
Tests
Offline tests
This change only affects UI accessibility rendering and screen reader focus management. 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari