1000152 - Desktop client needs ability to mute/unmute audio in a call.

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Comment 1

[:shell escalante]

incoming and outgoing mute

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver.patch

This patch adds two new "Mute audio" and "Mute video" buttons to the conversation controls. I didn't pick the icons Darrin used for his mockups because he had concerns me using them last week (licensing issue AFAICR). 

Though I think this is landable as-is for now as it's useful; we could update the icons once we start updating the styles.

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Side note: I'm totally unsure of the wording, feel free to send alternative suggestions (especially for French, these labels are incredibly hard to translate).

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8447265 [details] [diff] [review]
0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver.patch

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

How about we just steal the Talkilla icons for now? I've no idea where they came from (I *think* she created them), but if we're concerned, we could ping Boriss and check.

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> I've no idea where they came from

I made them, hence why they're ugly ;) Side note, I've lost the pixelmator source for them, not sure how a blocker this is…

Maybe we should just wait to have a proper icon set from Darrin? (any ETA btw?)

Comment 6

[Darrin Henein [:darrin]]

Most of the assets from the mockup are available from mmaslaney in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1016087

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8447265 [details] [diff] [review]
0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver.patch

Given we have some icons now, I think I'd rather get a patch with icons rather than text (although we'll need text for the tooltips!).

I think originally, the expectation was that this bug would also implement the css for the "overlay controls bar" at the top of the conversation window, although I realise that's unclear from the description, so we could potentially split that off if it makes sense.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver.patch

Here's a new patch with the icons and toolbar design provided in bug 1016087. 

While it will have to be ported to React as soon as bug 1033843 lands, this improves the look and feel of Loop enough so I think we could land this first.

Comment 9

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: New toolbar demo (GIF, 5.8MB)

This is a screen recording of the new toolbar in action, for UX validation.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8451347 [details] [diff] [review]
0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver.patch

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

There's various comments in here about matching the UX design spec. If we're not matching the spec on purpose, then we should be explicit about that in the ux-review request so that we can clarify the precise changes and check they are ok.

::: browser/components/loop/content/shared/css/conversation.css
@@ +12,5 @@
> +  position: absolute;
> +  z-index: 999; /* required to have it superimposed to the video element */
> +  left: 0;
> +  right: 0;
> +  background: rgba(0, 0, 0, .75);

Should have opacity .70 according to the spec.

@@ +26,5 @@
> +}
> +
> +.conversation .controls .btn {
> +  width: 32px;
> +  height: 24px;

The design docs say 40px by 30px

@@ +42,5 @@
> +
> +/* Hangup button */
> +.conversation .controls .btn-hangup {
> +  background-color: #D74345;
> +  background-image: url(../img/hangup-inverse-14x14.png);

Really, we should be using just one or two pngs of combined buttons with css pixmaps to draw the button, as iirc this saves memory/performance.

Lets file a follow-up bug for that optimisation.

@@ +47,3 @@
>  }
> +.conversation .controls .btn-hangup:hover {
> +  background-color: red;

The specs suggest a slightly different colour here, and also a colour for active, which we don't seem to be setting.

@@ +63,5 @@
> +  background-color: transparent;
> +  background-image: url(../img/audio-highlight-14x14.png);
> +}
> +.conversation .controls .btn-mute-audio.muted,
> +.conversation .controls .btn-mute-audio:hover {

It is a bit unclear from the spec, but I don't think we want the muted state to have the same background as hover. I think as the icon changes, we're meant to just have the normal background (.7)

We need an additional rule for active (0.5 rather than 0.35).

@@ +80,5 @@
> +    background-image: url(../img/[e-mail]);
> +  }
> +}
> +
> +/* Video mute button */

Same comments apply to this section, though I think we really should be doing some sort of ".media-control" class, so that we can share the hover/active/nothing alpha rules with the audio button, and hence cut down the duplication of rules for different buttons.

::: browser/components/loop/content/shared/js/views.js
@@ +106,5 @@
> +      '              data-l10n-id="hangup_button"></button></li>',
> +      '  <li><button class="btn btn-mute-audio"',
> +      '              data-l10n-id="mute_local_audio_button"></button></li>',
> +      '  <li><button class="btn btn-mute-video"',
> +      '              data-l10n-id="mute_local_video_button"></button></li>',

Based on the graphic design, I'd have expected the video button first.

