Closed Bug 1000152 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33

People

(Reporter: RT, Unassigned)

References

Details

(Whiteboard: [p=1])

User Story

As a FF browser user in a call I can mute/unmute my microphone so that I can't be heard.

- Override SDK options (http://tokbox.com/opentok/libraries/client/js/reference/Publisher.html see setStyle)
- Add a mute/unmute button as per: https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-active

Attachments

(3 files, 7 obsolete files)

21.99 KB, image/png
dhenein
: review+
Details
37.21 KB, patch
standard8
: review+
Details | Diff | Splinter Review
11.07 KB, image/png
Details
No description provided.
User Story: (updated)
Summary: Desktop client needs ability to mute/unmute audio → Desktop client needs ability to mute/unmute audio in a call.
Priority: -- → P1
Target Milestone: --- → mozilla32
incoming and outgoing mute
Whiteboard: [s=ui32]
Whiteboard: [s=ui32] → [p=?]
Target Milestone: mozilla32 → mozilla33
Whiteboard: [p=?] → [p=1]
User Story: (updated)
Assignee: nobody → nperriault
Blocks: 1029431
Blocks: 1000156
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.
Attachment #8447265 - Flags: review?(standard8)
Attachment #8447265 - Flags: review?(dmose)
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 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.
> 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?)
Flags: needinfo?(dhenein)
Most of the assets from the mockup are available from mmaslaney in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1016087
Flags: needinfo?(dhenein)
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.
Attachment #8447265 - Flags: review?(standard8)
Attachment #8447265 - Flags: review?(dmose)
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.
Attachment #8451347 - Flags: review?(standard8)
Attached image New toolbar demo (GIF, 5.8MB) (obsolete) —
This is a screen recording of the new toolbar in action, for UX validation.
Attachment #8451348 - Flags: review?(dhenein)
Attachment #8447265 - Attachment is obsolete: true
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/mute-inverse-14x14@2x.png);
> +  }
> +}
> +
> +/* 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/video-highlight-14x14@2x.png  (content/shared/img/video-highlight-14x14@2x.png)
> +  content/browser/loop/shared/img/video-inverse-14x14.png       (content/shared/img/video-inverse-14x14.png)
> +  content/browser/loop/shared/img/video-inverse-14x14@2x.png    (content/shared/img/video-inverse-14x14@2x.png)
> +  # 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.
Attachment #8451347 - Flags: review?(standard8)
Addressed review comments.
Attachment #8451347 - Attachment is obsolete: true
Attachment #8451598 - Flags: review?(standard8)
Attached image Screen Shot 2014-07-07 at 14.22.28.png (obsolete) —
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 :)
Attachment #8451348 - Attachment is obsolete: true
Attachment #8451348 - Flags: review?(dhenein)
Attachment #8451600 - Flags: review?(dhenein)
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?
Addressed comments. Also, rebased on the very latest master this time ;)

I also suppressed the TB embedded mute icons, as discussed on IRC.
Attachment #8451598 - Attachment is obsolete: true
Attachment #8451598 - Flags: review?(standard8)
Attachment #8451653 - Flags: review?(standard8)
Attached image rev3-icon-toolbar.png
Decreased opacity for non-streaming media icons, as per :darrin's suggestion.
Attachment #8451600 - Attachment is obsolete: true
Attachment #8451600 - Flags: review?(dhenein)
Attachment #8451655 - Flags: review?(dhenein)
Comment on attachment 8451655 [details]
rev3-icon-toolbar.png

This looks awesome, thanks!
Attachment #8451655 - Flags: review?(dhenein) → review+
(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? :(
Attachment #8451653 - Attachment is obsolete: true
Attachment #8451653 - Flags: review?(standard8)
Attachment #8451780 - Flags: review?(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/video-highlight-14x14@2x.png);
> +  }
> +  .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.
Attachment #8451780 - Flags: review?(standard8) → review-
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.
Attachment #8451930 - Flags: review?(standard8) → review+
Duplicate of this bug: 1000153
Duplicate of this bug: 1000156
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?
(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.
"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
Flags: needinfo?(nperriault)
I've split comment 25 & 26 out into bug 1036322.
(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.
Flags: needinfo?(nperriault) → needinfo?(dhenein)
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.
Flags: needinfo?(dhenein)
> 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.
Flags: needinfo?(dhenein)
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 :)
(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 ;)
Attached image 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.
> 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.
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.
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
Flags: needinfo?(dhenein)
(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.
Flags: needinfo?(rtestard)
I think you're right, the green icons did not get implemented for action.
Flags: needinfo?(rtestard)
Paul -- Can you file a follow up bug for any icons that are missing?   Thanks.
Flags: needinfo?(paul.silaghi)
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.
(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
Flags: needinfo?(paul.silaghi)
Apart from bug 1046781, this appears to be working as expected. Marking verified fixed.
Status: RESOLVED → VERIFIED
QA Contact: anthony.s.hughes
You need to log in before you can comment on or make changes to this bug.