Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runfix: allow keyboard navigation in full screen device select (WPB-6075) #16295

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

V-Gira
Copy link
Contributor

@V-Gira V-Gira commented Nov 29, 2023

BugWPB-6075 [Web] Cannot use keyboard navigation to select devices in call ui

Description

Arrow navigation doesn't work to select a device in the new select in full screen calls

Checklist

  • PR has been self reviewed by the author;
  • Hard-to-understand areas of the code have been commented;
  • If it is a core feature, unit tests have been added;

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (6e72b1f) 0.00% compared to head (c6a32f0) 45.42%.
Report is 12 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff            @@
##           dev   #16295       +/-   ##
========================================
+ Coverage     0   45.42%   +45.42%     
========================================
  Files        0      740      +740     
  Lines        0    24218    +24218     
  Branches     0     5504     +5504     
========================================
+ Hits         0    11000    +11000     
- Misses       0    11806    +11806     
- Partials     0     1412     +1412     

@V-Gira V-Gira force-pushed the virgile/keyboard-nav-av-settings branch from eb1f320 to 7208811 Compare January 15, 2024 11:55
@@ -465,6 +470,8 @@ const FullscreenVideoCall: React.FC<FullscreenVideoCallProps> = ({
{audioOptionsOpen ? (
<>
<Select
// eslint-disable-next-line jsx-a11y/no-autofocus
autoFocus
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autoFocus is needed foe arrow navigation to function in the menu

Comment on lines 456 to 462
onClick={() => {
setAudioOptionsOpen(prev => !prev);
onClick={event => {
const target = event.target as Element;
if (!target.closest('#select-microphone')) {
setAudioOptionsOpen(prev => !prev);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prevents the onclick event to trigger in the select menu
It would trigger when selecting an option right after setting the optionsOpen state of the menu to false, leaving the menu open

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you cannot toggle the button anymore? How do you close it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add the code comment to make that clearer
The select menu (#select-microphone) is part of the button.
If it's not the target of the click event (when the actual dropdown arrow is clicked), the toggle works normally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this clear things up?
// We want to ensure to only toggling the menu open or closed when clicking the icon, not the select menu

@@ -238,6 +239,7 @@ const FullscreenVideoCall: React.FC<FullscreenVideoCallProps> = ({
const camera = videoOptions[0].options.find(item => item.value === selectedOption) ?? selectedVideoOptions[0];
setSelectedVideoOptions([camera]);
switchCameraInput(camera.id);
setVideoOptionsOpen(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set the optionsOpen state to false here in the updateOptions method instead of toggling it off on the click event

@V-Gira V-Gira marked this pull request as ready for review January 15, 2024 17:10
@V-Gira V-Gira requested review from otto-the-bot and a team as code owners January 15, 2024 17:10
@V-Gira V-Gira changed the title runfix: allow keyboard navigation in full screen device select runfix: allow keyboard navigation in full screen device select (WPB-6075) Jan 15, 2024
Copy link
Contributor

@atomrc atomrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@V-Gira V-Gira merged commit 2378bea into dev Jan 16, 2024
10 checks passed
@V-Gira V-Gira deleted the virgile/keyboard-nav-av-settings branch January 16, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants