874401 - Active Camera Or Microphone should be clearly displayed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

When WebRTC is using a camera or microphone, we should display a clear and persistent notification that this is the case. (Probably by using an Android notification)

Comment 1

[Gian-Carlo Pascutto [:gcp]]

blassey noted fullscreen Android apps might hide the notification bar.

Comment 2

[Ian Barlow (:ibarlow)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #1)
> blassey noted fullscreen Android apps might hide the notification bar.

Do we have access to device hardware lights? That might be a useful additional indicator

Comment 3

[Jason Smith [:jsmith]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #1)
> blassey noted fullscreen Android apps might hide the notification bar.

One note (from previous B2G UX discussions) - if we're fullscreen, we can't use any persistent UI for indicators, as any UI we include while we're fullscreen is spoofable.

Comment 4

[Ian Barlow (:ibarlow)]

Yeah, this is why I am asking about the hardware lights

Comment 5

[Aaron Train [:aaronmt]]

(In reply to Ian Barlow (:ibarlow) from comment #2)
> (In reply to Gian-Carlo Pascutto (:gcp) from comment #1)
> > blassey noted fullscreen Android apps might hide the notification bar.
> 
> Do we have access to device hardware lights? That might be a useful
> additional indicator

Note that we can't assume that every device has LED indicators, unfortunately.

Comment 6

[Ian Barlow (:ibarlow)]

(In reply to Aaron Train [:aaronmt] from comment #5)

> Note that we can't assume that every device has LED indicators,
> unfortunately.

Also true. But for those that do, perhaps there is an opportunity to take advantage of them. 

But you're right, there may simply be a few edge cases where it is possible for a user to have a WebRTC stream running in the background while they are in another app that hides the notification bar, and there isn't anything the Firefox can do to notify them about it. And maybe that's ok. Even the Phone app runs into this on Android. I can place a call, switch to the camera and have no indication that it's on.

Comment 7

[Ian Barlow (:ibarlow)]

... no indication that the *phone* is on, to be clear.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Talked with mfinkle earlier and this implements what we talked about. It needs a little cleanup. Also, this logic is in Tabs.java because I assumed at first that we'd care about that tabID. At some point in the future, I think we probably will, so I left it there.

The lights stuff doesn't work on my S3, so its not tested yet. Other than that seems pretty good. Just need to cleanup and maybe get some better images (I stole from AOSP). Will post a screenshot of the notification.

More advanced things, like showing the active tab, switching to it when clicked, etc. I'd like to do in follow ups. This is mostly to get us in a working position.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Screenshot of notif

A bit blurry, but shows the icon and text. If its visible, this will show a little mic in the notification bar as well.

I stole some text from desktop and modified it a bit, but this definately needs ian love.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Screenshot with better icon

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 758873 [details] [diff] [review]
WIP Patch

* Remove all console.log's before landing
* I would like to move more of this into WebrtcUI.js and less in Tabs.java
  * I don't know that we'll use a tabID from the notification. Multiple tabs running webrtc sessions will make it tricky to get right.
  * It would be nice if most of the notification code is isolated from Tabs.java so we can reuse it for other notifications.
  * Move to GeckoApp for now and maybe move to a separate file later?
* The strings are just too long for notifications. We should strive for "no ellipsis"
  * Maybe use "Recording" instead of "camera / microphone" ?
  * Remove the date stamp from the right side of notification ?
* If "recording-window-ended" is not used don't add it to browser.js lazy loader

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch v2

OK. This creates a Notification:Show and Notification:Hide set of messages, and creates a NotificationHelper class to deal with them. Its pretty tightly tied to this one particular use case for now. But its a base for a better Notifications.jsm

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Screenshot with new strings

Comment 14

[Jason Smith [:jsmith]]

Looking at the screenshot included here, some comments:

* The icon being used here isn't correct. You want a camera icon for video only, mic icon for mic only, and a camera+mic icon for video+audio only
* The "Recording" wording feels off. Maybe something more general to indicate that the camera/mic is actively being used?
* Note - make sure you handle the case where multiple getUserMedia requests are made - that can alter the always visible indicator if say, I request video only in one request then in a different request on the same page, I ask for audio only.
* Could there be a problem potentially here that we aren't giving clear indication of where the camera is being shared to?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Jason Smith [:jsmith] from comment #14)
> Looking at the screenshot included here, some comments:

Thanks for the comments


> * The icon being used here isn't correct. You want a camera icon for video
> only, mic icon for mic only, and a camera+mic icon for video+audio only

Maybe, maybe not. Let's not try to overload an image to express too much meaning to the user. We definitely need a fresh image from UX. This one is merely a placeholder.

> * The "Recording" wording feels off. Maybe something more general to
> indicate that the camera/mic is actively being used?

Don't blow our limited area for string length and consider l10n too

> * Note - make sure you handle the case where multiple getUserMedia requests
> are made - that can alter the always visible indicator if say, I request
> video only in one request then in a different request on the same page, I
> ask for audio only.

We either use one general notification, or we need to use a separate notification for each gUM session. We can't be perfect here, it's just not going to work.

> * Could there be a problem potentially here that we aren't giving clear
> indication of where the camera is being shared to?

Again, this is not going to be able to handle the ideal case. We are limited in most versions of Android about what we can effectively display in the notification. Also, we shouldn't overload the notification into something that is hard to use or understand. We are using the notification to let users know something is happening and send them back to Fx to get more info. We can try to use more effective strings.

We can also do soemthing interesting if the user taps the notification. Send them to Firefox and display a popup list of open tabs with active sessions. This UI would have more room to list the "camera" and "microphone" state for each session. Could also allow the user to switch to the tab or close a session.

I don't think that UI would block enabling WebRTC though.

Comment 16

[Wesley Johnston (:wesj)]

I'm not in love with "Recording". I should note this will show a little number indicator off to the right if more than one page is accessing the camera/mic (although I just found a bug in that...)

Comment 17

[Wesley Johnston (:wesj)]

Also, we have strings that look like they were written for this that say "You are currently sharing your microphone with <somesite.com>." I tried shortening them to "Microphone shared with %S", but even then I didn't get much of the host in before the ellipsis appeared (on an S3).

Comment 18

[Aaron Train [:aaronmt]]

Attachment: Chrome Beta (Screenshot)

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 759807 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/chrome/content/WebrtcUI.js b/mobile/android/chrome/content/WebrtcUI.js

>+  get notificationId() {
>+    delete this.notificationId;
>+    return this.notificationId = uuidgen.generateUUID().toString();

Will we only want one single ID? This could be called multiple times during the life of the app. I guess that we currently don't track different tab using different notifications, so only one notification will ever be shown at a time.

I guess it's OK, until we change the way the notification works.

>+  notify: function() {

>+      let win = windows.GetElementAt(0);

No need for this anymore?

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

>+getUserMedia.sharing.title = Recording

Need UX and Copy help on this. Remember we have very little space

> getUserMedia.sharingCamera.message = You are currently sharing your camera with %S.
> getUserMedia.sharingMicrophone.message = You are currently sharing your microphone with %S.
> getUserMedia.sharingCameraAndMicrophone.message = You are currently sharing your camera and microphone with %S.

Do we need to keep these strings?

>+getUserMedia.sharingCamera.message2 = Camera shared.
>+getUserMedia.sharingMicrophone.message2 = Microphone shared.
>+getUserMedia.sharingCameraAndMicrophone.message2 = Camera and microphone shared.

Also need UX and Copy approval

r+ from me with nits fixed

Comment 20

[Ian Barlow (:ibarlow)]

I'd like to simplify the notification strings a little if we can, for l1on's sake. I would also like us to use a unique icon for each combination of active devices. So, 1 for camera only, 1 for mic only, and 1 for camera+mic. I'll post the icons here shortly, and below are the proposed shortened strings.

--

[icon] camera-only
[title] %S
[text] Camera is on

--

[icon] mic-only
[title] %S
[text] Microphone is on

--

[icon] camera-and-mic
[title] %S
[text] Camera and microphone are on

--

Comment 21

[Jason Smith [:jsmith]]

Comment on attachment 759807 [details] [diff] [review]
Patch v2

Review of attachment 759807 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/app/mobile.js
@@ +735,5 @@
>  pref("dom.payment.provider.0.uri", "https://marketplace.firefox.com/mozpay/?req=");
>  pref("dom.payment.provider.0.type", "mozilla/payments/pay/v1");
>  pref("dom.payment.provider.0.requestMethod", "GET");
>  
>  // This needs more tests and stability fixes first, as well as UI.

Nit - this comment is no longer relevant, should we should delete this.

Comment 22

[Ian Barlow (:ibarlow)]

Attachment: alert icon graphics

Here are the notification icons for Mic, Camera, and Camera+Mic, for both GB- devices and ICS+ devices.

Comment 23

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d038d584102c

Comment 24

[Wesley Johnston (:wesj)]

Missed hg adding a file:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a346d1818e86

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d038d584102c
https://hg.mozilla.org/mozilla-central/rev/a346d1818e86

Comment 26

[Cristina Madaras, QA [:CristinaM]]

Test cases added in Moztrap:
https://moztrap.mozilla.org/manage/case/14085/ - Active Microphone notification
https://moztrap.mozilla.org/manage/case/14084/ - Active Camera notification
https://moztrap.mozilla.org/manage/case/14083/ - Active Camera and Microphone notification

Comment 27

[katamari]

How do I disable this zit that gcp has suggested we include by default in firefox?