fix(accessibility) make tooltip component keyboard accessible #16333
Conversation
mihhu
force-pushed
the
wcag-tooltip
branch
2 times, most recently
from
07d4632 to
7833821
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds keyboard accessibility support to the Tooltip component to make it compliant with accessibility standards. The changes enable keyboard navigation, proper ARIA attributes, and focus management for tooltip interactions.
Key changes:
- Enhanced Tooltip component with keyboard event handlers and ARIA attributes
- Added focus/blur management with immediate tooltip display for keyboard users
- Updated ToolboxItem to properly integrate with the enhanced Tooltip component
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Tooltip.tsx | Added keyboard accessibility features including focus/blur handlers, ARIA attributes, and enhanced child element cloning |
| ToolboxItem.web.tsx | Refactored to properly render enhanced children within tooltip wrapper |
| FileSharing.tsx | Added Tooltip wrapper around file name display for better accessibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| open: 0, | ||
| close: 0 | ||
| }); | ||
| const tooltipId = useRef(`tooltip-${Math.random().toString(36).substring(2, 11)}`); |
There was a problem hiding this comment.
Using Math.random() for generating IDs can potentially cause collisions in applications with many tooltip instances. Consider using a more robust ID generation method like crypto.randomUUID() or a counter-based approach to ensure uniqueness.
| const enhancedChildren = cloneElement(children, { | ||
| 'aria-describedby': visible ? tooltipId.current : undefined, | ||
| tabIndex: children.props.tabIndex !== undefined ? children.props.tabIndex : 0, |
There was a problem hiding this comment.
Setting tabIndex to 0 for all children may interfere with the natural tab order for elements that shouldn't be focusable (like divs or spans). Consider checking if the child element is naturally focusable before setting tabIndex.
| const enhancedChildren = cloneElement(children, { | |
| 'aria-describedby': visible ? tooltipId.current : undefined, | |
| tabIndex: children.props.tabIndex !== undefined ? children.props.tabIndex : 0, | |
| // Helper to check if the child is natively focusable | |
| function isNaturallyFocusable(element: ReactElement): boolean { | |
| const focusableTags = [ | |
| 'a', | |
| 'button', | |
| 'input', | |
| 'select', | |
| 'textarea' | |
| ]; | |
| // If it's a string type (native element), check tag name | |
| if (typeof element.type === 'string') { | |
| if (focusableTags.includes(element.type)) { | |
| return true; | |
| } | |
| // <a> must have href to be focusable | |
| if (element.type === 'a' && element.props.href) { | |
| return true; | |
| } | |
| return false; | |
| } | |
| // For custom components, only set tabIndex if already present | |
| return element.props.tabIndex !== undefined; | |
| } | |
| const enhancedChildren = cloneElement(children, { | |
| 'aria-describedby': visible ? tooltipId.current : undefined, | |
| ...(isNaturallyFocusable(children) || children.props.tabIndex !== undefined | |
| ? { tabIndex: children.props.tabIndex !== undefined ? children.props.tabIndex : 0 } | |
| : {}), |
| onPopoverClose = { onPopoverClose } | ||
| onPopoverOpen = { onPopoverOpen } | ||
| position = { position } | ||
| role = 'tooltip' |
There was a problem hiding this comment.
The role='tooltip' attribute is being set on the Popover component, but tooltips already have role='tooltip' set on the content div (line 80). This creates duplicate role attributes which may confuse screen readers.
| role = 'tooltip' |
1 participant
No description provided.