fix: Add screen reader support to Message search for announcing number of search result matches.#37248
fix: Add screen reader support to Message search for announcing number of search result matches.#37248antm-rp wants to merge 2 commits into
Conversation
…number of matching results in message search immediately after the search has been performed.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThese changes add accessibility improvements to message search result announcements in the UI and correct spelling errors in English translation strings, including a new pluralization structure for search result counts. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/i18n/src/locales/en.i18n.json (1)
4652-4655: Improve pluralization and 0-count UX for search results
- Add a “zero” form for clearer 0 results announcement.
- Avoid “Found 1 single result”; prefer “Found 1 result”.
Apply:
"Search_Messages_Count": { - "one": "Found {{count}} single result", - "other": "Found {{count}} results" + "zero": "No results found", + "one": "Found {{count}} result", + "other": "Found {{count}} results" },Ensure the calling code supports the “zero” plural form; otherwise it may still announce “Found 0 results.”
apps/meteor/client/views/room/contextualBar/MessageSearchTab/components/MessageSearch.tsx (1)
40-53: Test across multiple screen readers and browsers.The inconsistent behavior between Chromium+Orca and Firefox+Orca highlights the importance of cross-browser/screen-reader testing. Consider testing with:
- NVDA on Windows/Firefox
- JAWS on Windows/Edge or Chrome
- VoiceOver on macOS/Safari
Different combinations handle dynamic ARIA announcements differently, and a persistent aria-live region (as suggested above) generally provides the most consistent results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/room/contextualBar/MessageSearchTab/components/MessageSearch.tsx(2 hunks)packages/i18n/src/locales/en.i18n.json(6 hunks)
🔇 Additional comments (6)
packages/i18n/src/locales/en.i18n.json (5)
318-318: Spelling fix looks good“Maximum Invalid Email OTP Codes Allowed” reads correctly.
584-584: Typo correction approved“accommodate” is correct.
949-949: Typo correction approved“downgrade” is correct.
991-991: Typo correction approved“Channels added successfully” is correct.
6601-6601: Spelling fix approved“notifications” is correct.
apps/meteor/client/views/room/contextualBar/MessageSearchTab/components/MessageSearch.tsx (1)
6-6: LGTM!Importing
VisuallyHiddenfrom react-aria is appropriate for creating screen-reader-only announcements.
| {messageSearchQuery.data.length === 0 && ( | ||
| <Box is='section' aria-live='assertive' aria-atomic='true' tabIndex={-1} | ||
| key={`no-results-${messageSearchQuery.data.length}`} | ||
| aria-label={t('Search_Messages_Count', { count: messageSearchQuery.data.length })} | ||
| > | ||
| <ContextualbarEmptyContent title={t('No_results_found')} /> | ||
| </Box> | ||
| )} |
There was a problem hiding this comment.
Consider using aria-live='polite' instead of 'assertive'.
Using aria-live='assertive' interrupts the screen reader immediately, which can be jarring for users. For search results, aria-live='polite' is more appropriate as it waits for the user to finish their current activity before announcing.
Additionally:
- The
keyprop will always evaluate to"no-results-0"since this branch only executes whendata.length === 0. If the intent is to force a re-mount for subsequent zero-result searches, consider using a timestamp or search query hash instead. - The
tabIndex={-1}attribute makes the element programmatically focusable but not keyboard-navigable. Clarify if this is intentional or consider removing it. - Combining
aria-labelwithContextualbarEmptyContent'stitleprop may result in redundant screen reader announcements.
Apply this diff to use polite announcements:
- <Box is='section' aria-live='assertive' aria-atomic='true' tabIndex={-1}
+ <Box is='section' aria-live='polite' aria-atomic='true'
key={`no-results-${messageSearchQuery.data.length}`}
aria-label={t('Search_Messages_Count', { count: messageSearchQuery.data.length })}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {messageSearchQuery.data.length === 0 && ( | |
| <Box is='section' aria-live='assertive' aria-atomic='true' tabIndex={-1} | |
| key={`no-results-${messageSearchQuery.data.length}`} | |
| aria-label={t('Search_Messages_Count', { count: messageSearchQuery.data.length })} | |
| > | |
| <ContextualbarEmptyContent title={t('No_results_found')} /> | |
| </Box> | |
| )} | |
| {messageSearchQuery.data.length === 0 && ( | |
| <Box is='section' aria-live='polite' aria-atomic='true' | |
| key={`no-results-${messageSearchQuery.data.length}`} | |
| aria-label={t('Search_Messages_Count', { count: messageSearchQuery.data.length })} | |
| > | |
| <ContextualbarEmptyContent title={t('No_results_found')} /> | |
| </Box> | |
| )} |
🤖 Prompt for AI Agents
In
apps/meteor/client/views/room/contextualBar/MessageSearchTab/components/MessageSearch.tsx
around lines 40 to 47, change aria-live='assertive' to aria-live='polite' so
screen readers announce search results less intrusively; replace the static
key={`no-results-${messageSearchQuery.data.length}`} (always "no-results-0")
with a value that changes when a new zero-result search occurs (e.g., include a
timestamp or a hash of the current search query) if you intend to force
remounts; remove or reconsider tabIndex={-1} (either omit it if not needed for
programmatic focus or keep it intentionally with a comment) and eliminate the
redundant aria-label since ContextualbarEmptyContent already provides a title to
avoid double announcements.
| {messageSearchQuery.data.length > 0 && ( | ||
| <VisuallyHidden role='status' aria-live='assertive' aria-atomic='true' aria-relevant='text' id='search-results-announcer'> | ||
| {t('Search_Messages_Count', { count: messageSearchQuery.data.length })} | ||
| </VisuallyHidden> | ||
| )} |
There was a problem hiding this comment.
Conditionally rendering aria-live regions is unreliable.
When the component re-renders with new search results, the VisuallyHidden element may already be mounted from the previous search. React won't re-mount it, so the text change might not trigger a screen reader announcement. This likely explains why Firefox + Orca didn't announce results in your testing.
Best practice: Keep the aria-live region mounted persistently and update its content dynamically. This ensures screen readers reliably detect and announce content changes across all browsers.
Additionally:
role='status'has an implicitaria-live='polite', but you're overriding it to'assertive'. For search results,'polite'is more appropriate.- The
id='search-results-announcer'is never referenced elsewhere in the code. aria-relevant='text'is redundant as the default is'additions text'.
Consider this refactor using a persistent aria-live region:
+ // Add state to track announcement text
+ const [announcementText, setAnnouncementText] = useState('');
+
+ // Update announcement when data changes
+ React.useEffect(() => {
+ if (messageSearchQuery.data) {
+ setAnnouncementText(t('Search_Messages_Count', { count: messageSearchQuery.data.length }));
+ }
+ }, [messageSearchQuery.data, t]);
+
return (
<Box display='flex' flexDirection='column' flexGrow={1} flexShrink={1} flexBasis={0}>
+ {/* Persistent announcer - always mounted */}
+ <VisuallyHidden role='status' aria-live='polite' aria-atomic='true'>
+ {announcementText}
+ </VisuallyHidden>
+
{messageSearchQuery.data && (
<>
{messageSearchQuery.data.length === 0 && (
- <Box is='section' aria-live='assertive' aria-atomic='true' tabIndex={-1}
- key={`no-results-${messageSearchQuery.data.length}`}
- aria-label={t('Search_Messages_Count', { count: messageSearchQuery.data.length })}
- >
+ <Box is='section'>
<ContextualbarEmptyContent title={t('No_results_found')} />
</Box>
)}
-
- {messageSearchQuery.data.length > 0 && (
- <VisuallyHidden role='status' aria-live='assertive' aria-atomic='true' aria-relevant='text' id='search-results-announcer'>
- {t('Search_Messages_Count', { count: messageSearchQuery.data.length })}
- </VisuallyHidden>
- )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {messageSearchQuery.data.length > 0 && ( | |
| <VisuallyHidden role='status' aria-live='assertive' aria-atomic='true' aria-relevant='text' id='search-results-announcer'> | |
| {t('Search_Messages_Count', { count: messageSearchQuery.data.length })} | |
| </VisuallyHidden> | |
| )} | |
| // Inside your MessageSearchTab component, before the return: | |
| const [announcementText, setAnnouncementText] = useState(''); | |
| // Update the announcer text whenever the search results change | |
| React.useEffect(() => { | |
| if (messageSearchQuery.data) { | |
| setAnnouncementText(t('Search_Messages_Count', { count: messageSearchQuery.data.length })); | |
| } | |
| }, [messageSearchQuery.data, t]); | |
| // In your render: | |
| return ( | |
| <Box display='flex' flexDirection='column' flexGrow={1} flexShrink={1} flexBasis={0}> | |
| {/* Persistent announcer – always mounted */} | |
| <VisuallyHidden role='status' aria-live='polite' aria-atomic='true'> | |
| {announcementText} | |
| </VisuallyHidden> | |
| {messageSearchQuery.data && ( | |
| <> | |
| {messageSearchQuery.data.length === 0 && ( | |
| <Box is='section'> | |
| <ContextualbarEmptyContent title={t('No_results_found')} /> | |
| </Box> | |
| )} | |
| {/* …your existing rendering of actual search results when length > 0 */} | |
| </> | |
| )} | |
| </Box> | |
| ); |
🤖 Prompt for AI Agents
In
apps/meteor/client/views/room/contextualBar/MessageSearchTab/components/MessageSearch.tsx
around lines 49-53, the VisuallyHidden aria-live region is conditionally mounted
which prevents some screen readers from detecting updates; instead keep a
persistent aria-live element (always rendered) and update its inner text based
on messageSearchQuery.data.length, change role to "status" (or remove explicit
aria-live so it uses the role's polite behavior), remove the unused id and
redundant aria-relevant attribute, and ensure the rendered string is updated
whenever messageSearchQuery.data changes so screen readers reliably announce new
results.
|
Which issue number does this fix? Have you checked that the team want to look at this? Read this on Rocket.Chat and github |
|
This is the implementation of WA-24 @reetp |
3 participants
fix: Add screen reader support for Search messages
Proposed changes
This PR improves accessibility/WCAG compability - improvement to the user experience when using the Search Messages functionality together with Screen reader software.
When a user performs a search in the Search Messages (
MessageSearchcomponent) interface, this fix introduces support for screen readers to announce the number of results found. If no matching results are found, then the user is informed that no results have been found.The result feedback is to be announced immediately upon a search and should not force the user to navigate to the result list.
Issue(s)
Steps to test or reproduce
Further comments
Development and testing:
When the number of results are greater then zero, include a
VisuallyHiddenblock to make the results available to screen reader. There may be other and better approaches than this.Summary by CodeRabbit
Bug Fixes
New Features