Closed Bug 1333468 Opened 3 years ago Closed 2 years ago

Implement "Device Accessible" privacy indicator (spec requirement)

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
relnote-firefox --- 60+
firefox60 --- fixed

People

(Reporter: jib, Assigned: johannh)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

The spec now mandates two privacy indicators, and we're missing the second one [1]:

 - a "Live" recording indicator (which we have), and
 - an "Accessible" permission indicator (which we don't have).

This bug is about implementing the second one. FWIW Chrome almost has one [2], they just need to make it appear as early as page-load (right now it only appears after gUM use).

This ties into our "mute"/"disabled" story (bug 1299515): A permission indicator would let us consider turning off the camera hardware light during mute/disable, which is desired by both devs and users, and much of the reason behind all this spec work. 

Note that the existing indicators buried in the page info drop-down are not sufficient for this. There's something about live recording of someone's voice and face that trumps other privacy concerns.

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#privacy-indicator-requirements
[2] https://www.w3.org/2011/04/webrtc/wiki/images/1/1e/WebRTCWG-2016-09-22.pdf#20
Marking P2 based on bug 1299515.
Rank: 25
Priority: -- → P2
I wanted to update this with a dump from recent email thread on shorter-term needs: At minimum need UX here to solve bug 1299515.

E.g. something like replacing the red pulsing indicators with gray cam/mic icons during periods of mute (e.g. maybe like the temporary permission-block icons but without the slash through it).

Also, the page-info drop-down would still need to offer revocation of permissions during mute (but maybe with gray icons again):

    Permissions:
    [gray cam]  Use the Camera      Allowed Temporarily x
    [gray mic]  Use the Microphone  Allowed Temporarily x

The above is for illustration purposes only. The idea is to convey that permissions are still on but you're not live at the moment. The invariant we want is "once indicators disappear, they can't come back" (unless users opt in to "Remember").

We might want to defer for now the part of the spec that says this indicator should be on at page-load for sites with persistent permissions.
Rank: 25 → 18
Priority: P2 → P1
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
As mentioned in the email thread, here is a link to the spec draft. Feel free to ask any questions or add any comments:
https://drive.google.com/a/mozilla.com/file/d/0B8kj4Mlm-HJecktURGQzanI0c1U/view?usp=sharing
I'll take a look at this :)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

The approach here uses integer constants (0, 1 and 2 at the moment) to differentiate between disabled, active and paused sharing state.

Andreas, assuming you haven't entirely made up your mind about this yet, I'd prefer to receive the different states in this format from mediaCaptureWindowState(), possibly defined as constants in https://searchfox.org/mozilla-central/source/dom/media/nsIMediaManager.idl. This seems like the best alternative to double-booleans, bitflags or strings to me.

Florian, it'd be great to get feedback on this so that we can switch to review quickly once bug 1299515 is done. I'll add tests in a separate commit.

One thing you might note is that I use a lot of implicit

if (!sharingState) {

cases. I think I'd prefer to do something like

if (sharingState != MediaManagerService.DISABLED)

instead, once we have these constants available. I'm also a bit unhappy about adding "Paused" to the screensharing strings, but I don't know a better alternative. Feel free to throw in ideas :)
Attachment #8929389 - Flags: feedback?(florian)
No longer blocks: 1299515
Depends on: 1299515
Sgtm. I'll have WIP patches for bug 1299515 ready soonish. I'll put the api changes in one of them for you to review. I am only planning to change the type for camera and microphone however, as I understand we don't have planned UI for the others being in a disabled state.
(In reply to Andreas Pehrson [:pehrsons] from comment #8)
> Sgtm. I'll have WIP patches for bug 1299515 ready soonish. I'll put the api
> changes in one of them for you to review. I am only planning to change the
> type for camera and microphone however, as I understand we don't have
> planned UI for the others being in a disabled state.

Ah, that probably makes my patch simpler :)
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

https://reviewboard.mozilla.org/r/200716/#review205970

::: browser/base/content/browser.js:8027
(Diff revision 1)
> +         !aPermission.sharingState.includes("Paused"))) {
>        classes += " in-use";
> +
> +      // Synchronize control center and identity block blinking animations.
> +      window.requestAnimationFrame(() => {
> +        let sharingIconBlink = document.getElementById("sharing-icon").getAnimations()[0];

Do we need a BrowserUtils.promiseLayoutFlushed call before the rAF?

::: browser/themes/shared/identity-block/identity-block.inc.css:137
(Diff revision 1)
> -#sharing-icon {
> -  animation: 3s linear identity-box-sharing-icon-pulse infinite;
> +  animation: 1.5s ease in-use-blink infinite;
> +  -moz-context-properties: fill;
> +  fill: rgb(224, 41, 29);
>  }
>  
> -/* This should remain identical to tab-sharing-icon-pulse in tabs.inc.css */
> +@keyframes in-use-blink {

Why is this animation changing and why does it no longer matter to keep it in sync with the tab animation?
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

Looks generally reasonable. I haven't tried the patch locally. I don't understand why we would not have the disabled UI for screen sharing (comment 8).
Attachment #8929389 - Flags: feedback?(florian) → feedback+
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

https://reviewboard.mozilla.org/r/200716/#review205970

> Do we need a BrowserUtils.promiseLayoutFlushed call before the rAF?

Still trying to find out about this, shouldn't block the general review, I hope. I'll leave this unresolved for now.

> Why is this animation changing and why does it no longer matter to keep it in sync with the tab animation?

This animation is no longer the same as the tab animation, it no longer alternates between the identity icon and the camera icon, it just blinks.
See Also: → 1436074
Florian, I realized that there's a WebExtension API that uses the sharingState attribute internally (for querying tabs that are streaming camera, microphone or screen). Feel free to review these patches with that in mind. I'll make a separate patch for someone in add-ons to review.
Aha! Turns out these WE APIs helpfully use the tabbrowser.getTabSharingState method, so we just have to fine-tune that one. I don't think we need any special review for that.
Comment on attachment 8950518 [details]
Bug 1333468 - Part 2 - Add tests for paused/disabled sharing state indicators.

https://reviewboard.mozilla.org/r/219794/#review227504

::: browser/base/content/test/webrtc/browser_devices_get_user_media_paused.js:29
(Diff revision 3)
> +      video: MediaManagerService.STATE_CAPTURE_ENABLED,
> +      audio: MediaManagerService.STATE_CAPTURE_ENABLED,
> +    });
> +
> +    // Disable both audio and video.
> +    await ContentTask.spawn(gBrowser.selectedBrowser, null, function() {

You have this code duplicated several times, I think this could be moved to a setTrackEnabled helper at the top of the file, taking a parameter indicating if we want to affect audio, video or all tracks.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_paused.js:40
(Diff revision 3)
> +
> +    await expectObserverCalled("recording-device-events", 2);
> +
> +    // It sometimes takes a bit longer before the change propagates to the UI,
> +    // wait for it to avoid intermittents.
> +    await BrowserTestUtils.waitForCondition(

waitForCondition waits using timers, it's kind of a last resort solution. Is there no specific event we can wait for here (and in the other places where you have this pattern)?

::: browser/base/content/test/webrtc/head.js:464
(Diff revision 3)
>      ok(screenSelector.hidden, "screen selector hidden");
>  }
>  
>  // aExpected is for the current tab,
>  // aExpectedGlobal is for all tabs.
> +// paused is an array of currently disabled streams

There's no 'paused' array in the arguments of that function.

::: browser/base/content/test/webrtc/head.js:466
(Diff revision 3)
>  
>  // aExpected is for the current tab,
>  // aExpectedGlobal is for all tabs.
> +// paused is an array of currently disabled streams
>  async function checkSharingUI(aExpected, aWin = window, aExpectedGlobal = null) {
> +  function isPaused(stream) {

the 'stream' parameter name seems confusing, maybe use streamState instead?

::: browser/base/content/test/webrtc/head.js:501
(Diff revision 3)
>        if (idToConvert == "microphone")
>          return "audio";
>        return idToConvert;
>      };
> -    let expected = aExpected[convertId(id)];
> +    let convertedId = convertId(id);
> +    let expected = aExpected[convertedId];

What is this change for?

::: browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js:19
(Diff revision 3)
>    // just testing that extension tabs get the info and are updated when it is
>    // called.
> -  gBrowser.setBrowserSharing(tab.linkedBrowser, {screen: "Window", microphone: true, camera: true});
> +  gBrowser.setBrowserSharing(tab.linkedBrowser, {
> +    sharing: "screen",
> +    screen: "Window",
> +    microphone: MediaManagerService.STATE_CAPTURE_ENABLED,

You don't need the media manager service to access a constant that exists on the interface, just use Ci.nsIMediaManagerService.STATE_...
Attachment #8950518 - Flags: review?(florian) → review-
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

https://reviewboard.mozilla.org/r/200716/#review227502

r- for this problem found while testing: 
1. data:text/html,<iframe src="https://mozilla.github.io/webrtc-landing/gum_test.html"></iframe><iframe src="https://mozilla.github.io/webrtc-landing/gum_test.html"></iframe>
2. Share a camera with the first frame.
3. Click the "video" button in the second frame, but click "Don't allow" in the prompt

Actual result: the status in the control center switches to "blocked temporarily" and all the sharing indicators disappear from the control center and the identity area (the global sharing indicator remains).

Expected result: the per-tab sharing indicator should remain.

f+ for the rest of the patch, although I still find it unfortunate to (probably) trigger style flushes with the getAnimations() calls from within a requestAnimationFrame callback.

::: browser/themes/shared/identity-block/identity-block.inc.css:139
(Diff revision 3)
> -#sharing-icon {
> -  animation: 3s linear identity-box-sharing-icon-pulse infinite;
> +  animation: 1.5s ease in-use-blink infinite;
> +  -moz-context-properties: fill;
> +  fill: rgb(224, 41, 29);
>  }
>  
> -/* This should remain identical to tab-sharing-icon-pulse in tabs.inc.css */
> +@keyframes in-use-blink {

If you remove this comment, you need to remove https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/browser/themes/shared/tabs.inc.css#252 too.
Attachment #8929389 - Flags: review?(florian) → review-
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

https://reviewboard.mozilla.org/r/200716/#review205970

> Still trying to find out about this, shouldn't block the general review, I hope. I'll leave this unresolved for now.

Talked to Emilio and yeah, we need that.
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

https://reviewboard.mozilla.org/r/200716/#review227502

Thanks for finding this. This was because I didn't add the additional sharingState attributes to removeBrowserSpecificIndicator. I moved them to getTabStateForContentWindow.
Comment on attachment 8950518 [details]
Bug 1333468 - Part 2 - Add tests for paused/disabled sharing state indicators.

https://reviewboard.mozilla.org/r/219794/#review227504

> waitForCondition waits using timers, it's kind of a last resort solution. Is there no specific event we can wait for here (and in the other places where you have this pattern)?

I can't think of a better way to do it, suggestions welcome, however I don't think waitForCondition (with a short timer) is a bad solution.

> There's no 'paused' array in the arguments of that function.

Yeah, that's left over from something I tried earlier.

> What is this change for?

Ah, I just forgot to revert this.

> You don't need the media manager service to access a constant that exists on the interface, just use Ci.nsIMediaManagerService.STATE_...

Ah, indeed, thanks!
Comment on attachment 8950518 [details]
Bug 1333468 - Part 2 - Add tests for paused/disabled sharing state indicators.

https://reviewboard.mozilla.org/r/219794/#review228254

It would be good to also have coverage for cases that involve frames, but I'm not blocking review on it.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_paused.js:6
(Diff revision 4)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function setTrackEnabled(audio, video) {
> +  return ContentTask.spawn(gBrowser.selectedBrowser, {audio, video}, function(args) {
> +    let global = content.wrappedJSObject;

nit: let stream = content.wrappedJSObject.gStreams[0]; would slightly reduce duplication.
And then stream.get*Tracks()[0].enabled = args.*; can probably fit on a single line.

::: browser/base/content/test/webrtc/head.js:6
(Diff revision 4)
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource:///modules/SitePermissions.jsm");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "MediaManagerService",
> +                                   "@mozilla.org/mediaManagerService;1",
> +                                   "nsIMediaManagerService");

I don't think you need this here. You may want to define 2 constants like
const STATE_CAPTURE_DISABLED = Ci.nsIMediaManagerService.STATE_CAPTURE_DISABLED;
Attachment #8950518 - Flags: review?(florian) → review+
Comment on attachment 8929389 [details]
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.

https://reviewboard.mozilla.org/r/200716/#review228274

Looks good to me.

::: browser/base/content/browser.js:8018
(Diff revision 4)
> +      BrowserUtils.promiseLayoutFlushed(document, "style", () => {
> +        let sharingIconBlink = document.getElementById("sharing-icon").getAnimations()[0];
> +        let imgBlink = img.getAnimations()[0];
> +        if (sharingIconBlink && imgBlink) {
> +          window.requestAnimationFrame(() => {
> +              imgBlink.startTime = sharingIconBlink.startTime;

nit: remove 2 spaces of indent here. I hope the 'startTime' getter doesn't cause a style flush. If it does you would need to get this before the rAF callback and store it in a variable.
Attachment #8929389 - Flags: review?(florian) → review+
Blocks: 1440607
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4052f7934074
Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state. r=florian
https://hg.mozilla.org/integration/autoland/rev/12d5f88351d1
Part 2 - Add tests for paused/disabled sharing state indicators. r=florian
https://hg.mozilla.org/mozilla-central/rev/4052f7934074
https://hg.mozilla.org/mozilla-central/rev/12d5f88351d1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Release Note Request (optional, but appreciated)
[Why is this notable]:

Protects user privacy during face-mute on all camera-using web-sites. End-users will now see their camera light go out when they face-mute on all web-sites that access their camera, which means they no longer need to trust these sites to not be recording them while muted (Firefox's camera and mic indicators go from blinking red to gray, but don't disappear during mute).

Differentiation: Chrome doesn't have this feature. Some sites (notably Hangouts and Meet) achieve the same effect in Chrome through non-standard means (ending camera capture early on mute instead of temporarily disabling the video track, and requesting camera again on unmute) which causes a bad user experience in Firefox where users are prompted again for permission needlessly on unmute.

Firefox's model is to spec, and superior because it doesn't rely on users granting persistent permission to sites, like Chrome does implicitly.

[Affects Firefox for Android]: No
[Suggested wording]:

Something like: "Enhanced camera privacy indicators. Firefox now turns off your camera when you face-mute on web-sites that use your camera, turning off your camera light so you don't have to wonder if the site is still recording you. The light will come back on whenever recording resumes."

[Links (documentation, blog post, etc)]: Working on a blog. UX spec draft in comment 4.
relnote-firefox: --- → ?
Note: this feature is already mentioned under developer release notes https://developer.mozilla.org/en-US/Firefox/Releases/60 but we feel this feature deserves a more prominent mention, due to it's end-user benefits.
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #40)
> Note: this feature is already mentioned under developer release notes
> https://developer.mozilla.org/en-US/Firefox/Releases/60 but we feel this
> feature deserves a more prominent mention, due to it's end-user benefits.

Adding dev-doc-needed, so we can explore giving this a better mention.
Keywords: dev-doc-needed
Added this section to the getUserMedia() page: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getUserMedia#User_privacy

Will that do the job? I don't currently have a link to a test I can run to see this in action, so let me know if my description of what the user sees is wrong.
Flags: needinfo?(jib)
Flags: needinfo?(jhofmann)
Seems fine to me :)
Flags: needinfo?(jhofmann)
Seems good to me, modulo a few minor details. What's described seems correct for desktop and for non-persistent permissions, which is the default in Firefox.

There are slight differences when permission is persisted (☑ Remember this decision). As written, readers might expect to see the gray camera/mic icon(s) on page-load on sites they've granted persistent permissions for. This would be consistent with the spec's example of independent live vs. accessible indicators. Instead, we show a • dot in the ⓘ info icon, which is consistent with Firefox's other persistent permissions. The user can click on the ⓘ to see all the permissions listed.

The gray camera/mic icon is only shown when camera/mic is "live but muted", which in Firefox is distinct from non-live-but-persisted-access. This strikes a balance that covers the default (non-persistent) case 100%, while remains consistent for users of persistent permissions.

On android, we don't support turning off camera/mic on mute. This is only for lack of UX, which is trickier there.
Flags: needinfo?(jib)
You need to log in before you can comment on or make changes to this bug.