885796 - WebRTC doesn't have UI to stop sharing camera nor microphone

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect)

Description

[alex_mayorga]

Attachment: Screen capture of door hanger.

WebRTC must have UI to stop sharing camera or microphone.

Right now the only way to stop the sharing is by closing or reloading the tab which is not intuitive.

I believe the door hanger UI should remain the same even after a WebRTC call is ongoing, this way the user could stop sharing.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Requesting UX input from Boriss before I request code reviews. Is this something we want?

Comment 2

[Florian Quèze [:florian]]

Attachment: Screenshot with the patch applied

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch v2

bug 805632 had already implemented the back-end, so the patch here could be simplified.

Comment 4

[Jennifer Morrow [:Boriss] (UX)]

Great patch, and thanks for attachment 8341108 [details], Florian!  The other reason this is badly needed, beyond ceasing sharing, is changing cameras once the sharing has begun.  Florian, does that dropdown arrow on the split button currently list other cameras and microphones?  If so, happy to test & ui+

Comment 5

[Florian Quèze [:florian]]

Attachment: Screenshot of the dropdown menu

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #4)

> The other reason this is badly needed, beyond ceasing sharing, is changing
> cameras once the sharing has begun.  Florian, does that dropdown arrow on
> the split button currently list other cameras and microphones?  If so, happy
> to test & ui+

Unfortunately, the drop down menu contains only a more or less pointless "Not Now" item (see attached screenshot), that when clicked just hides the panel. This appears automatically whenever there are actions associated with the notification, and isn't something I control.

The patch here is a small first step toward improving our permission UI. Letting the user change the camera and microphone during a call requires much more work (possibly weeks of effort) and is in my opinion better addressed in a separate bug (bug 880312).

Comment 6

[Justin Dolske [:Dolske]]

* We should add an option to PopupNotifications.show() to allow suppressing the default "Not Now" option. This came up with Mixed Content Blocking (MCB) and was mildly awkward there, but it feels really awkward here (attachment 8342208 [details]). An always-present "Not Now" made sense when we were only using doorhangers for question+action permissions, but we seem to be increasingly using these prompts as hidden-by-default informational panels, where it doesn't seem to make much sense.

* I think it would be helpful to have an explicit "Continue sharing" secondary option (which would do nothing). That feels a lot more natural than "Not Now", and provides a clear choice that isn't "Stop".

