Closed Bug 990678 Opened 6 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

(firefox34 verified)

VERIFIED FIXED
mozilla34
Tracking Status
firefox34 --- verified

People

(Reporter: standard8, Assigned: aoprea)

References

Details

(Whiteboard: [p=1])

User Story

As a FF browser user (signed-in or not), I can answer an incoming call with audio only or audio+ video, so that I can decide if the caller will see me or not.

Approximate UX:

- Accept with audio only / Accept with Audio+Video / Reject buttons are displayed (need to check the reject versus decline options with UX).
- If "Accept with Audio+Video" is chosen, the call is connected with audio and video and starts.
- If "Accept with Audio only" is chosen, the call is connected with audio only and starts.

To implement:

- Need to implement a drop-down menu to select the above
- Resultant values passed to the publish method in the sdk

Attachments

(3 files, 3 obsolete files)

No description provided.
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.
Blocks: 972014
No longer blocks: 974875
User Story: (updated)
Depends on: 1000134
User Story: (updated)
Depends on: 1000761
Depends on: 1000762
Priority: -- → P1
Target Milestone: --- → mozilla32
No longer blocks: 972014
Blocks: 1000146
Whiteboard: [needs UX]
Whiteboard: [needs UX] → [needs UX][s=ui32]
Depends on: 1011472
Whiteboard: [needs UX][s=ui32] → [p=?]
Target Milestone: mozilla32 → mozilla33
Blocks: 1020451
Blocks: 1012743
Blocks: 1022590
No longer blocks: 1012743
No longer depends on: 1000134
No longer depends on: 1000761
No longer depends on: 1011472
User Story: (updated)
Summary: Implement UI for accepting/rejecting a call → Desktop client needs ability to answer a call
Depends on: 1011472
No longer depends on: 1015486
No longer depends on: 1011472
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?
User Story: (updated)
Flags: needinfo?(dhenein)
Summary: Desktop client needs ability to answer a call → Ability to answer a call as audio+video or audio only
Whiteboard: [p=?] → [p=?][need ux clarification]
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"
(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.
Flags: needinfo?(dhenein) → needinfo?(rtestard)
(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"
Flags: needinfo?(rtestard)
> 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?
Flags: needinfo?(rtestard)
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
Flags: needinfo?(rtestard)
(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?
Flags: needinfo?(mreavy)
Whiteboard: [p=?][need ux clarification] → [p=1][need ux clarification]
Blocks: 1029436
(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.
Flags: needinfo?(mreavy)
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.
We assume here that the default answering mode (pressing the button and not going through the chevron) is audio+video.
User Story: (updated)
Updating to match trello board
Assignee: nobody → nperriault
Assignee: nperriault → 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.
Whiteboard: [p=1][need ux clarification] → [p=1]
Target Milestone: mozilla33 → 33 Sprint 3- 7/21
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.
Depends on: 1040901
Flags: needinfo?(sescalante)
Whiteboard: [p=1] → [p=?]
Target Milestone: 33 Sprint 3- 7/21 → 34 Sprint 1- 8/4
perfect.  thanks for letting me know.
Flags: needinfo?(sescalante)
Whiteboard: [p=?] → [p=1]
Depends on: 1042060
Assignee: dmose → aoprea
Depends on: 1048333
Blocks: 1048333
No longer depends on: 1048333
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
Attachment #8470645 - Flags: review?(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.
Attachment #8470645 - Flags: review?(standard8) → review-
(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
Duplicate of this bug: 1048333
Attachment #8471712 - Flags: review?(standard8)
No longer depends on: 1042060
Status: NEW → ASSIGNED
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/audio-default-16x16@1.5x.png");

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.
Attachment #8471712 - Flags: review?(standard8) → feedback+
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.
Flags: needinfo?(dmose)
(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
(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.
(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"
(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.
Attachment #8473131 - Flags: review?(standard8)
Attachment #8473801 - Flags: review?(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
Attachment #8473801 - Flags: review?(standard8) → review+
https://hg.mozilla.org/integration/fx-team/rev/55b8364d966e
Flags: needinfo?(dmose)
Target Milestone: 34 Sprint 1- 8/4 → mozilla34
https://hg.mozilla.org/mozilla-central/rev/55b8364d966e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Looks like this landed with tests. Does this need QA testing?
Whiteboard: [p=1] → [p=1][qa?]
(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?
Flags: qe-verify?
Flags: needinfo?(aoprea)
Whiteboard: [p=1][qa?] → [p=1]
(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.
Flags: needinfo?(aoprea)
(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.
Verified on FF 35 in bug 1042060
Status: RESOLVED → VERIFIED
Thanks for verifying this Paul. 

Mark, the user story will be covered by smoketests.
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
Depends on: 1068594
You need to log in before you can comment on or make changes to this bug.