::: browser/components/loop/jar.mn
@@ +34,5 @@
> +  content/browser/loop/shared/img/[e-mail]  (content/shared/img/[e-mail])
> +  content/browser/loop/shared/img/video-inverse-14x14.png       (content/shared/img/video-inverse-14x14.png)
> +  content/browser/loop/shared/img/[e-mail]    (content/shared/img/[e-mail])
> +  # Shared scripts
> +  content/browser/loop/shared/js/client.js  (content/shared/js/client.js)

This no longer exists (although you've got the right one listed up above)

@@ +44,5 @@
> +  content/browser/loop/shared/libs/jquery-2.1.0.js        (content/shared/libs/jquery-2.1.0.js)
> +  content/browser/loop/shared/libs/backbone-1.1.2.js      (content/shared/libs/backbone-1.1.2.js)
> +  content/browser/loop/shared/libs/sjcl-dev20140604.js    (content/shared/libs/sjcl-dev20140604.js)
> +  content/browser/loop/shared/libs/token.js               (content/shared/libs/token.js)
> +  content/browser/loop/shared/libs/hawk-browser-2.2.1.js  (content/shared/libs/hawk-browser-2.2.1.js)

sjc1, token.js, hawk-browser have been removed from latest master, so they shouldn't be here.

Comment 11

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver-rev2.patch

Addressed review comments.

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Screen Shot 2014-07-07 at 14.22.28.png

I took the liberty to use a blue background to highlight a muted media stream; I really think it improves the interface legibility for end users, though I'm not an expert, so your take Darrin :)

Comment 13

[Darrin Henein [:darrin]]

Comment on attachment 8451600 [details]
Screen Shot 2014-07-07 at 14.22.28.png

Looks great! I agree that the muted states are clearer here. One suggestion: can we decrease the opacity of the non-streaming white versions... to indicate that it is 'disabled'? Essentially, if video is not streaming, it should be clear that you can't video-mute... does that make sense?

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver-rev3.patch

Addressed comments. Also, rebased on the very latest master this time ;)

I also suppressed the TB embedded mute icons, as discussed on IRC.

Comment 15

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: rev3-icon-toolbar.png

Decreased opacity for non-streaming media icons, as per :darrin's suggestion.

Comment 16

[Darrin Henein [:darrin]]

Comment on attachment 8451655 [details]
rev3-icon-toolbar.png

This looks awesome, thanks!

Comment 17

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver-rev4.patch

(In reply to Nicolas Perriault (:NiKo`) from comment #14)
> Created attachment 8451653 [details] [diff] [review]
> 0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver-rev3.patch
> 
> Addressed comments. Also, rebased on the very latest master this time ;)

I lied. What's wrong with me with with making patches? :(

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8451780 [details] [diff] [review]
0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver-rev4.patch

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

Ok, this is very nearly r+. I think we just need to fix the Retina issues, then we'll be good.

::: browser/components/loop/content/shared/css/conversation.css
@@ +84,5 @@
> +.conversation .controls .btn-mute-audio.muted,
> +.conversation .controls .btn-mute-audio.streaming:hover {
> +  background-image: url(../img/mute-inverse-14x14.png);
> +}
> +@media (min-resolution: 2dppx) {

I just remembered to test this on a retina display, and found that the icons were appearing at double size.

According to http://designmodo.com/design-retina-displays/ we need to set the size of the image to get it to render correctly.

@@ +117,5 @@
> +    background-image: url(../img/[e-mail]);
> +  }
> +  .conversation .controls .btn-mute-video.muted,
> +  .conversation .controls .btn-mute-video.streaming:hover {
> +    background-image: url(../img/video-inverse-14x14.png);

This should use the 2x version.

Comment 19

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver-rev5.patch

New patch tested against an emulated retina display (using http://embraceware.com/blog/2013/07/test-retina-graphics-hidpi-on-a-non-retina-display/).

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8451930 [details] [diff] [review]
0001-Bug-1000152-Added-mute-unmute-buttons-to-Loop-conver-rev5.patch

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

Looks great. r=Standard8.

Comment 21

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/416d7931e349

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/416d7931e349

Comment 25

[Francesco Lodolo [:flod]]

21.15 +unmute_local_audio_button.title=Unute your audio

Not really English (no need for a new ID in this case to fix it).

Also, why do we have different mute/unmute for audio and vide?

Comment 26

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> Also, why do we have different mute/unmute for audio and vide?

Not really English either.

I'm looking at the image and I'm even more confused: does "Mute video" mean that I'm disabling my webcam? That would be extremely confusing.

Comment 27

[Romain Testard [:RT]]

"mute video" allows to disable the local webcam.
There is no control allowing to disable the remote webcam.

Although I am confused about https://bug1000152.bugzilla.mozilla.org/attachment.cgi?id=8451655
Why do we have 3 states for video (or audio)?:
*Nothing muted
*Local muted
*Local streaming

We need only 2: 
*Not muted
*Local Muted

Comment 28

[Mark Banner (:standard8)]

I've split comment 25 & 26 out into bug 1036322.

Comment 29

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Romain Testard [:RT] from comment #27)
> We need only 2: 
> *Not muted
> *Local Muted

So what are the blue icons used for? I think it's important to notify the user that its device is actually streaming to the other party. A muted camera/mic is the result of an intentional action, while streaming is the default action performed after having established the connection.

Maybe I misunderstood the designs here, so any clear explanation for every icon state is warmly welcome.

Comment 30

[Darrin Henein [:darrin]]

I think Romain is right here. My initial intent was to use blue icons for 'active' states (i.e audio has been muted) and just grey/white icons for 'normal/streaming' states. I hadn't considered the streaming/not case.

Let's revert to my initial plan: blue for user initiated states (audio and video muted), white for streaming, transparent white for not streaming. We reserve the white icon on blue background for 'actions' i.e. in an audio call, add video to the call (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-active-audio).

Is that clear? Sorry for the confusion by my ui-review+ above.

Comment 31

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> Is that clear?

Well, not entirely :/

> blue for user initiated states (audio and video muted), white for streaming

Ohh, so a media blue icon stands for a muted stream? Shouldn't we use a crossed versions for the icons? I may be wrong but highlighting something in blue usually stands for "enabled" to me… I just want to be sure to understand properly the requirements here :)

> transparent white for not streaming

Hmm, "transparent white", do you mean greyed? (eg. white with opacity set to .7 on a dark background) 
If so, do we want an actually streaming media icon being greyed?

> We reserve the white icon on blue background for 'actions' i.e. in an audio call, add video to the call

Okay that makes sense.

Comment 32

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Darrin, if it's not too much asking, would you mind adding the different media icon states to your mockups? It would certainly help me a lot properly understanding the spec here :)

Comment 33

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Nicolas Perriault (:NiKo`) from comment #31)
> > transparent white for not streaming
> If so, do we want an actually streaming media icon being greyed?

Scratch this one, I misread the quote ;)

