fix(fcc): Leave meeting button accessibility issue. #847
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
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 and nitpick comments (1)
src/components/WebexMeetingControl/WebexMeetingControl.jsx (1)
Line range hint
38-75: Consider comprehensive accessibility improvements.While this fix addresses the "Leave meeting" button, consider enhancing other accessibility aspects:
- The
hintproperty is used for ariaLabel in the "asItem" case but not in the regular button case- The pressed state for toggle buttons could benefit from an aria-pressed announcement
Consider this enhancement:
<Button className={sc('control-button')} type={controlTypeToButtonType[type] || 'default'} size={48} isDisabled={isDisabled} onClick={action} - ariaLabel={text || tooltip} + ariaLabel={hint || text || tooltip} pressed={isActive && type === 'TOGGLE'} + aria-pressed={isActive && type === 'TOGGLE'} tooltip={tooltip} autoFocus={autoFocus} tabIndex={tabIndex} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/WebexMeetingControl/WebexMeetingControl.jsx(1 hunks)
🔇 Additional comments (2)
src/components/WebexMeetingControl/WebexMeetingControl.jsx (2)
67-67: LGTM! Accessibility improvement for screen readers.
The change ensures that screen readers always have meaningful text to announce by falling back to the tooltip when text is not provided. This fixes the "undefined, button" announcement issue.
67-67: Verify edge case: both text and tooltip undefined.
While the change improves accessibility, let's verify that either text or tooltip is always defined for all control types to prevent potential "undefined" announcements.
✅ Verification successful
Both text and tooltip are safely handled in all cases
Looking at the complete component implementation, we can verify that:
- For regular buttons, if both
textandtooltipare undefined, the button won't be rendered at all because thedisplayobject is checked first:if (!display || Object.keys(display).length === 0) { output = ''; } - For item buttons,
textis always rendered in the span element, so it must be provided by the adapter - The
ariaLabelfallbacktext || tooltipis safe because:- For regular buttons, at least one must exist for the button to be meaningful
- For item buttons, a different fallback is used:
hint || text
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for control definitions to verify text/tooltip presence
ast-grep --pattern 'display = {
$$$
text: $_,
tooltip: $_,
$$$
}'
# Search for direct Button usage with potential missing text/tooltip
ast-grep --pattern '<Button
$$$
text={$_}
tooltip={$_}
$$$
>'
Length of output: 147
Script:
#!/bin/bash
# Let's try a different approach to find control definitions and usages
# Search for text/tooltip prop definitions in the component
rg -U "text.*tooltip|tooltip.*text" src/components/WebexMeetingControl/WebexMeetingControl.jsx
# Search for Button usages in the component
rg -U "<Button.*?>" -A 5 src/components/WebexMeetingControl/WebexMeetingControl.jsx
# Check the control types and their properties
rg -U "const.*Control.*=.*{" -A 10 src/components/WebexMeetingControl/WebexMeetingControl.jsx
Length of output: 418
Script:
#!/bin/bash
# Let's fix the regex and try again
# Search for control definitions and their properties
rg -U "const.*Control.*=" -A 10 src/components/WebexMeetingControl/WebexMeetingControl.jsx
# Look for Button component usage with more context
rg -U "<Button" -A 5 src/components/WebexMeetingControl/WebexMeetingControl.jsx
# Search for text and tooltip prop assignments
rg "text[=:]|tooltip[=:]" src/components/WebexMeetingControl/WebexMeetingControl.jsx
Length of output: 663
Script:
#!/bin/bash
# Let's look for the props being passed to the component and how they're used
# Search for the component's prop definitions and destructuring
rg -U "WebexMeetingControl.*=.*{" -A 5 src/components/WebexMeetingControl/WebexMeetingControl.jsx
# Look for where text and tooltip are defined/assigned
rg -U "(text|tooltip)\s*=\s*" -A 2 src/components/WebexMeetingControl/WebexMeetingControl.jsx
# Check for default props or prop types if any
rg -U "(defaultProps|propTypes)" -A 5 src/components/WebexMeetingControl/WebexMeetingControl.jsx
Length of output: 960
Script:
#!/bin/bash
# Let's check the entire component file content to understand the full context
# Get the full component file content
cat src/components/WebexMeetingControl/WebexMeetingControl.jsx
# Also check for any test files that might show how the component is used
fd "WebexMeetingControl.test" --exec cat {}
Length of output: 5261
adhmenon
left a comment
adhmenon
left a comment
There was a problem hiding this comment.
Approving as of now, as it does make sense to use tooltip as a workaround, as it won't change.
However, as discussed offline, please do create a ticket in tech debt to investigate why the text & hint are coming as undefined. The issue needs to be checked separately.
|
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-587755 for further investigation |
## [1.276.1](v1.276.0...v1.276.1) (2024-12-03) ### Bug Fixes * **fcc:** Leave meeting button accessibility issue. ([#847](#847)) ([1cdf594](1cdf594))
3 participants
Issue:
SPARK-564414
In case of accessibility, when we were over the
Leave meetingbutton, the screen reader says "undefined, button"Fix
Vidcast of testing:
https://app.vidcast.io/share/5a101376-0ffd-4881-bcd8-2d41c1e746c3
Manual Tests Performed
Join MeetingbuttonSummary by CodeRabbit
New Features
Bug Fixes