fix: Add screen reader support to read encrypted channel icon #37284
Conversation
|
|
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 |
antm-rp
changed the title
WalkthroughUpdated the Encrypted room header icon with improved accessibility attributes (ARIA labels, roles, tabindex) and changed icon color from g500 to g800. Added a new localization string for displaying encrypted room status with the room name. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (1)
apps/meteor/client/views/room/Header/icons/Encrypted.tsx (1)
17-17: Remove redundantaria-hidden={false}.Setting
aria-hidden={false}is redundant since elements are visible to assistive technologies by default unless explicitly hidden. This line can be safely removed.Apply this diff:
<HeaderState title={t('Encrypted')} icon='key' color={colors.g800} - aria-hidden={false} aria-live='polite' role='img' tabindex={0} aria-label={t('Encrypted_channel', { roomName: room.name })} />
📜 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/Header/icons/Encrypted.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🔇 Additional comments (1)
packages/i18n/src/locales/en.i18n.json (1)
1931-1931: LGTM — new string fits the existing pattern.The added localization key reuses the
{{roomName}}placeholder consistently with neighboring entries. No further changes needed.
| return e2eEnabled && room?.encrypted ? ( | ||
| <HeaderState | ||
| title={t('Encrypted')} | ||
| icon='key' | ||
| color={colors.g800} | ||
| aria-hidden={false} | ||
| aria-live='polite' | ||
| role='img' | ||
| tabindex={0} | ||
| aria-label={t('Encrypted_channel', { roomName: room.name })} | ||
| /> | ||
| ) : null; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that HeaderState component supports ARIA props.
Ensure that the HeaderState component properly forwards the ARIA attributes (aria-hidden, aria-live, role, aria-label) to the underlying DOM element. If these props aren't supported, they won't have any effect on accessibility.
Run the following script to check the HeaderState component implementation:
🏁 Script executed:
#!/bin/bash
# Description: Locate and examine the HeaderState component to verify ARIA prop support
# Find the HeaderState component file
fd -t f "HeaderState" -e ts -e tsx -e js -e jsx | head -5
# Search for the component definition and prop spreading
rg -A 20 "export.*HeaderState" --type tsx --type tsLength of output: 196
🏁 Script executed:
#!/bin/bash
# Find the import statement in Encrypted.tsx to identify which HeaderState is used
cat apps/meteor/client/views/room/Header/icons/Encrypted.tsxLength of output: 750
🏁 Script executed:
#!/bin/bash
# Examine the HeaderState component that is imported in Encrypted.tsx
cat apps/meteor/client/components/Header/HeaderState.tsxLength of output: 563
🏁 Script executed:
#!/bin/bash
# Examine the underlying HeaderStateComponent from ui-client
cat packages/ui-client/src/components/Header/HeaderState.tsx
echo "---"
# Also examine HeaderV2State
cat packages/ui-client/src/components/HeaderV2/HeaderState.tsxLength of output: 1465
Fix the tabindex prop to use camelCase tabIndex.
The HeaderState component properly forwards ARIA attributes (aria-hidden, aria-live, role, aria-label) through prop spreading and its AllHTMLAttributes type definition. However, in the Encrypted component, the attribute is incorrectly specified as tabindex={0} instead of tabIndex={0}. React normalizes HTML attributes to camelCase, so the lowercase version will not be applied to the DOM element.
tabIndex={0}
🤖 Prompt for AI Agents
In apps/meteor/client/views/room/Header/icons/Encrypted.tsx around lines 12 to
23, the JSX uses the incorrect lowercase prop `tabindex={0}` which React will
not apply to the DOM; change it to the camelCase `tabIndex={0}` so the attribute
is forwarded and applied correctly (no other changes needed).
| aria-hidden={false} | ||
| aria-live='polite' | ||
| role='img' | ||
| tabindex={0} |
There was a problem hiding this comment.
Remove tabindex={0} from non-interactive icon.
Adding tabindex={0} makes this static icon keyboard-focusable, creating an unnecessary tab stop. Screen readers can already access elements with role='img' and aria-label without requiring keyboard focus. This degrades the keyboard navigation experience by adding a focusable element that has no associated action.
Apply this diff to remove the tabindex:
<HeaderState
title={t('Encrypted')}
icon='key'
color={colors.g800}
aria-hidden={false}
aria-live='polite'
role='img'
- tabindex={0}
aria-label={t('Encrypted_channel', { roomName: room.name })}
/>📝 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.
| tabindex={0} | |
| <HeaderState | |
| title={t('Encrypted')} | |
| icon='key' | |
| color={colors.g800} | |
| aria-hidden={false} | |
| aria-live='polite' | |
| role='img' | |
| aria-label={t('Encrypted_channel', { roomName: room.name })} | |
| /> |
🤖 Prompt for AI Agents
In apps/meteor/client/views/room/Header/icons/Encrypted.tsx around line 20,
remove the unnecessary tabindex={0} on the static icon element so it is not
included in the keyboard tab order; keep role="img" and aria-label for screen
reader access and ensure no other interactive handlers require keyboard
focus—just delete the tabindex prop from the JSX.
3 participants
fix: Add screen reader support to read encrypted channel icon
Proposed changes (including videos or screenshots)
Add support for screen readers to properly announce the encrypted icon indicating a channel is encrypted.
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Style
New Features