feat(meetings-widgets): fixed-join-meeting-acc #846
Conversation
WalkthroughThe changes in this pull request focus on the Changes
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
41-43: Inconsistent accessibility label pattern detected.While the main button implementation now uses just
textfor ariaLabel, the item rendering path still useshint || text. This inconsistency should be addressed for uniform accessibility behavior.Apply this diff to maintain consistency:
className={sc('as-item')} tooltip={tooltip} - ariaLabel={hint || text} + ariaLabel={text}
📜 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 the Join Meeting button.
The change from hint || text to text aligns with the PR objective to fix the incorrect hint display issue.
67-67: Verify impact on other control types.
The removal of the hint fallback might affect other control types that rely on hints for accessibility.
✅ Verification successful
Let me gather more specific information about the accessibility attributes and control types.
Let me search for the WebexMeetingControl component and its usage to verify the impact of the ariaLabel change.
Let me search specifically for the hint usage in the WebexMeetingControl component.
Based on the search results, I can now provide a definitive response about the ariaLabel change:
No impact on accessibility - hint fallback is properly maintained.
The code shows that the hint fallback is still preserved in two ways:
- For item controls:
ariaLabel={hint || text} - For button controls:
ariaLabel={text} - For dropdown controls:
ariaLabel={hint}
The component maintains proper accessibility by using either the hint or text as appropriate for different control types. The change in question doesn't remove the hint fallback functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all control types and their usage patterns to ensure the ariaLabel change doesn't negatively impact other controls
# Search for control type definitions and their accessibility-related properties
ast-grep --pattern 'type === "$_"'
# Search for hint usage patterns in test files
fd -e test.jsx -e test.js --exec rg -A 5 'hint:' {}
Length of output: 837
Script:
#!/bin/bash
# Search for ariaLabel usage in the codebase
rg -g '*.{jsx,js}' -B 2 -A 2 'ariaLabel'
# Search specifically for WebexMeetingControl related files and their tests
fd -e jsx -e js WebexMeetingControl --exec cat {}
Length of output: 112806
Script:
#!/bin/bash
# Search for WebexMeetingControl component implementation and its usage
rg -g '*.{jsx,js}' -B 3 -A 3 'WebexMeetingControl'
# Search for hint usage in WebexMeetingControl
rg -g '*.{jsx,js}' -B 2 -A 2 'hint.*WebexMeetingControl'
Length of output: 147152
Script:
#!/bin/bash
# Search for hint usage in WebexMeetingControl files
rg -g 'WebexMeetingControl*' -B 5 -A 5 'hint'
# Search for ariaLabel usage in WebexMeetingControl
rg -g 'WebexMeetingControl*' -B 5 -A 5 'ariaLabel'
Length of output: 7778
There was a problem hiding this comment.
Looks good.
@pagour98 I guess this would cause a conflict with your changes
| @@ -64,7 +64,7 @@ function renderButton(sc, action, display, style, showText, asItem, autoFocus, t | |||
| size={48} | |||
| isDisabled={isDisabled} | |||
| onClick={action} | |||
| ariaLabel={hint || text} | |||
| ariaLabel={text} | |||
There was a problem hiding this comment.
What were the hints doing earlier. Since we are removing hint, do we know what else gets affected?
There was a problem hiding this comment.
Hints were messing up the accessibility - it was saying Mute, video off and that messed up the acessibility. I didn't remove it anywhere else because they use the hint to check if the status changes and then keep refreshing the buttons. Here it is simply removed so accessibility is fixed.
There was a problem hiding this comment.
@adhmenon This Button element has been used everywhere for example (Mute, Unmute, ShareScreen, Show Participant, Settings, Leave meeting), Can you check once these buttons as well after joining a meeting?
There was a problem hiding this comment.
Let's resolve this after @adhmenon replies.
There was a problem hiding this comment.
Thanks for pointing out @pagour98 and @sreenara . Yes, it works even with all buttons. I checked the older behaviour and it is the same for all buttons (except ofc the join meeting one).
I think they were not setting hint for most buttons and for join meeting they were but this causes an issue in the voiceover. I think it has not broken anything else.
Here is a Vidcast showing the proof.
# [1.276.0](v1.275.3...v1.276.0) (2024-11-27) ### Features * **meetings-widgets:** fixed-join-meeting-acc ([#846](#846)) ([a1e9183](a1e9183))
|
🎉 This PR is included in version 1.276.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Issue
SPARK-564413
In case of accessibility, when we were over the
Join Meetingbutton, it incorrectly said the hint - 'mute, video on' and not the text.Fix
GIF
Vidcast of working of buttons
https://app.vidcast.io/share/ae43d3c3-76ab-4c20-8dc9-47f6fb4e9ce3
Manual Tests
Join Meetingbutton.Summary by CodeRabbit