990678 - Ability to answer a call as audio+video or audio only

Status: VERIFIED FIXED

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

Comment 1

[Mark Banner (:standard8)]

This is an MVP thing, for now, as we'll use the permissions prompt for a crude accept/decline, and once we've punched through the prompt (into privileged code), then we'll need to implement this.

Comment 2

[Romain Testard [:RT]]

Attachment: Incoming call from URL click

Comment 3

[Romain Testard [:RT]]

Attachment: Incoming call from direct call

Comment 4

[Mark Banner (:standard8)]

Mutating and updating into what this is really becoming about - audio/video acceptance.

Darrin: The mock-ups suggest that the callee can decide whether or not to send audio or audio and video.

However, a question arose, how do we handle rejecting video for a person? e.g. someone has initiated a video & audio call, but the callee doesn't want the video due to being on a paid-for-bandwidth device.

Can the callee reject the video, if so how?

Comment 5

[Romain Testard [:RT]]

A user can answer an incoming call with audio+video or audio only.
Users have a default answering mode set in the settings panel (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/) which can be set to audio only or audio+video. The default answering mode defines the behavior when pressing the "Answer button" upon an incoming call request and the list of both answering modes (audio+video or audio only) is available if the user presses the chevron located on the right hand side of the answer button.

So if the user wants to answer an incoming call with audio only on his side (he could although receive the caller video stream if the caller shares his video) he has 2 options:
* If his default answering mode is set to "audio only", he just presses the "answer" button
* In any case he can press the chevron and select "Audio only"

Comment 6

[Darrin Henein [:darrin]]

