Closed
Bug 1333468
Opened 7 years ago
Closed 6 years ago
Implement "Device Accessible" privacy indicator (spec requirement)
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: jib, Assigned: johannh)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
Bug 1333468 - Part 1 - Move WebRTC sharing indicator into the identity block and add a paused state.
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
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
Reporter | ||
Comment 2•7 years ago
|
||
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
Comment 3•7 years ago
|
||
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Comment 4•7 years ago
|
||
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
Assignee | ||
Comment 5•6 years ago
|
||
I'll take a look at this :)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
(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 10•6 years ago
|
||
mozreview-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 ::: 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 11•6 years ago
|
||
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+
Assignee | ||
Comment 12•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ea4db0da701679e43e0b061345b2fb1104f484c
Assignee | ||
Comment 13•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b465d3df1e0f8c41ba32abf1ca3e9131f0d23e46
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc52e3043fe233de894e3a64f596119a6e5220ea
Assignee | ||
Comment 19•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•6 years ago
|
||
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.
Assignee | ||
Comment 23•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfd6c6ec20fe9d227f25253733396b93b0fa3d0a
Comment 24•6 years ago
|
||
mozreview-review |
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 25•6 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6abe45843dac1d08fd6ea0d53ae839d44607638
Assignee | ||
Comment 29•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 30•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 31•6 years ago
|
||
mozreview-review-reply |
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 32•6 years ago
|
||
mozreview-review |
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 33•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 34•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4cf9b806329a014f61e2327e4c5d5ed3c1dc0db
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 37•6 years ago
|
||
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
Comment 38•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4052f7934074 https://hg.mozilla.org/mozilla-central/rev/12d5f88351d1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
status-firefox54:
affected → ---
Reporter | ||
Comment 39•6 years ago
|
||
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:
--- → ?
Reporter | ||
Comment 40•6 years ago
|
||
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.
Comment 41•6 years ago
|
||
(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
Comment 42•6 years ago
|
||
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)
Updated•6 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Comment 44•6 years ago
|
||
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)
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•