Comment 34

[Romain Testard [:RT]]

Attachment: muted_unmuted.png

I think that for "muted" and "unmuted" states in a call we should have only 2 visual indicators for the mic or camera, regardless of whether you started muted or not otherwise it'll be confusing.

I attach what I think we need (just taken off the "streaming" which don't make sense to me.

Comment 35

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> I attach what I think we need (just taken off the "streaming" which don't make sense to me.

Okay so basically we should simply remove the "blue media icon on dark background" to indicate streaming media, right? (sorry if I'm annoying, I want to be sure to fully understand :))

If this is confirmed, we should file a new bug for removing these.

Comment 36

[Romain Testard [:RT]]

Yes that's my view.
Darrin please confirm that we're not mis-interpreting anything from your designs?
Nothing muted = streaming to me and the streaming icons you have (blue icons in black background) seem redundant with the muted icons.

Comment 37

[Darrin Henein [:darrin]]

Yes, makes sense. I updated the designs (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-active), so as follows:

- streaming/not muted = full white icon
- muted = white icon on blue background
- action (see video camera icon in audio-only call) = white icon on green background

Comment 38

[Paul Silaghi, QA [:pauly]]

(In reply to Darrin Henein [:darrin] from comment #37)
> Yes, makes sense. I updated the designs
> (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-active), so as
> follows:
> 
> - streaming/not muted = full white icon
> - muted = white icon on blue background
> - action (see video camera icon in audio-only call) = white icon on green
> background
Are these addressed anywhere? I don't see the green camera in nightly/aurora.

Comment 39

[Romain Testard [:RT]]

I think you're right, the green icons did not get implemented for action.

Comment 40

[Maire Reavy [:mreavy]]

Paul -- Can you file a follow up bug for any icons that are missing?   Thanks.

Comment 41

[Mark Banner (:standard8)]

The green buttons haven't been implemented, because we don't yet have any audio-only calls possible. We definitely want a new bug on allowing audio-only calls being upgraded to audio+video.

Comment 42

[Paul Silaghi, QA [:pauly]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #40)
> Paul -- Can you file a follow up bug for any icons that are missing?  
> Thanks.
bug 1046781

Comment 43

[u279076]

Apart from bug 1046781, this appears to be working as expected. Marking verified fixed.