(In reply to Romain Testard [:RT] from comment #5)
> If the user wants to answer an incoming call with audio only on his side
> (he could although receive the caller video stream if the caller shares his
> video) he has 2 options:
> * If his default answering mode is set to "audio only", he just presses the
> "answer" button
> * In any case he can press the chevron and select "Audio only"

I was under the impression that the answering 'mode' was only for what the user who is answering wants to send. I.e. you video call me but I only want to send audio, so I answer with Audio-only (for me).

Sounds like we need some clarification here. My scenario leads to mixed calls (audio on one side, audio + video on the other) which is fine, and something Romain had asked for UI for, so that was my line of thinking.

Comment 7

[Romain Testard [:RT]]

(In reply to Darrin Henein [:darrin] from comment #6)
> (In reply to Romain Testard [:RT] from comment #5)
> > If the user wants to answer an incoming call with audio only on his side
> > (he could although receive the caller video stream if the caller shares his
> > video) he has 2 options:
> > * If his default answering mode is set to "audio only", he just presses the
> > "answer" button
> > * In any case he can press the chevron and select "Audio only"
> 
> I was under the impression that the answering 'mode' was only for what the
> user who is answering wants to send. I.e. you video call me but I only want
> to send audio, so I answer with Audio-only (for me).
> 
> Sounds like we need some clarification here. My scenario leads to mixed
> calls (audio on one side, audio + video on the other) which is fine, and
> something Romain had asked for UI for, so that was my line of thinking.

Yes - apologies if I expressed myself poorly but here are the scenarios:
1 Caller offers audio only and callee answers with audio only 
2 Caller offers audio only and callee answers with audio+video
3 Caller offers audio+video and callee answers with audio only
4 Caller offers audio+video and callee answers with audio+video

So back to Mark's question "Can the callee reject the video, if so how?"
This is [3] and the callee can do it as follows:
* Either he has "Audio only" as default answering mode and he just has to press the "Answer" button
* Or he presses the chevron, both "Audio only" and "Audio+Video" options appear and he selects "Audio only"

Comment 8

[Darrin Henein [:darrin]]

> So back to Mark's question "Can the callee reject the video, if so how?"
> This is [3] and the callee can do it as follows:
> * Either he has "Audio only" as default answering mode and he just has to
> press the "Answer" button
> * Or he presses the chevron, both "Audio only" and "Audio+Video" options
> appear and he selects "Audio only"

Hmm, I still thought something different... I thought how the callee answers only determines what *they share*. How do I accept a video+audio call but only share audio (and still see their video)? I didn't think we were considering rejecting the media incoming, but only controlling the outgoing media. Isn't this what everyone else does?

Comment 9

[Romain Testard [:RT]]

What I understood from Mark's question "Can the callee reject the video, if so how?" was "Can the callee decline to stream his video whilst still seeing the caller's video"

I think we agree on the approach, just misinterpretation of the wording, here are the scenarios again with explicit results:
1 Caller offers audio only and callee answers with audio only : the caller can't see the callee, the callee can't see the caller
2 Caller offers audio only and callee answers with audio+video : the caller can see the callee, the callee can't see the caller
3 Caller offers audio+video and callee answers with audio only : the caller can't see the callee, the callee can see the caller
4 Caller offers audio+video and callee answers with audio+video : the caller can see the callee, the callee can see the caller

Comment 10

[Mark Banner (:standard8)]

(In reply to Darrin Henein [:darrin] from comment #8)
> I didn't think we were
> considering rejecting the media incoming, but only controlling the outgoing
> media. Isn't this what everyone else does?

Yes it is currently what most other people do atm. However I believe Maire was suggesting that we because we can reject video in WebRTC easily, then we could do that. This way, even though (a) is intending on sending video, (b) could decide not to receive it because they are on a paid or low bandwidth connection (without first having to connect the call, and ask the other side to turn video off).

Maire, am I summarising that right?

Comment 11

[Maire Reavy [:mreavy]]

(In reply to Mark Banner (:standard8) from comment #10)
> (In reply to Darrin Henein [:darrin] from comment #8)
> > I didn't think we were
> > considering rejecting the media incoming, but only controlling the outgoing
> > media. Isn't this what everyone else does?
> 
> Yes it is currently what most other people do atm. However I believe Maire
> was suggesting that we because we can reject video in WebRTC easily, then we
> could do that. This way, even though (a) is intending on sending video, (b)
> could decide not to receive it because they are on a paid or low bandwidth
> connection (without first having to connect the call, and ask the other side
> to turn video off).
> 
> Maire, am I summarising that right?

I am interested in those scenarios, but I think that should be a separate bug.  And I'm fine if that bug is lower priority -- I see it as a nice-to-have and I think it could come after MVP.

In the interest of time, I think we should limit the scope to controlling what media we send.  And file a follow up bug (prioritized separately) for what media we choose to receive.

Comment 12

[Romain Testard [:RT]]

I agree that this could be useful, I created a bug to track this post MVP:
https://bugzilla.mozilla.org/show_bug.cgi?id=1030068


This bug is still valid as a way for the callee to offer audio+video or audio only to the caller when answering an incoming call request.

Comment 13

[Romain Testard [:RT]]

We assume here that the default answering mode (pressing the button and not going through the chevron) is audio+video.

Comment 14

[Maire Reavy [:mreavy]]

Updating to match trello board

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Our previous bug went away, so after talking to Standard8, Andrei and I are grabbing this one as it's the next untouched bug in the queue.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

In playing around as we were implementing this, we realized that the UX for an audio-only call (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-active-audio) hasn't been implemented yet, and as a result, audio-only calls look terrible.

As such, this bug needs to depend on getting on that being implemented first, which we're not going to have time to do before uplift.

That said, we've done a lot of valuable work in making the incoming call view start to use pieces of the specified UX (ie it looks better) using React.  We very much do want _that_ work in before uplift, because it includes a number of important strings.
That work has been moved to bug 1040901, which we do hope to land today.

The only remaining work for this bug, I think, is:

* make the audio-only layout stuff work
* add in the drop-down menu items and hook them up

I'm pulling this bug out of this sprint, and will move bug 1040901 in.

Giving shell a needinfo? on this so that she can correct any mistakes I've made here.

Comment 17

[:shell escalante]

perfect.  thanks for letting me know.

Comment 18

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Add ability to make audio only calls in Loop standalone and desktop

Comment 19

[Andrei Oprea [mention me on [e-mail]]]

Patch implements ability to start an audio only or audio-video call on standalone (bug 1048333) and answer with audio or audio-video on desktop. I've added both in the same patch because it makes sense for them to land together and it's easier to test this way. 

The UI for the audio only view will be handled by bug 1042060

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8470645 [details] [diff] [review]
Add ability to make audio only calls in Loop standalone and desktop

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

Some general comments:

- I know we're not doing much css at the moment, but I was expecting the microphone symbols in the drop-down menu. They appear to be there on the standalone side, but not the desktop side.

- The UX mock-up appears to have changed, for the desktop side at least, to just a two-part button with no drop-down:

https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming

Its unclear to me if we want to add all the sub-menu infra just to remove it later, or if we should get clarification if its wanted for standalone UI as well. However, both of these feel like scope creep for this bug, so maybe we should just move forward as-is at the moment, and have a follow-up for re-laying out with whatever the new UX is.


I've not looked at the tests, as it seemed like some of the comments might have a significant affect on them.

::: browser/components/loop/content/js/conversation.jsx
@@ +28,5 @@
>      },
>  
> +    getInitialProps: {
> +      video: {enabled: true},
> +      audio: {enabled: true}

These are never actually used, hence processing the received callType isn't actually doing anything.

I know there was a comment somewhere for basing the default for the accept button on the incoming callType (can't find it atm), but its unclear what the intentions are in this patch.

@@ +230,5 @@
>          // background worker on the desktop client.
>          // Bug 1032700 should fix this.
> +        this._conversation.setIncomingSessionData(sessionData[0]);
> +        var callType = this._conversation.get("callType");
> +        var videoStream = callType === "audio" ? false : true;

This seems the wrong way around, why not just:

var videoStream = callType === "audio-video"

?

@@ +286,5 @@
>          return;
>        }
>  
> +      var callType = this._conversation.get("selectedCallType");
> +      var videoStream = callType === "audio" ? false : true;

Ditto, though this is starting to suggest a helper function would be useful.

::: browser/components/loop/content/shared/js/models.js
@@ +15,5 @@
>    var ConversationModel = Backbone.Model.extend({
>      defaults: {
> +      connected:    false,         // Session connected flag
> +      ongoing:      false,         // Ongoing call flag
> +      callType:     "audio-video", // Audio or audio-video calls

I think this would be better defaulted as undefined. It gets overwritten in setIncomingSessionData anyway, and undefined is a better flag to a dev if they are doing something wrong (like trying to use callType on the standalone UI).

Also, I think selectedCallType should be added and documented here.

Both should be explicit about in which circumstances they are for (e.g. callType is for info from the 'other' peer, whereas selectedCallType is the user's selected type).

@@ +154,5 @@
> +      this.set({
> +        sessionId:    sessionData.sessionId,
> +        sessionToken: sessionData.sessionToken,
> +        apiKey:       sessionData.apiKey,
> +        callType:     sessionData.callType

It is unclear when we'll have loop-server 0.10.0 released, so I think maybe keeping a default value here would be a good idea: sessionData.callType || "audio-video"

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +158,5 @@
>      },
>  
>      componentDidMount: function() {
> +      /* Listen for events & hide dropdown menu if user clicks away */
> +      window.addEventListener('click', this.clickHandler);

nit: use double-quotes please (several places)

@@ +180,4 @@
>       */
> +    _initiateOutgoingCall: function(callType) {
> +      return function() {
> +        this.props.model.set("callType", callType);

This should be selectedCallType to be consistent with the incoming call views

::: browser/components/loop/standalone/content/l10n/data.ini
@@ +24,5 @@
>  promote_firefox_hello_heading=Download Firefox to make free audio and video calls!
>  get_firefox_button=Get Firefox
>  call_url_unavailable_notification=This URL is unavailable.
>  initiate_call_button_label=Click Call to start a video chat
> +initiate_audio_video_call_button=Video call

The UX mock-up says this should be "Call":

https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-start

Maybe though we should have the "Video Call" as a tooltip.

Comment 21

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Mark Banner (:standard8) from comment #20)
> Comment on attachment 8470645 [details] [diff] [review]
> Add ability to make audio only calls in Loop standalone and desktop
> 
> Review of attachment 8470645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> ::: browser/components/loop/content/js/conversation.jsx
> @@ +28,5 @@
> >      },
> >  
> > +    getInitialProps: {
> > +      video: {enabled: true},
> > +      audio: {enabled: true}
> 
> These are never actually used, hence processing the received callType isn't
> actually doing anything.
> 
> I know there was a comment somewhere for basing the default for the accept
> button on the incoming callType (can't find it atm), but its unclear what
> the intentions are in this patch.

This was based off of Niko's comment [0]
Answering based on callType will be handled by bug 1042060

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=1042060#c6

Comment 23

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Add ability to make audio only calls in Loop standalone and desktop

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 8471712 [details] [diff] [review]
Add ability to make audio only calls in Loop standalone and desktop

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

Looking good, just a few comments left. There's also one question to ask which I'll ask in a mo.

::: browser/components/loop/standalone/content/css/webapp.css
@@ +126,5 @@
>  
> +.start-audio-only-call,
> +.start-audio-video-call {
> +  background-color: none;
> +  background-image: url("../../content/shared/img/[e-mail]");

Please drop "../content" from all of these urls, as currently it will break when we put it on the dev site.

@@ +133,5 @@
> +  background-repeat: no-repeat;
> +  cursor: pointer;
> +}
> +
> +  .start-audio-only-call {

nit: inconsistent indentation in this file.

@@ +147,5 @@
> +  background-size: 20px;
> +}
> +
> +  .start-audio-video-call {
> +    background-image: url("../../content/shared/img/video-inverse-14x14.png");

This can be collapsed into the rule above - no need to be separate.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +157,5 @@
>        client: React.PropTypes.object.isRequired
>      },
>  
>      componentDidMount: function() {
> +      /* Listen for events & hide dropdown menu if user clicks away */

Nit: please use // style comments

@@ +175,5 @@
>      },
>  
>      /**
>       * Initiates the call.
> +     * Returns a function that can be used to initiate audio or audio-video calls.

I don't understand why this needs to return a function, and I couldn't find anything obvious in the react docs.

If this is really needed, then I think we need a short comment as to why, as otherwise people not familiar with react may get confused (same in conversation.jsx).

@@ +219,2 @@
>      render: function() {
> +      var cx = React.addons.classSet;

I don't see the need to alias this here.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +461,5 @@
> +        sinon.assert.calledWith(model.trigger, "change:selectedCallType");
> +        sinon.assert.calledWith(model.trigger, "change");
> +      });
> +
> +        it("should set selectedCallType to audio-video", function() {

nit: this test and the next are indented two spaces too far.

Comment 25

[Mark Banner (:standard8)]

Dan: Are you and Mike happy to include this patch in the conversation css reorg patch?

The button currently looks like it needs the layout tweaking on desktop. So we either need to fix it here, or in the css reorg bug.

Comment 26

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Mark Banner (:standard8) from comment #24)
> Comment on attachment 8471712 [details] [diff] [review]
> Add ability to make audio only calls in Loop standalone and desktop
> 
> Review of attachment 8471712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +175,5 @@
> >      },
> >  
> >      /**
> >       * Initiates the call.
> > +     * Returns a function that can be used to initiate audio or audio-video calls.
> 
> I don't understand why this needs to return a function, and I couldn't find
> anything obvious in the react docs.
> 
> If this is really needed, then I think we need a short comment as to why, as
> otherwise people not familiar with react may get confused (same in
> conversation.jsx).
> 

Just so we don't have two different functions one for initiating an audio call and one for audio-video

Comment 27

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Mark Banner (:standard8) from comment #24)
> Comment on attachment 8471712 [details] [diff] [review]
> Add ability to make audio only calls in Loop standalone and desktop
> 
> Review of attachment 8471712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +133,5 @@
> > +  background-repeat: no-repeat;
> > +  cursor: pointer;
> > +}
> > +
> > +  .start-audio-only-call {
> 
> nit: inconsistent indentation in this file.
> 

It's not a bug it's a feature. It indented because it's related to the above block of rules, it overrides some of them and doesn't make much sense on its own. Same as :hover rules. But I can switch it back.

Comment 28

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Mark Banner (:standard8) from comment #24)
> Comment on attachment 8471712 [details] [diff] [review]
> Add ability to make audio only calls in Loop standalone and desktop
> 
> Review of attachment 8471712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +219,2 @@
> >      render: function() {
> > +      var cx = React.addons.classSet;
> 
> I don't see the need to alias this here.

Used to generate "dropdownMenuClasses"

Comment 29

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Add ability to make audio only calls in Loop standalone and desktop

Comment 30

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #28)
> (In reply to Mark Banner (:standard8) from comment #24)
> > Comment on attachment 8471712 [details] [diff] [review]
> > Add ability to make audio only calls in Loop standalone and desktop
> > 
> > Review of attachment 8471712 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +219,2 @@
> > >      render: function() {
> > > +      var cx = React.addons.classSet;
> > 
> > I don't see the need to alias this here.
> 
> Used to generate "dropdownMenuClasses"

What I meant is that you might as well just use React.addons.classSet directly, rather than have the intermediate cx.

Comment 31

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Add ability to make audio only calls in Loop standalone and desktop

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 8473801 [details] [diff] [review]
Add ability to make audio only calls in Loop standalone and desktop

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

Looks good, r=Standard8

Comment 33

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/55b8364d966e

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55b8364d966e

Comment 35

[u279076]

Looks like this landed with tests. Does this need QA testing?

Comment 36

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #35)
> Looks like this landed with tests. Does this need QA testing?

Andrei, can you please answer this?

Comment 37

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #36)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #35)
> > Looks like this landed with tests. Does this need QA testing?
> 
> Andrei, can you please answer this?

This patch does not require testing. The tests are enough for this feature. Thanks.

Comment 38

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #37)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #36)
> > (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #35)
> > > Looks like this landed with tests. Does this need QA testing?
> > 
> > Andrei, can you please answer this?
> 
> This patch does not require testing. The tests are enough for this feature.
> Thanks.

I think I disagree. Whilst the tests are enough on a unit test level, we don't actually have a functional tests for this. Hence, end-to-end it could break, even though the unit tests pass.

I think the tests should be fairly easy to base on the user story.

Comment 39

[Paul Silaghi, QA [:pauly]]

Verified on FF 35 in bug 1042060

Comment 40

[u279076]

Thanks for verifying this Paul. 

Mark, the user story will be covered by smoketests.