* ...upon which you'll find that, as with MCB, you can't have a secondary action that keeps the doorhanger around. Clicking "Continue sharing" would make the icon go away. :( But both this bug and MCB really just want to relabel the "Not Now" do-nothing action, so maybe that's the easy way to fix all of this (an option to .show() that provides a new label+accesskey for the "not now" action.

* What should the default action here be? The dialog isn't shown by default, so it kinda seems like the default should be "Continue Sharing" (eg, curious user clicks it, so default is the change-nothing choice). Especially since this seems purposed as fallback UI if the page itself doesn't make it clear how to stop sharing. I guess I don't feel very strong about it either way, but wanted to see if it was considered.

Comment 7

[Justin Dolske [:Dolske]]

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

This looks ok -- previous comments aside -- but I'd like a test. Breaking "stop sharing" in the future would be particularly embarrassing, because it's important that actually work!

Comment 8

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #6)
After reading comment 6, it looks like the UX suggestions are:
1. make the "Not Now" item optional, and hide it for this specific notification.
2. make the wording of the "Not Now" item customizable, so that we can more appropriately label it "Continue sharing".
3. make "Continue Sharing" the default action, and hide "Stop Sharing" in the sub-menu.
4. keep the patch as is.

Boriss, which of these paths do you want us to follow?

My personal opinion is that the issue with the "Not now" item isn't specific to WebRTC and could be better addressed in its own bug. The "Not now" item is awkward, and while labeling it "Continue Sharing" would make more sense, it still feels very unnatural to me to have a menu item that doesn't do anything.
I would be curious to have some context about the reason why this "Not now" item was added in the first place. Were we concerned user may not find ways to dismiss the panel? (Clicking outside of it or on the [x] seems obvious too me; but I guess I'm not really representative of our user base :))

Comment 9

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #7)
> Comment on attachment 8341143 [details] [diff] [review]

> I'd like a test. Breaking
> "stop sharing" in the future would be particularly embarrassing, because
> it's important that actually work!

I share the same feeling; I would feel more comfortable if this feature was tested.

Unfortunately, I don't have any idea of how a test could easily be created here.
When testing this as a human, I verify that the patch works by checking that clicking the "Stop Sharing" button turns off the green light of the camera, but that's not something that can be automated.

I was thinking that we could check that the toolbar icon listing the "sites you are currently sharing your camera or microphone with" disappears. But then I remembered that we can't use real media streams in tests, and that fake streams don't use the devices so they don't cause the UI to appear.

Unless I missed something, none of the WebRTC UI currently has tests. None of bug 729522, bug 799417 or bug 805632 added tests. See also bug 799417 comment 43.

While breaking the "Stop Sharing" button would be embarrassing, I think it wouldn't be more embarrassing than breaking the indicators that a device is being shared. So I don't think the lack of test here should block landing the feature.

One idea I have to address this situation is to add a hidden preference that would cause getUserMedia fake streams to still show the permission UI, and list a fake shared device in the UI. If this idea works, I suspect it will be a non-trivial amount of work; and unrelated to the patch here. So I would suggest either doing it in its own bug, or maybe in bug 804611 where I'm adding persistent permissions (that also really wants tests...).

Any opinion?

Comment 10

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> (In reply to Justin Dolske [:Dolske] from comment #6)
> I would be curious to have some context about the reason why this "Not now"
> item was added in the first place. Were we concerned user may not find ways
> to dismiss the panel? (Clicking outside of it or on the [x] seems obvious
> too me; but I guess I'm not really representative of our user base :))

Exactly right!  We did user testing and found that without the X, users sometimes felt they had no idea what action would get rid of the notification without saying "yes."  We also found users press the big button without reading, but that's another story.

The "not now" option was also included so users would know that this wasn't their only chance to grant the permission.  They could, for instance, plug in a webcam and then click the icon again.  Hence "Not now" rather than "Hell no."

In Comment 6, Dolske correctly describes the ideal patch.  The ideal patch would:

1. Have the default action be "Continue Sharing".  The UX plan with doorhangers is to have the "big button" be as non-invasive as possible - that clicking it without reading means your functionality doesn't break.  The alternative is that a curious user clicks the video icon, doesn't see another alternative than to stop sharing, and accidentally disrupts their video call.  That's Florian's #2 and #3 in Comment 8.
2. Have "Stop Sharing" be a secondary item in the split button.

We can leave this patch as is and have a halfway solution and file bugs for #1 and #2.  But, if it's possible and if Florian's willing to spin up another patch, it'd be great to fix the problems dolske mentioned.  Particularly since the patch as is could lead users to accidentally disconnect their webcams pretty easily.

Comment 11

[Florian Quèze [:florian]]

Attachment: Screenshot with 'Continue Sharing'

Per comment 6 and comment 10, I'm preparing a new patch that will display "Continue Sharing" in the button, "Stop Sharing" as an action in the popup menu, and hide the "Not Now" item.

Comment 12

[Florian Quèze [:florian]]

Attachment: Patch v3

After sleeping on it I wasn't fully satisfied of my answer in comment 9 to your request for a test, so I looked into it again. Even just forcing this notification to be displayed requires MediaManagerService.mediaCaptureWindowState to believe we are currently sharing a device with the current web page.
So I see only 2 solutions to make this testable:
- add a hidden preference in the media manager code to request permissions for fake streams.
- override the nsIMediaManagerService implementation by a JS one.
It looks like Gavin already suggested these 2 possible solutions in bug 799417 comment 44. I still think implementing these solutions would be better handled in separate bugs and shouldn't block this bug.

My changes to PopupNotifications do have tests though! :-)

Comment 13

[Florian Quèze [:florian]]

Attachment: Patch v3 unbitrotted

Fix the bitrot caused by the patch landed in bug 723951.

Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=31dd34109a15

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 8357186 [details] [diff] [review]
Patch v3 unbitrotted

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

Bonus points if you spin off the PopupNotification.jsm/notification.xml to a separate bug.

r+ with the buttontype change.

::: toolkit/modules/PopupNotifications.jsm
@@ +195,5 @@
>     *          - label (string): the button's label.
>     *          - accessKey (string): the button's accessKey.
>     *          - callback (function): a callback to be invoked when the button is
> +   *            pressed. If the function returns true, the notification will be
> +   *            dismissed instead of removed.

I'm a little bit wary of doing this -- previously the return value was ignored, and so there could be existing callers returning true (pointlessly), which would now break.

Not sure how big a deal that is without auditing some existing callers. A workaround, if we wanted, would be to have another |options| flag to opt-in to this.

Gavin knows this code better -- any thoughts? [Also, I vague remember something about wanting to make the callback invocation async, not sure if that matters here?]

@@ +556,5 @@
> +          popupnotification.setAttribute("hidenotnow", "true");
> +          // If 'Not Now' is hidden and there's no secondary action,
> +          // make the menubutton a regular button.
> +          if (!n.secondaryActions.length)
> +            popupnotification.setAttribute("buttontype", "");

I'd prefer to avoid helping callers make poorly-designed single action prompts unless there's a good / unavoidable reason for it. The intent here with |hidenotnow| is to hide the default "Not now" because the caller is providing a _replacement_.

Just set the attribute here, and add a check in show() to require that if you set |hidenotnow| you must also provide at least one secondary action.

Comment 15

[Florian Quèze [:florian]]

Attachment: Patch v3 (webrtc UI only)

Thanks for the review!

As requested in comment 14, I filed bug 958071 for the PopupNotification.jsm/notification.xml changes.

This new attachment contains only the changes to the WebRTC UI. Carrying forward dolske's r+, and removing the needinfo?gavin that I've moved to bug 958071.

Comment 16

[Florian Quèze [:florian]]

Attachment: Patch v4 (webrtc UI only)

Patch updated for compatibility with the new attachment 8360991 [details] [diff] [review] from bug 958071.

Comment 17

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/815d5970e947

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/815d5970e947

Comment 20

[Florian Quèze [:florian]]

Note: I actually added automated tests for this as part of bug 804611.

Comment 21

[Ioana (away)]

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> Note: I actually added automated tests for this as part of bug 804611.

Removing keyword in this case..