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

fix: Display Media Device labels for Firefox #17562

Merged

Conversation

EnricoSchw
Copy link
Contributor

@EnricoSchw EnricoSchw commented Jun 12, 2024

Description

In Firefox, the labels of the Media devices can only be read under certain constrains. This means that the device names cannot be displayed in the select menu. This PR is a fix for it

Screenshots/Screencast (for UI changes)

Before this fix, the selection fields are displayed with blank labels or common names:

Bildschirmfoto 2024-06-12 um 14 06 46 Bildschirmfoto 2024-06-12 um 18 21 16

With this fix, the selection fields are displayed with device names:

Bildschirmfoto 2024-06-12 um 16 34 24 Bildschirmfoto 2024-06-12 um 18 21 58

Notes

The fix has one disadvantage. I had to change the device list initialization process to avoid asking a user for audio access rights on first login starting page.

Bildschirmfoto 2024-06-24 um 15 06 23

In order to display the correct device labels, we need to make an access authorization query when creating the device list. But we should not ask for microphone and camera access when not needed.

To avoid a permission query being displayed when starting the app, I have rewritten the MediaDeviceHandler and The device list is no longer created in the constructor. The method MediaDeviceHandler.initializeMediaDevices is now integrated into the three points in our app where a call can be accepted or started. In addition, this method is also called in the device settings. So, a device list is only created before the first call or when the settings are called.

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;

Important details for the reviewers

(Delete this section if unnecessary)

  • use (x) data
  • can be reviewed commit-by-commit
  • be sure to look at ...

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2024

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 36.53846% with 33 lines in your changes missing coverage. Please review.

Project coverage is 46.11%. Comparing base (c0c9c7f) to head (58675ea).
Report is 70 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #17562      +/-   ##
==========================================
- Coverage   46.11%   46.11%   -0.01%     
==========================================
  Files         760      760              
  Lines       25016    25034      +18     
  Branches     5721     5724       +3     
==========================================
+ Hits        11537    11545       +8     
- Misses      12036    12043       +7     
- Partials     1443     1446       +3     

@tlebon
Copy link
Contributor

tlebon commented Jun 12, 2024

does this change anything for chromium browsers?

@tlebon
Copy link
Contributor

tlebon commented Jun 12, 2024

thanks for taking the initiative on this!

@EnricoSchw
Copy link
Contributor Author

@tlebon Thx for your fast response

Unfortunately it also has an impact on Chrome. The device lists are created during the initialization process of the app, so that the access popup also appears in Chrome.

I see three solutions here:

  1. A browser switch for Firefox (could be a temporary solution)
  2. Only have a numbered device list for Firefox
  3. Only initialize the device list when it is needed

What do you think?

More detail:
Some users will click away the pop-up and not allow access. When the mic is needed for a call, the pop-up will no longer appear and the mic will be blocked. We will receive many support requests because the mic is not working.
This is very bad UX and that's why I marked the PR as draft.

@WolfiWire
Copy link

Do we have the possibility to create a custom dialog (that explains better why we are asking for these access rights), or are we bound to the browser default popup?

@EnricoSchw
Copy link
Contributor Author

@WolfiWire I would use this one.

Bildschirmfoto 2024-06-14 um 10 17 12

@EnricoSchw EnricoSchw changed the title Display Media Device labels for Firefox fix: Display Media Device labels for Firefox Jun 17, 2024
  - Now the devices are only queried before a call or when the settings are opened.
Copy link

sonarcloud bot commented Jun 24, 2024

@EnricoSchw EnricoSchw marked this pull request as ready for review June 24, 2024 13:25
@EnricoSchw EnricoSchw requested review from otto-the-bot and a team as code owners June 24, 2024 13:25
@EnricoSchw
Copy link
Contributor Author

@WolfiWire I have rewritten the device list initialization. This means that our app behaves the same for the user as before. Only internally the process is different.

@EnricoSchw
Copy link
Contributor Author

@tlebon Please, could someone take a look at this?

@tlebon
Copy link
Contributor

tlebon commented Jul 2, 2024

I'm still OoO (sick). Maybe @wireapp/web can take a look. Or else I will tomorrow.

@EnricoSchw
Copy link
Contributor Author

I'm still OoO (sick).

Get well soon!

Maybe @wireapp/web can take a look. Or else I will tomorrow.

Thx. sometime this week would be enough

@EnricoSchw EnricoSchw merged commit 97a5c39 into dev Jul 5, 2024
13 checks passed
@EnricoSchw EnricoSchw deleted the fix/WPB-9688-enumerate-media-device-label-for-firefox branch July 5, 2024 13:38
EnricoSchw added a commit that referenced this pull request Jul 5, 2024
EnricoSchw added a commit that referenced this pull request Jul 5, 2024
EnricoSchw added a commit that referenced this pull request Jul 5, 2024
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.

None yet

7 participants