Closed Bug 874401 Opened 6 years ago Closed 6 years ago

Active Camera Or Microphone should be clearly displayed

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox24 + verified
fennec 24+ ---

People

(Reporter: gcp, Assigned: wesj)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [getUserMedia][android-gum+])

Attachments

(4 files, 3 obsolete files)

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)
Depends on: 862377
Whiteboard: [getUserMedia][android-gum+]
blassey noted fullscreen Android apps might hide the notification bar.
Blocks: 830831
(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
(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.
Yeah, this is why I am asking about the hardware lights
(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.
(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.
... no indication that the *phone* is on, to be clear.
tracking-fennec: --- → ?
Attached patch WIP Patch (obsolete) — Splinter Review
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.
Attachment #758873 - Flags: feedback?(mark.finkle)
Attached image Screenshot of notif (obsolete) —
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.
Flags: needinfo?(ibarlow)
tracking-fennec: ? → 24+
Attached image Screenshot with better icon (obsolete) —
Attachment #758878 - Attachment is obsolete: true
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
Attachment #758873 - Flags: feedback?(mark.finkle) → feedback+
Attached patch Patch v2Splinter Review
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
Attachment #758873 - Attachment is obsolete: true
Attachment #759807 - Flags: review?(mark.finkle)
Attached image Screenshot
Screenshot with new strings
Attachment #759277 - Attachment is obsolete: true
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?
(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.
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...)
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).
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
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
Attachment #759807 - Flags: review?(mark.finkle)
Attachment #759807 - Flags: review+
Attachment #759807 - Flags: feedback?(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

--
Flags: needinfo?(ibarlow)
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.
Attached file alert icon graphics
Here are the notification icons for Mic, Camera, and Camera+Mic, for both GB- devices and ICS+ devices.
https://hg.mozilla.org/mozilla-central/rev/d038d584102c
https://hg.mozilla.org/mozilla-central/rev/a346d1818e86
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Keywords: verifyme
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(aaron.train)
Keywords: verifyme
Blocks: 887561
Attachment #759807 - Flags: feedback?(ibarlow) → feedback?
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
Flags: in-moztrap?(aaron.train) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.