Implement accessibility changes for search suggestions#85296
Implement accessibility changes for search suggestions#85296ShridharGoel wants to merge 15 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 |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@ShridharGoel can you borrow stuff from here - https://github.com/Expensify/App/pull/85166/changes Feels like a lot of overlap in functions. Eg: the hook useAccessibilityAnnouncement Wdyt? we'll resolve conflicts if they happen |
|
cc @Krishna2323 if you can advise us here |
|
@MelvinBot suggest changes #85296 (comment) |
|
@rushatgabhane @ShridharGoel — Good catch, there is significant overlap between this PR and #85166. Here's a detailed breakdown and suggestions: Overlap SummaryBoth PRs implement accessibility announcements for screen readers, but they take different architectural approaches:
Specific Suggestions1. Reuse
|
|
@rushatgabhane So should we hold one till the other is done? |
|
@ShridharGoel no we don't need to hold You can use that PR as the base and build on top of it and sync it if there are changes |
|
or you can copy paste relevant stuff if that is simpler |
melvin-bot
bot
requested review from
aimane-chnaif
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] |
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
|
Will need help from someone with a physical iPhone to test on iOS. |
I have physical iPhone to test |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return {sections: nextSections, styledRecentReports: nextStyledRecentReports}; | ||
| }, [autocompleteQueryValue, autocompleteSuggestions, expensifyIcons, getAdditionalSections, recentReportsOptions, recentSearchesData, searchOptions, searchQueryItem, styles, translate]); | ||
|
|
||
| const sectionItemText = sections?.at(1)?.data?.[0]?.text ?? ''; |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and will automatically memoize this pure computation. firstRecentReportFlatIndex is derived from firstRecentReportKey and sections with no side effects -- the compiler handles this without manual useMemo.
Remove the useMemo and compute the value directly:
let firstRecentReportFlatIndex = -1;
if (firstRecentReportKey) {
let flatIndex = 0;
for (const section of sections) {
// ... existing iteration logic ...
}
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const timeout = setTimeout(() => { | ||
| setLiveRegionAnnouncement(announcement); | ||
| setAnnouncementTimeout(null); | ||
| }, 50); |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The value 50 is a magic number used as a setTimeout delay for TalkBack live region updates. While the comment above explains the general strategy, the numeric value should be extracted into a named constant for readability and maintainability.
Extract it into a named constant:
const TALKBACK_LIVE_REGION_UPDATE_DELAY_MS = 50;
// Then use:
}, TALKBACK_LIVE_REGION_UPDATE_DELAY_MS);Consider also adding it to CONST.TIMING if it may be reused elsewhere.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Please fix conflict |
…ility77390 # Conflicts: # Mobile-Expensify # src/components/SelectionList/components/TextInput.tsx # src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx # src/hooks/useAccessibilityAnnouncement/index.ts # src/hooks/useDebouncedAccessibilityAnnouncement.ts # src/hooks/useDebouncedValue.ts
| const suggestionsCount = sections.reduce((total, section) => total + section.data.filter((item) => item.searchItemType !== CONST.SEARCH.SEARCH_ROUTER_ITEM_TYPE.FIND_ITEM).length, 0); | ||
| const trimmedAutocompleteQueryValue = autocompleteQueryValue.trim(); | ||
| const suggestionsAnnouncement = suggestionsCount > 0 ? translate('search.suggestionsAvailable', {count: suggestionsCount}, trimmedAutocompleteQueryValue || undefined) : ''; | ||
| useDebouncedAccessibilityAnnouncement(suggestionsAnnouncement, !!suggestionsAnnouncement, autocompleteQueryValue); |
There was a problem hiding this comment.
For this component, we only need this change and others are just refactor right? And what's the refactor for?
There was a problem hiding this comment.
Yes, the sections block was moved into useMemo at SearchAutocompleteList.
Because sections, styledRecentReports, and now suggestionsCount are all derived from the same inputs. This groups that derived work in one place instead of rebuilding pieces separately.
|
|
||
| const sectionItemText = sections?.at(1)?.data?.[0]?.text ?? ''; | ||
| const normalizedReferenceText = sectionItemText.toLowerCase(); | ||
| const suggestionsCount = sections.reduce((total, section) => total + section.data.filter((item) => item.searchItemType !== CONST.SEARCH.SEARCH_ROUTER_ITEM_TYPE.FIND_ITEM).length, 0); |
There was a problem hiding this comment.
Calculate count when data is pushed to sections. No need to iterate separately.
i.e.
let suggestionsCount = 0;
...
suggestionsCount += recentSearchesData.length;
sections.push({title, data: recentSearchesData})
...
| const normalizedReferenceText = sectionItemText.toLowerCase(); | ||
| const suggestionsCount = sections.reduce((total, section) => total + section.data.filter((item) => item.searchItemType !== CONST.SEARCH.SEARCH_ROUTER_ITEM_TYPE.FIND_ITEM).length, 0); | ||
| const trimmedAutocompleteQueryValue = autocompleteQueryValue.trim(); | ||
| const suggestionsAnnouncement = suggestionsCount > 0 ? translate('search.suggestionsAvailable', {count: suggestionsCount}, trimmedAutocompleteQueryValue || undefined) : ''; |
There was a problem hiding this comment.
Use one/other approach for singular/plural copy.
|| undefined is redundant
| const suggestionsCount = sections.reduce((total, section) => total + section.data.filter((item) => item.searchItemType !== CONST.SEARCH.SEARCH_ROUTER_ITEM_TYPE.FIND_ITEM).length, 0); | ||
| const trimmedAutocompleteQueryValue = autocompleteQueryValue.trim(); | ||
| const suggestionsAnnouncement = suggestionsCount > 0 ? translate('search.suggestionsAvailable', {count: suggestionsCount}, trimmedAutocompleteQueryValue || undefined) : ''; | ||
| useDebouncedAccessibilityAnnouncement(suggestionsAnnouncement, !!suggestionsAnnouncement, autocompleteQueryValue); |
There was a problem hiding this comment.
Should 3rd param be trimmedAutocompleteQueryValue?
There was a problem hiding this comment.
That third param is only used as the debounce signal in useDebouncedAccessibilityAnnouncement, so using autocompleteQueryValue makes whitespace edits reset the timer too. The announcement text itself already uses the trimmed value.
| options={textInputOptions} | ||
| onSubmit={selectFocusedItem} | ||
| dataLength={flattenedData.length} | ||
| dataLength={itemsCount} |
There was a problem hiding this comment.
What's this change for?
There was a problem hiding this comment.
This is to keep the announced count aligned with actual suggestion rows. flattenedData.length includes section headers, while itemsCount only counts real items, so dataLength={itemsCount} avoids overcounting when headers are present.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari
|
| const normalizedReferenceText = sectionItemText.toLowerCase(); | ||
| const trimmedAutocompleteQueryValue = autocompleteQueryValue.trim(); | ||
| const suggestionsAnnouncement = suggestionsCount > 0 ? translate('search.suggestionsAvailable', {count: suggestionsCount}, trimmedAutocompleteQueryValue) : ''; | ||
| useDebouncedAccessibilityAnnouncement(suggestionsAnnouncement, !!suggestionsAnnouncement, autocompleteQueryValue); |
There was a problem hiding this comment.
No announcement on search router. Works fine on Report search though
| const normalizedReferenceText = sectionItemText.toLowerCase(); | ||
| const trimmedAutocompleteQueryValue = autocompleteQueryValue.trim(); | ||
| const suggestionsAnnouncement = suggestionsCount > 0 ? translate('search.suggestionsAvailable', {count: suggestionsCount}, trimmedAutocompleteQueryValue) : ''; | ||
| useDebouncedAccessibilityAnnouncement(suggestionsAnnouncement, !!suggestionsAnnouncement, autocompleteQueryValue); |
There was a problem hiding this comment.
There's no announcement when no results found. Should we also fix this or out of scope?
There was a problem hiding this comment.
I'm pretty sure we fixed this on main, let's not undo that
There was a problem hiding this comment.
I'm pretty sure we fixed this on main, let's not undo that
Which PR?
There was a problem hiding this comment.
This page is a bit different. This search is using separate input, not part of SelectionListWithSections
And even though no results found, there's always item to be clickable. Not sure what to announce in this case.
There was a problem hiding this comment.
This will fix it btw, feel free to implement if you want
const suggestionsAnnouncement = suggestionsCount > 0 ? translate('search.suggestionsAvailable', {count: suggestionsCount}, trimmedAutocompleteQueryValue) : '';
useDebouncedAccessibilityAnnouncement(suggestionsAnnouncement, !!suggestionsAnnouncement, autocompleteQueryValue);
+
+const noResultsFoundText = translate('common.noResultsFound');
+const shouldAnnounceNoResults = suggestionsCount === 0 && !!trimmedAutocompleteQueryValue;
+useDebouncedAccessibilityAnnouncement(noResultsFoundText, shouldAnnounceNoResults, autocompleteQueryValue);
There was a problem hiding this comment.
Updated (with isLoading check as well since it shouldn't be announced when results are loading)
|
There's issue on search router. Otherwise looks good. |
|
@aimane-chnaif Seems to be working fine. I think in your video, there are no search results, hence no announcement. Screen.Recording.2026-03-20.at.2.44.01.AM.mov |
|
Still not working. Can you test again after merge main? I merged main locally since the branch is very behind. bug.mov |
|
Just tried after pulling latest main. Can you check once by waiting a few seconds after the loading is complete before changing the query? Screen.Recording.2026-03-20.at.2.56.52.AM.mov |
Still not working for me. Let's test in adhoc build |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #77390 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Just tested adhoc build. MacOS VoiceOver, Google Chrome |
|
Still working for me. Maybe we can get it QA tested. Screen.Recording.2026-03-21.at.1.10.09.AM.mov |
|
@ShridharGoel bump |
|
@rushatgabhane can we ask QA (or you) to test this page on MacOS VoiceOver? |
|
yep on Chrome macos it doesn't workl 
|
4 participants
Explanation of Change
Implement accessibility changes for suggestions
Fixed Issues
$ #77390
PROPOSAL:
Tests
Precondition: User is logged in
Offline tests
QA Steps
Same as tests.
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.12.50.20.AM.mov
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari