fix: media shouldn't open permissions dialog #31805

indutny · 2021-11-12T09:39:09Z

Description of Change

Playing media shouldn't open Accessibility permissions dialog on macOS.
However, we still need to watch for media events, just not globally and
media_keys_listener_ is an API over global capture of the media keys.

The fix is to let chromium call UpdateWhichKeysAreListenedFor which
will call UpdateSystemMediaControlsEnabledControls and watch for
events on system_media_controls_ without triggering permissions popup.

Fix: #31558

cc @codebytere

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed an issue where playing media would open Accessibility permissions dialog on macOS.

indutny · 2021-11-12T09:40:38Z

FWIW, I tested it on the test case from #31492 and our app. Without use of globalShortcut the playback menu on macOS looks like it did in #31492:

and with globalShortcut I get accessibility dialog and the shortcut is working.

indutny · 2021-11-12T09:42:16Z

cc @zcbenz @MarshallOfSound @VerteDinde @jkleinsc @indutny-signal

indutny · 2021-11-12T09:43:21Z

It'd be great if backport to 15-x-y could be released with this change. Our weekly release pipeline is currently blocked because of this issue 😭

Playing media shouldn't open Accessibility permissions dialog on macOS. However, we still need to watch for media events, just not globally and `media_keys_listener_` is an API over global capture of the media keys. The fix is to let chromium call `UpdateWhichKeysAreListenedFor` which will call `UpdateSystemMediaControlsEnabledControls` and watch for events on `system_media_controls_` without triggering permissions popup.

indutny-signal · 2021-11-12T10:11:00Z

~~I think I messed something up while submitting the patch. Please give me few moments to figure it out. Sorry for the noise!~~

Nevermind, I forgot that globalShortcut won't ask for permission by itself. The code path was guarded by presence of permission and so I had to turn it on manually in order for it to work!

indutny · 2021-11-12T10:50:32Z

Added update to the patches as suggested by CircleCI.

indutny · 2021-11-12T13:24:28Z

FWIW, 13-x-y might need the back port too. We don’t use it at our company anymore, but the issue is present there as well.

thanks!

VerteDinde

Thanks for the PR, @indutny!

I pulled the changes locally and tested - this patch change looks good to me, but I'd like a second opinion from another maintainer with a little more experience with the media APIs before we merge 👍

indutny-signal · 2021-11-12T22:10:35Z

patches/chromium/fix_media_key_usage_with_globalshortcuts.patch

+#endif  // defined(OS_MAC)
+      should_start_watching &&
+      media_keys_listener_ &&
+       !media_keys_listener_->StartWatchingMediaKey(key_code)) {


media_keys_listener_ is implemented by ui/base/accelerators/media_keys_listener_mac.mm and it provides API for listening for media keys globally. In fact, the code in media_kes_listener_mac.mm calls CGEventTapCreate whenever new media key is watched and this immediately causes an Accessibility popup to appear since this permission is required for CGEventTapCreate to function at all.

indutny-signal · 2021-11-12T22:11:44Z

patches/chromium/fix_media_key_usage_with_globalshortcuts.patch

+-    media_keys_listener_ = ui::MediaKeysListener::Create(
+-        this, ui::MediaKeysListener::Scope::kGlobal);
+-    DCHECK(media_keys_listener_);
+-  }
 +  // This is required for proper functioning of MediaMetadata.
 +  system_media_controls_->AddObserver(this);


The trick thus is to let system_media_controls_ handle the keystroke whenever we have internal handling enabled (i.e. when there are no global accelerators assigned for a media key in electron), so we check media_key_handling_enabled_ above and fall through here if it is false (meaning that there are no accelerators in electron).

indutny-signal · 2021-11-12T22:12:10Z

Thanks @VerteDinde ! I added a few comments to clarify what I've done in the patch. Hope it helps!

indutny-signal · 2021-11-15T08:06:45Z

Thanks for reviews, everyone!

@codebytere is there a chance you could take a look at this? You've submitted the original fix so I'd appreciate a review from you a lot 🤗

indutny-signal · 2021-11-15T12:57:42Z

Fantastic, thanks @codebytere ! Please let me know if there's any additional action that I should take before this commit can be merged and backported. I'll be happy to help in any way (in part because we wish to get the update with the next 15-x-y release 😁)

release-clerk · 2021-11-15T13:16:21Z

Release Notes Persisted

Fixed an issue where playing media would open Accessibility permissions dialog on macOS.

trop · 2021-11-15T13:16:43Z

I have automatically backported this PR to "15-x-y", please check out #31836

trop · 2021-11-15T13:16:49Z

I have automatically backported this PR to "16-x-y", please check out #31837

indutny-signal · 2021-11-15T13:16:56Z

Yaaay! Thank you!

ArmelChesnais · 2021-12-21T19:26:26Z

Could this be backported to 14? 14.2.1 displays the same issue.

* fix: media shouldn't open permissions dialog Playing media shouldn't open Accessibility permissions dialog on macOS. However, we still need to watch for media events, just not globally and `media_keys_listener_` is an API over global capture of the media keys. The fix is to let chromium call `UpdateWhichKeysAreListenedFor` which will call `UpdateSystemMediaControlsEnabledControls` and watch for events on `system_media_controls_` without triggering permissions popup. * chore: update patches Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

indutny requested a review from a team as a code owner November 12, 2021 09:39

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 12, 2021

indutny force-pushed the fix/gh-31558 branch from f9e9023 to 84d3493 Compare November 12, 2021 09:49

MarshallOfSound added target/15-x-y semver/patch backwards-compatible bug fixes labels Nov 12, 2021

indutny-signal force-pushed the fix/gh-31558 branch from 84d3493 to cd30fe8 Compare November 12, 2021 10:01

chore: update patches

e1a9fc9

VerteDinde removed the target/17-x-y label Nov 12, 2021

VerteDinde approved these changes Nov 12, 2021

View reviewed changes

indutny-signal reviewed Nov 12, 2021

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 13, 2021

zcbenz approved these changes Nov 14, 2021

View reviewed changes

codebytere approved these changes Nov 15, 2021

View reviewed changes

codebytere merged commit d9e93b3 into electron:main Nov 15, 2021

This was referenced Nov 15, 2021

fix: media shouldn't open permissions dialog #31836

Merged

fix: media shouldn't open permissions dialog #31837

Merged

trop bot added in-flight/15-x-y and removed target/15-x-y labels Nov 15, 2021

indutny-signal deleted the fix/gh-31558 branch November 15, 2021 13:16

trop bot added in-flight/16-x-y and removed target/16-x-y labels Nov 15, 2021

trop bot added merged/15-x-y and removed in-flight/15-x-y labels Nov 15, 2021

indutny-signal mentioned this pull request Dec 17, 2021

[Bug]: Playing any form of audio causes electron to ask for Accessibility permissions on macOS #31558

Closed

3 tasks

commit-lint bot mentioned this pull request Feb 17, 2022

[pull] main from electron:main Blaquez-home/electron#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: media shouldn't open permissions dialog #31805

fix: media shouldn't open permissions dialog #31805

indutny commented Nov 12, 2021 •

edited

indutny commented Nov 12, 2021

indutny commented Nov 12, 2021

indutny commented Nov 12, 2021 •

edited

indutny-signal commented Nov 12, 2021 •

edited

indutny commented Nov 12, 2021

indutny commented Nov 12, 2021

VerteDinde left a comment

indutny-signal Nov 12, 2021

indutny-signal Nov 12, 2021

indutny-signal commented Nov 12, 2021

indutny-signal commented Nov 15, 2021

indutny-signal commented Nov 15, 2021

release-clerk bot commented Nov 15, 2021

trop bot commented Nov 15, 2021

trop bot commented Nov 15, 2021

indutny-signal commented Nov 15, 2021

ArmelChesnais commented Dec 21, 2021

fix: media shouldn't open permissions dialog #31805

fix: media shouldn't open permissions dialog #31805

Conversation

indutny commented Nov 12, 2021 • edited

Description of Change

Checklist

Release Notes

indutny commented Nov 12, 2021

indutny commented Nov 12, 2021

indutny commented Nov 12, 2021 • edited

indutny-signal commented Nov 12, 2021 • edited

indutny commented Nov 12, 2021

indutny commented Nov 12, 2021

VerteDinde left a comment

Choose a reason for hiding this comment

indutny-signal Nov 12, 2021

Choose a reason for hiding this comment

indutny-signal Nov 12, 2021

Choose a reason for hiding this comment

indutny-signal commented Nov 12, 2021

indutny-signal commented Nov 15, 2021

indutny-signal commented Nov 15, 2021

release-clerk bot commented Nov 15, 2021

trop bot commented Nov 15, 2021

trop bot commented Nov 15, 2021

indutny-signal commented Nov 15, 2021

ArmelChesnais commented Dec 21, 2021

indutny commented Nov 12, 2021 •

edited

indutny commented Nov 12, 2021 •

edited

indutny-signal commented Nov 12, 2021 •

edited