WIP: initial screen reader improvements for comments-ui #22260
Conversation
WalkthroughThis pull request applies accessibility attribute updates across several components in the comments user interface. In the Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/comments-ui/src/components/Frame.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-i18next". (The package "eslint-plugin-i18next" was not found when loaded as a Node module from the directory "/apps/comments-ui".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-i18next" was referenced from the config file in "apps/comments-ui/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/comments-ui/src/components/content/buttons/LikeButton.tsx (1)
41-50: 🛠️ Refactor suggestionEnhance the aria-label to provide more context.
The current aria-label only announces the like count. Consider providing more context about the button's purpose and state.
<button - aria-label={`${comment.count.likes} likes`} + aria-label={`${comment.liked ? 'Unlike' : 'Like'} this comment - ${comment.count.likes} ${comment.count.likes === 1 ? 'like' : 'likes'}`} + aria-pressed={comment.liked} className={`duration-50 group flex cursor-pointer items-center font-sans text-base outline-0 transition-all ease-linear sm:text-sm ${ comment.liked ? 'text-black/90 dark:text-white/90' : 'text-black/50 hover:text-black/75 dark:text-white/60 dark:hover:text-white/75' }`} data-testid="like-button" disabled={disabled} type="button" onClick={toggleLike}
🧹 Nitpick comments (4)
apps/comments-ui/src/components/content/forms/SortingForm.tsx (1)
51-59: Great addition of the aria-label! Consider these additional accessibility improvements.The aria-label addition is a good start for screen reader accessibility. Consider these additional improvements to enhance the experience:
<button aria-label="Sort comments by" + aria-expanded={isOpen} + aria-haspopup="menu" className="flex w-full items-center justify-between gap-2 text-sm font-medium text-neutral-900 focus-visible:outline-none dark:text-neutral-100" type="button" onClick={() => setIsOpen(!isOpen)} > {options.find(option => option.value === selectedOption)?.label} - <span className="h-2 w-2 stroke-[3px]"><ChevronIcon /></span> + <span className="h-2 w-2 stroke-[3px]" aria-hidden="true"><ChevronIcon /></span> </button>These changes will:
- Indicate the button's expanded state to screen readers
- Signal that the button opens a menu
- Hide the decorative chevron icon from screen readers
apps/comments-ui/src/components/content/buttons/MoreButton.tsx (1)
32-35: Consider additional accessibility improvements for the button and menu.While the aria-label is a good start, consider these additional accessibility enhancements:
- Add
aria-hidden="true"to the MoreIcon SVG since it's decorative- Add
aria-expanded={isContextMenuOpen}to the button to indicate menu state- Add
aria-haspopup="menu"to indicate that the button opens a menuApply this diff to enhance the accessibility:
- <button className="outline-0" type="button" onClick={toggleContextMenu} aria-label="More options"> - <MoreIcon className={`duration-50 gh-comments-icon gh-comments-icon-more outline-0 transition ease-linear hover:fill-black/75 dark:hover:fill-white/75 ${isContextMenuOpen ? 'fill-black/75 dark:fill-white/75' : 'fill-black/50 dark:fill-white/60'}`} /> + <button + className="outline-0" + type="button" + onClick={toggleContextMenu} + aria-label="More options" + aria-expanded={isContextMenuOpen} + aria-haspopup="menu" + > + <MoreIcon + aria-hidden="true" + className={`duration-50 gh-comments-icon gh-comments-icon-more outline-0 transition ease-linear hover:fill-black/75 dark:hover:fill-white/75 ${isContextMenuOpen ? 'fill-black/75 dark:fill-white/75' : 'fill-black/50 dark:fill-white/60'}`} + />apps/comments-ui/src/components/content/buttons/LikeButton.tsx (1)
51-55: Add aria attributes to the SVG icon.The SVG icon should be hidden from screen readers since it's decorative and the button's purpose is already conveyed through the aria-label.
<LikeIcon + aria-hidden="true" + role="presentation" className={animationClass + ` mr-[6px] ${ comment.liked ? 'fill-black dark:fill-white stroke-black dark:stroke-white' : 'stroke-black/50 group-hover:stroke-black/75 dark:stroke-white/60 dark:group-hover:stroke-white/75' } ${!comment.liked && canLike && 'group-hover:stroke-black/75 dark:group-hover:stroke-white/75'} transition duration-50 ease-linear`}apps/comments-ui/src/components/content/Avatar.tsx (1)
85-85: Add an empty alt attribute for better accessibility.While
role="presentation"correctly indicates this is a decorative image, adding an emptyalt=""attribute is considered a best practice for decorative images in HTML5.- {activeMember?.avatar_image && <img role="presentation" className={`absolute left-0 top-0 rounded-full ${dimensionClasses}`} data-testid="avatar-image" src={activeMember.avatar_image} />} + {activeMember?.avatar_image && <img role="presentation" alt="" className={`absolute left-0 top-0 rounded-full ${dimensionClasses}`} data-testid="avatar-image" src={activeMember.avatar_image} />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/comments-ui/src/components/content/Avatar.tsx(1 hunks)apps/comments-ui/src/components/content/buttons/LikeButton.tsx(1 hunks)apps/comments-ui/src/components/content/buttons/MoreButton.tsx(1 hunks)apps/comments-ui/src/components/content/forms/SortingForm.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Comments-UI tests
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Lint
- GitHub Check: i18n
🔇 Additional comments (5)
apps/comments-ui/src/components/content/forms/SortingForm.tsx (1)
61-77: Well-structured dropdown menu with proper ARIA attributes!The dropdown menu implementation follows accessibility best practices with appropriate ARIA roles and attributes. The use of semantic buttons for menu items is also commendable.
apps/comments-ui/src/components/content/buttons/MoreButton.tsx (1)
32-32: Good addition of aria-label for screen reader accessibility!The descriptive aria-label "More options" helps screen reader users understand the button's purpose.
apps/comments-ui/src/components/content/Avatar.tsx (3)
80-80: LGTM! Good use of aria-hidden.The
aria-hidden="true"attribute correctly prevents screen readers from announcing the decorative initials text, avoiding redundant information.
90-90: LGTM! Appropriate use of presentation role.The
role="presentation"on the figure element correctly ensures the entire avatar container is treated as decorative, maintaining consistency with its children.
76-93:❓ Verification inconclusive
Verify screen reader behavior.
While the accessibility attributes are correctly implemented, it would be valuable to verify the behavior with popular screen readers (NVDA, VoiceOver, JAWS) to ensure they interpret these changes as intended.
🌐 Web query:
What are the recommended screen readers for testing web accessibility?💡 Result:
The recommended screen readers for web accessibility testing prioritize coverage of major user bases and compatibility with common operating systems and browsers. Based on current best practices and usage statistics:
Primary Screen Readers
NVDA (NonVisual Desktop Access)
- Platform: Windows (free/open-source)
- Key Pairing: Chrome or Firefox
- Strengths: Catches ~90% of compatibility issues[4][6]. Ideal for developers due to cost-effectiveness and portability[4][8].
JAWS (Job Access With Speech)
- Platform: Windows (paid)
- Key Pairing: Chrome or Edge
- Strengths: Most widely used by blind users (53.7% primary usage)[6]. Critical for enterprise-level compliance due to its dominance in professional settings[1][8].
VoiceOver
- Platform: macOS/iOS (built-in)
- Key Pairing: Safari
- Strengths: Essential for Apple ecosystem testing[1][4][8].
TalkBack
- Platform: Android (built-in)
- Key Pairing: Chrome
- Strengths: Mandatory for mobile accessibility testing[2][8].
Testing Best Practices
Browser Pairings: Test screen readers with their most common browser counterparts:
Screen Reader Recommended Browsers NVDA Chrome, Firefox JAWS Chrome, Edge VoiceOver Safari TalkBack Chrome (Android) Coverage: Prioritize NVDA + Chrome first to address ~64% of users[4][8], then expand to JAWS + Edge/Chrome and VoiceOver + Safari.
Checklist Compliance: Ensure all content passes criteria like alt text for images, logical heading hierarchy, keyboard navigation, and ARIA label support[1][2][10].
Why These Tools?
- Usage Statistics: JAWS (53.7%) and NVDA (30.7%) dominate desktop usage[6], while VoiceOver/TalkBack are critical for mobile[8].
- Cost Efficiency: NVDA’s free model makes it accessible for routine testing[1][6].
- Legal Alignment: These tools align with WCAG 2.1/2.2 requirements for semantic HTML and dynamic content accessibility[2][5].
For comprehensive compliance, combine automated tools (e.g., BrowserStack Accessibility Toolkit) with manual testing across these screen reader/browser pairs[2][5].
Citations:
- 1: https://www.siteimprove.com/glossary/screen-reader-testing/
- 2: https://www.browserstack.com/guide/test-websites-with-screen-readers
- 3: https://beaccessible.com/post/screen-reader-accessibility-testing/
- 4: https://assistivlabs.com/articles/screen-reader-browser-pairing-stats
- 5: https://das.osu.edu/accessibility-testing-screen-readers
- 6: https://www.boia.org/blog/what-screen-readers-work-best-for-web-accessibility-testing
- 7: https://dequeuniversity.com/online-courses/descriptions/screenreaders
- 8: https://www.digitala11y.com/screen-readers-browsers-which-is-the-best-combination-for-accessibility-testing/
- 9: https://accessibility.huit.harvard.edu/nvda
- 10: https://doit.illinois.gov/initiatives/accessibility/testing/screen-reader-testing.html
Confirm Screen Reader Compatibility
The accessibility updates in the avatar component appear to comply with WCAG guidelines. However, it's important to verify that these changes behave as expected across popular screen readers. Please test the component using:
- NVDA on Windows (with Chrome or Firefox)
- JAWS on Windows (with Chrome or Edge)
- VoiceOver on macOS/iOS (with Safari)
- Optionally, TalkBack for Android environments to cover mobile accessibility
This manual testing will ensure that decorative elements and images are correctly handled and announced by the screen readers.
Adding missing roles and labels for comments-ui, hiding avatars, etc.
WIP, more to come, and I need an actual screen reader user to validate.