fix: Add screen reader support for reading the number of results when…#37407
fix: Add screen reader support for reading the number of results when…#37407antm-rp wants to merge 3 commits into
Conversation
… performing a search based on room names.
|
|
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 |
WalkthroughAdds live-region announcements for search result counts: introduces local Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Search Component
participant React as React (state/render)
participant Effect as useEffect
participant i18n as i18n Service
participant A11y as Live Region (SR)
User->>UI: update search query
UI->>React: update items (items.length changes)
React->>Effect: trigger effect (items.length)
Effect->>i18n: t("Search_results", { count })
i18n-->>Effect: pluralized string
Effect->>React: set announcementText
React->>UI: re-render (results container key changes → remount)
UI->>A11y: update VisuallyHidden / aria-label
A11y-->>User: screen reader announces updated count / no-results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (2)
packages/i18n/src/locales/en.i18n.json (1)
4673-4676: Pluralization looks good; ensure callers pass count.Keys and placeholders match i18n patterns. Please confirm usage is
t('Search_results', { count })so singular/plural resolves correctly. Optionally add a zero form if UX prefers “No results”:"Search_results": { + "zero": "No results", "one": "{{count}} result", "other": "{{count}} results" },Also consider adding this key to other locales (or confirm acceptable fallback to English).
apps/meteor/client/sidebar/search/SearchList.tsx (1)
323-328: Remove redundant undefined check.Since
itemsis initialized as an empty array on line 219 (const { data: items = [], isLoading }),items.lengthis always a number and will never beundefined. The conditional check on line 325 is unnecessary.Apply this diff to simplify the logic:
const [announcementText, setAnnouncementText] = useState(''); useEffect(() => { - if (items.length !== undefined) { - setAnnouncementText(t('Search_results', { count: items.length })); - } + setAnnouncementText(t('Search_results', { count: items.length })); }, [items.length, t]);
📜 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/sidebar/search/SearchList.tsx(2 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/sidebar/search/SearchList.tsx (1)
apps/meteor/app/utils/lib/i18n.ts (1)
t(6-6)
🔇 Additional comments (1)
apps/meteor/client/sidebar/search/SearchList.tsx (1)
369-373: Verify that this ARIA pattern works correctly across screen readers.The combination of
role='listbox',aria-live='polite',aria-atomic='true', and a dynamicaria-labelis non-standard. The more common pattern for announcing result counts is a separate live region element:<div aria-live="polite" aria-atomic="true" className="sr-only"> {announcementText} </div>While your approach may work, it should be tested across multiple screen readers (NVDA, JAWS, VoiceOver) to ensure:
- The count is announced when search results change
- The announcement doesn't interfere with navigating the list items
- Both zero results and non-zero results are announced correctly
If testing reveals issues, consider refactoring to use a dedicated live region element separate from the listbox.
| aria-live='polite' | ||
| aria-atomic='true' | ||
| aria-busy={isLoading} | ||
| key={`no-results-${items.length}`} |
There was a problem hiding this comment.
Remove the dynamic key prop—it breaks live region behavior and user interactions.
Changing the key forces React to unmount and remount the entire results container, which:
- Destroys and recreates the Virtuoso list component and all children
- Resets scroll position and internal state
- Can interrupt ongoing keyboard navigation or focus
- Is unnecessary for ARIA live regions to function
ARIA live regions (via aria-live='polite' on line 369) automatically announce changes when the aria-label content updates—no remounting is needed. Remove the key prop and let the existing live region attributes handle the announcements.
Apply this diff:
aria-live='polite'
aria-atomic='true'
aria-busy={isLoading}
- key={`no-results-${items.length}`}
aria-label={announcementText}
onClick={handleClick}📝 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.
| key={`no-results-${items.length}`} | |
| aria-live='polite' | |
| aria-atomic='true' | |
| aria-busy={isLoading} | |
| aria-label={announcementText} | |
| onClick={handleClick} |
🤖 Prompt for AI Agents
In apps/meteor/client/sidebar/search/SearchList.tsx around line 372 the dynamic
key={`no-results-${items.length}`} causes React to unmount/remount the results
container (resetting Virtuoso state, scroll, focus and breaking live-region
interactions); remove the key prop entirely so the container is not
recreated—rely on the existing aria-live='polite' and aria-label updates to
announce changes instead.
There was a problem hiding this comment.
I have tried with removing this line as suggested, but the effect seem to be that the result is no longer read. Understandably we do not want to remount the Virtouso component, so what are our options here?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for VisuallyHidden component imports and usage
rg -n "VisuallyHidden" --type=tsx --type=ts -C2
# Check the NavBarSearchListbox implementation mentioned in the AI summary
cat apps/meteor/client/NavBarV2/NavBarSearch/NavBarSearchListbox.tsx
Length of output: 2908
@antm-rp You're absolutely right! The issue is that ARIA live regions respond to changes in DOM text content, not changes to aria-label attributes. Looking at your NavBarSearchListbox.tsx implementation (which correctly handles this), the solution is to use a VisuallyHidden component with the announcement text as actual text content.
Here's what you need to do:
- Import
VisuallyHiddenfromreact-aria(add to existing React imports) - Remove the dynamic
keyprop (to avoid remounting) - Remove
aria-labelfrom the Box (it doesn't trigger announcements) - Add a
VisuallyHiddencomponent inside the Box withrole='status'containing the announcement text
Apply this change:
// Add to imports at the top
import { VisuallyHidden } from 'react-aria';
// In the return statement, modify the Box:
<Box
ref={boxRef}
role='listbox'
id={listId}
tabIndex={-1}
flexShrink={1}
h='full'
w='full'
data-qa='sidebar-search-result'
aria-live='polite'
aria-atomic='true'
aria-busy={isLoading}
onClick={handleClick}
>
<VisuallyHidden role='status' aria-live='polite' aria-atomic='true'>
{announcementText}
</VisuallyHidden>
<VirtualizedScrollbars>
<Virtuoso
style={{ height: '100%', width: '100%' }}
totalCount={items.length}
data={items}
computeItemKey={(_, room) => room._id}
itemContent={(_, data): ReactElement => <Row data={itemData} item={data} />}
ref={listRef}
/>
</VirtualizedScrollbars>
</Box>This matches the pattern you successfully used in NavBarSearchListbox.tsx. When announcementText updates (due to items.length changing), the text content inside VisuallyHidden changes, which triggers the live region announcement—without remounting the Virtuoso component.
🧬 Code Graph Analysis Results
[apps/meteor/tests/e2e/page-objects/fragments/sidebar.ts, lines 3-88]
export class Sidebar {
private readonly page: Page;
constructor(page: Page) {
this.page = page;
}
// New navigation locators
get sidebar(): Locator {
return this.page.getByRole('navigation', { name: 'Sidebar' });
}
get teamCollabFilters(): Locator {
return this.sidebar.getByRole('tablist', { name: 'Team collaboration filters' });
}
get omnichannelFilters(): Locator {
return this.sidebar.getByRole('tablist', { name: 'Omnichannel filters' });
}
get allTeamCollabFilter(): Locator {
return this.teamCollabFilters.getByRole('button', { name: 'All' });
}
get favoritesTeamCollabFilter(): Locator {
return this.teamCollabFilters.getByRole('button', { name: 'Favorites' });
}
get discussionsTeamCollabFilter(): Locator {
return this.teamCollabFilters.getByRole('button', { name: 'Discussions' });
}
// TODO: fix this filter, workaround due to virtuoso
get topChannelList(): Locator {
return this.sidebar.getByTestId('virtuoso-top-item-list');
}
get channelsList(): Locator {
// TODO: fix this filter, workaround due to virtuoso
// return this.sidebar.getByRole('list', { name: 'Channels' }).filter({ has: this.page.getByRole('listitem') });
return this.sidebar.getByTestId('virtuoso-item-list');
}
getSearchRoomByName(name: string) {
return this.channelsList.getByRole('button', { name, exact: true });
}
get firstCollapser(): Locator {
return this.topChannelList.getByRole('region').first();
}
get teamsCollapser(): Locator {
return this.sidebar.getByRole('region', { name: 'Collapse Teams' }).first();
}
get channelsCollapser(): Locator {
return this.channelsList.getByRole('region', { name: 'Collapse Channels' });
}
get directMessagesCollapser(): Locator {
return this.channelsList.getByRole('region', { name: 'Collapse Direct messages' });
}
get firstChannelFromList(): Locator {
return this.channelsList.getByRole('listitem').first();
}
async escSearch(): Promise<void> {
await this.page.keyboard.press('Escape');
}
async markItemAsUnread(item: Locator): Promise<void> {
await item.hover();
await item.focus();
await item.getByRole('button', { name: 'Options', exact: true }).click();
await this.page.getByRole('menuitem', { name: 'Mark Unread' }).click();
}
getCollapseGroupByName(name: string): Locator {
return this.channelsList.getByRole('button').filter({ has: this.page.getByRole('heading', { name, exact: true }) });
}
getItemUnreadBadge(item: Locator): Locator {
return item.getByRole('status', { name: 'unread' });
}
}
There was a problem hiding this comment.
Indeed it matches the pattern used in other components, however, having tested it locally with the suggested changes does not result in a consistent behavior where the results are read, but often skipped. Potentially, this could be an issue with the screen reader, and may need further testing as the arguments for using a VisuallyHidden object are strong.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ARIA live region screen reader skipping announcements best practices
💡 Result:
Short summary of causes and actionable best practices.
Why announcements get skipped
- Live region not empty on page load (AT may ignore initial content). [1][3]
- Updates happen too quickly or the node is removed before AT reads it. [4][3]
- Multiple live regions or competing politeness levels (assertive can cancel polite). [1][2]
- Missing/incorrect roles/attributes so AT doesn’t treat the element as a status. [5]
- Browser / screen reader differences and timing of accessibility tree updates. [6]
Concrete best practices (implement these)
- Start with an empty live region and add text in a separate update (create then set content). [1][6]
- Use the correct role/value: role="status" or aria-live="polite" for non‑urgent; role="alert" or aria-live="assertive" only for urgent interruptions. Match visual urgency to ARIA. [1][7]
- Keep messages short, plain-text, and non-interactive so they’re readable in one pass. [4][7]
- Empty the region between messages and wait a short debounce (commonly 300–500 ms) before inserting the next message to ensure AT detects the new addition. If you add the region dynamically, wait ~2s before first update. [4][3][6]
- Use aria-atomic and aria-relevant appropriately: aria-atomic="true" when you need the whole region read; aria-relevant to limit announced change types. [1][2]
- Use aria-busy while building complex content to prevent premature announcements. [1][2]
- Limit number of live regions and avoid firing multiple announcements at the same time; queue server/client messages if needed. [2][7]
- If messages auto-dismiss, ensure they persist long enough for users to hear them (or provide a way to retrieve them); don’t remove immediately after injection. [4][5]
- Don’t rely on focus changes to announce status (only use when shifting focus is required); live regions should not steal keyboard focus. [1][7]
- Test across AT/browser combos (NVDA+Firefox, JAWS+IE/Edge, VoiceOver+Safari) and with real users; behavior varies. [7][1]
If you want, I can:
- review a snippet of your implementation and point out specific fixes, or
- provide a tiny JS utility that manages an accessible live region with debounce, clearing, and proper attributes.
@antm-rp You've hit a well-documented issue with ARIA live regions! The inconsistency is caused by several timing and configuration problems in the current implementation. Based on accessibility best practices, here's what's going wrong and how to fix it:
Root Causes
- Duplicate live regions: Having
aria-live='polite'on both the Box and the VisuallyHidden creates competing announcements - Rapid updates: When users type quickly,
items.lengthchanges rapidly with no debouncing, causing screen readers to skip announcements - aria-busy interference: The
aria-busy={isLoading}on the same element as the live region can suppress announcements
Solution
Here's a robust implementation following WCAG best practices:
// Add to imports at the top
import { VisuallyHidden } from 'react-aria';
// Inside the SearchList component, replace the existing announcement logic:
// Debounce the items length to avoid rapid-fire announcements
const debouncedItemsLength = useDebouncedValue(items.length, 400);
const [announcementText, setAnnouncementText] = useState('');
useEffect(() => {
// Update announcement only after debounced stabilization
setAnnouncementText(t('Search_results', { count: debouncedItemsLength }));
}, [debouncedItemsLength, t]);
// In the return statement:
<Box
ref={boxRef}
role='listbox'
id={listId}
tabIndex={-1}
flexShrink={1}
h='full'
w='full'
data-qa='sidebar-search-result'
// Remove aria-live, aria-atomic, aria-busy from here
onClick={handleClick}
>
{/* Single dedicated live region */}
<VisuallyHidden role='status'>
{announcementText}
</VisuallyHidden>
<VirtualizedScrollbars>
<Virtuoso
style={{ height: '100%', width: '100%' }}
totalCount={items.length}
data={items}
computeItemKey={(_, room) => room._id}
itemContent={(_, data): ReactElement => <Row data={itemData} item={data} />}
ref={listRef}
/>
</VirtualizedScrollbars>
</Box>Key Changes
- Single live region: Only the
VisuallyHiddenwithrole='status'acts as the live region (role="status" implies aria-live="polite") - Debouncing: 400ms debounce on
items.lengthprevents rapid updates from overwhelming the screen reader - No aria-busy: Removed from the live region container to avoid suppression
- Separation of concerns: The Box remains a semantic listbox; the VisuallyHidden handles announcements
This approach ensures screen readers reliably announce the count after the user stops typing, without conflicting regions or timing issues.
Test this with a 1-2 second pause after typing to confirm announcements work consistently.
… performing a search based on room names. Also add support in new feature interface with top navbar.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/NavBarV2/NavBarSearch/NavBarSearchListbox.tsx (2)
60-62: Remove redundant aria-live attributes to prevent conflicts.The parent
Tilecontainer hasaria-live='polite'(lines 60-62), and theVisuallyHiddenchildren also declarearia-live='polite'(lines 68-70, 76-78). Nested live regions can cause duplicate announcements or interfere with each other.Since the
VisuallyHiddencomponents are specifically designed for announcements, remove the aria-live attributes from the parent container.Apply this diff:
<Tile ref={containerRef} position='absolute' zIndex={99} padding={0} pb={16} mbs={4} minHeight='x52' maxHeight='50vh' display='flex' width='100%' flexDirection='column' - aria-live='polite' - aria-atomic='true' aria-busy={isLoading} >Also applies to: 68-70, 76-78
65-87: Add dynamic key to force remount for updated announcements.According to the AI summary,
SearchList.tsxuses "a unique key on the results container derived from items.length to trigger re-announcements for count changes," but this file lacks that pattern. Without a key that changes withitems.length, the container won't remount when the count changes, and screen readers may not re-announce updated counts (e.g., when results change from 5 to 3).Apply this diff to add a dynamic key:
- <div {...overlayProps} role='listbox' aria-label={t('Channels')} tabIndex={-1} onKeyDown={handleKeyDown}> + <div key={items.length} {...overlayProps} role='listbox' aria-label={t('Channels')} tabIndex={-1} onKeyDown={handleKeyDown}>
🧹 Nitpick comments (1)
apps/meteor/client/NavBarV2/NavBarSearch/NavBarSearchListbox.tsx (1)
40-45: Remove redundant undefined check.The condition
items?.length !== undefinedis unnecessary sinceitemsdefaults to[]on line 38, makingitems.lengthalways defined.Apply this diff:
const [announcementText, setAnnouncementText] = useState(''); useEffect(() => { - if (items?.length !== undefined) { - setAnnouncementText(t('Search_results', { count: items.length })); - } + setAnnouncementText(t('Search_results', { count: items.length })); }, [items?.length, t]);
📜 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 (1)
apps/meteor/client/NavBarV2/NavBarSearch/NavBarSearchListbox.tsx(3 hunks)
🔇 Additional comments (1)
apps/meteor/client/NavBarV2/NavBarSearch/NavBarSearchListbox.tsx (1)
3-5: LGTM!The added imports are appropriate for implementing screen reader announcements.
…of items. The "No results found" is not always read.
1 participant
fix: Add screen reader support for reading the number of results when performing a search based on room names.
Proposed changes (including videos or screenshots)
I have added support for screen readers to announce the number of results when performing a search in the left hand room search function. This is done by the addition of two new tags.
Issue(s)
Steps to test or reproduce
Perform a search, verify that screen reader properly announce the same result as visually presented.
Further comments
Summary by CodeRabbit
New Features
Localization