1024506 - While call is "connecting", "Add other call", "Keypad" and "microphone" buttons should be disabled.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Loli (:lolimartinezcr)]

Hamachi
2.0
Gecko-fcf1d2a
Gaia-2bb6663

Reproducible 100%

STRs:
1. Tap dialer application
2. Write phone number
3. Tap call button
4. While user can see "Connecting" user can see "add other call" button enable

Actual result
user can see "add other call" button enabled. I think it isn't correct because while is "connecting" isn't posible call other phone (I need info UX)

Expected result:
User can see "Add other call" button disabled.

Comment 1

[Loli (:lolimartinezcr)]

Also Keypad and microphone buttons should be disabled while call is "connecting"

Comment 2

[Maria Angeles Oteo (:oteo)]

Just commenting that this bug is not a regression and these buttons have always been enabled when the call is "connecting".

Tested on hamachi v1.4 version:
B-90
Gecko-dcfc7e2
Gaia-43da854
Platform version:30.0
BuildID:20140612024834

Comment 3

[Victoria Gerchinhoren [:vicky]]

Yes, they have been implemented like that, but Loli is right, we should fix this.

Thanks!

Comment 4

[Maria Angeles Oteo (:oteo)]

Paco, can you work on it?
Thanks a lot!

Comment 5

[Paco Rampas [:paco]]

Attachment: patch in github

Comment 6

[Paco Rampas [:paco]]

Attachment: new-call-disabled-enabled.png

Comment 7

[Paco Rampas [:paco]]

Attachment: new-call-disabled-enabled.png

Comment 9

[Anthony Ricaud (:rik)]

I'm not sure this is a desirable UX. Let me share two scenarios:

- I'm calling a service center. I know that as soon as it answers, I'll have to type numbers to navigate. So I open the keyboard while it is connecting.
- I'm setting up a conference call. I dial the first person. While I wait for the first person to answer, I start looking for the second one.

Carrie: What do you think?

(I'm clearing the review while we wait for UX input)

Comment 10

[Anthony Ricaud (:rik)]

For each solution, I think it's better to disable the buttons that do the actions. So disabling call buttons and keypad buttons. Not the buttons that open the keyboard or display the dialer.

Comment 11

[Maria Angeles Oteo (:oteo)]

(In reply to Anthony Ricaud (:rik) from comment #9)
> I'm not sure this is a desirable UX. Let me share two scenarios:
> 
> - I'm calling a service center. I know that as soon as it answers, I'll have
> to type numbers to navigate. So I open the keyboard while it is connecting.
> - I'm setting up a conference call. I dial the first person. While I wait
> for the first person to answer, I start looking for the second one.

You are right, in fact if I am in a noisy environment and I have to join to a conference call I would like also to mute my microphone before the call is successfully established.

I'm checking in my i-phone and the only option that is disabled is the option to make a multiparty call. The option to open the address book is available and if I go to a contact and try to call him (while the call has not been established) I am returned to the call (no action is taken).
If I do the same after the call has been connected, the first call is put on hold and the second one (with the contact I have selected) is started.

The only weird thing that I have seen on FxOS is that if I click on "Add calls" so Contact is opened, go to details and click on a phone number, while connecting, nothing happens (which seems to be ok) however if after connecting the call I click again on the phone number no action is done (which is not ok)

Comment 12

[Carrie Wang [New account: [e-mail]]]

My suggestion would be only disable the "Add call" button here. Because users can't really connect to a second call at that moment, the button is useless in this scenario and might cause confusion. 

p.s. I can't reproduce Maria's problem here. I can add a call successfully when there is one call connected.

Comment 13

[Maria Angeles Oteo (:oteo)]

I continue seeing that issue in 2.0 latest version following this STR:
1. A starts a call with contact B
2. During the connection, A clicks on "Add Call" button in the call screen
3. Contacts is opened and A select contact C in the Agenda
4. When A press on contact C telephone number in details, nothing happens, what's is ok
5. B answers and the call is established between A and B
6. After finishing the call, when you try to call to other contact of the Agenda from contact details, the call button does not work.

Anyway, if we implement Carrie's proposal and disable the "Add call", this bug would disappear ;)

Comment 14

[Paco Rampas [:paco]]

Attachment: patch in github

Comment 15

[Paco Rampas [:paco]]

Comment on attachment 8448569 [details]
patch in github

>https://github.com/mozilla-b2g/gaia/pull/21217

Comment 16

[Paco Rampas [:paco]]

Attachment: patch in github

Comment 17

[Paco Rampas [:paco]]

Now, it is only disable the add call button, like Carrie said.

Comment 18

[Anthony Ricaud (:rik)]

Carrie: If we can disable all the buttons that actually place a call (in the call log, in contacts, etc) when we cannot place a call, would that be a better UX than disabling this callscreen button?

We can take this patch to improve the current situation but we should probably open another bug to do even better in the future.

Comment 19

[Anthony Ricaud (:rik)]

Comment on attachment 8448571 [details] [review]
patch in github

I think the logic in this patch is wrong. We should disable the "place new call" button when there is at least one call that is in the "connecting" state. And re-enable it when there is no calls in the connecting state.

Hsin-Yi: Can you confirm that those are the only cases when we cannot place a new call?

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Carrie Wang [:carrie] from comment #12)
> My suggestion would be only disable the "Add call" button here. Because
> users can't really connect to a second call at that moment, the button is
> useless in this scenario and might cause confusion. 
> 
> p.s. I can't reproduce Maria's problem here. I can add a call successfully
> when there is one call connected.

Echo on Carrie's suggestion.

We can't disable keypad even the call is connecting, not connected yet. It's due to the 3gpp spec definition. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=852314#c13

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Anthony Ricaud (:rik) from comment #19)
> Comment on attachment 8448571 [details] [review]
> patch in github
> 
> I think the logic in this patch is wrong. 

Agree the logic is wrong.

> We should disable the "place new
> call" button when there is at least one call that is in the "connecting"
> state. And re-enable it when there is no calls in the connecting state.
> 
> Hsin-Yi: Can you confirm that those are the only cases when we cannot place
> a new call?

Rik, you are right. And to be more precise, if there's at least one call with "dialing" or "alerting" state. That's when user sees 'Connecting' on call screen.

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> (In reply to Anthony Ricaud (:rik) from comment #19)
> > Comment on attachment 8448571 [details] [review]
> > patch in github
> > 
> > I think the logic in this patch is wrong. 
> 
> Agree the logic is wrong.
> 
> > We should disable the "place new
> > call" button when there is at least one call that is in the "connecting"
> > state. And re-enable it when there is no calls in the connecting state.
> > 
> > Hsin-Yi: Can you confirm that those are the only cases when we cannot place
> > a new call?
> 
> Rik, you are right. And to be more precise, if there's at least one call
> with "dialing" or "alerting" state. That's when user sees 'Connecting' on
> call screen.

And here is the list of thorough call states:
http://dxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyCall.cpp?from=TelephonyCall.cpp#68-99

Comment 23

[Paco Rampas [:paco]]

Comment on attachment 8448571 [details] [review]
patch in github

I have updated the patch. I think currently the logic is like your comment 19. Also, I have changed the issues that you said to me on github. 

There are two commits on patch because I have just started the changes of tests.

Comment 24

[Anthony Ricaud (:rik)]

Comment on attachment 8448571 [details] [review]
patch in github

Sorry, it is not :( Comments in Github.

Comment 25

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 21867.html

Hi Anthony! I prepared a new PR since the previous one was causing some weird behaviours and this way it is faster :)

As you will see I have to update calls_handler.js and handled_call.js to get the desired behavior since the oncallschange does not notify (at least it wasn't) when a call evolves from 'dialing' to 'connected' for example.

Any comments are more than welcome ;)

P.D.: The testing of this patch was kind of weird due to bug 1039553, but I would say I am pretty confident about it :)

Comment 26

[Germán Toro del Valle (:gtorodelvalle)]

Hi again! I noticed something which may end up being confusing to the user. The thing is that currently the "Place new call" button is kept enabled when there are 2 ongoing calls (1 held and 1 ongoing, I mean) which does not have much sense since it is not possible to establish a third one. What do you think about this, Carrie?

In case you want to change the current behaviour we could deal with it in this very bug or open a new one for it ;) Thanks!

Comment 27

[Anthony Ricaud (:rik)]

Germàn: Before reviewing, I'd like to check if Gecko should send us that oncallschanged.

Hsin-Yi: I thought that Telephony.oncallschanged was fired every time after all the  TelephonyCall.statechange are fired. We don't receive a oncallschanged when a call is going from 'dialing' to 'connected'. Is that expected?

Comment 28

[Anthony Ricaud (:rik)]

Germán, Carrie: Let's open a new bug for that.

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Anthony Ricaud (:rik) from comment #27)
> Germàn: Before reviewing, I'd like to check if Gecko should send us that
> oncallschanged.
> 
> Hsin-Yi: I thought that Telephony.oncallschanged was fired every time after
> all the  TelephonyCall.statechange are fired.

No, it's not the design.
Telephony.oncallschanged is fired when 1) Telephony.calls array is loaded and sync. with backend 2) when the number of Telephony.calls array changes, i.e. a new call is added or an existing call is removed

> We don't receive a
> oncallschanged when a call is going from 'dialing' to 'connected'. Is that
> expected?

It's intentional as there is call state change but no the number of calls change.

Comment 30

[Anthony Ricaud (:rik)]

Comment on attachment 8457945 [details]
21867.html

I think a cleaner approach is to have aCall.addEventListener('statechange', CallsHandler.updatePlaceNewCall.bind(CallsHandler))

And updatePlaceNewCall does something like (pseudoish-code):
var isDialing = telephony.calls.some( call => call.state == 'dialing' or 'alerting')
if (isDialing) CallScreen.disablePlaceNewCall()
else CallScreen.enablePlaceNewCall()


Also, I don't understand the changes in ConferenceGroupHandler, could you explain them?

Comment 31

[Carrie Wang [New account: [e-mail]]]

Hi, 

So, our rule is that if there is a call connecting, we will disable the add call button. In this case, if there are two calls, one is on hold and the other is connecting, then yes, we shall disable the add call button here. Thanks!

Comment 32

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8457945 [details]
21867.html

Yeah, I would say that with this approach the code is better grouped. The only drawback is that we check if there is an alerting or dialing call and update the "place new call" enabled/disabled button status (this does not mean it may change) on every call status change, but I guess you already counted on that ;)

Comment 33

[Anthony Ricaud (:rik)]

Comment on attachment 8457945 [details]
21867.html

We're missing the tests that disable the place new call button (when we receive another call or place another call).

I've left two small comments on Github too.

Comment 34

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8457945 [details]
21867.html

Comments included in the patch ;) Thanks Anthony!

Comment 35

[Anthony Ricaud (:rik)]

Comment on attachment 8457945 [details]
21867.html

On top of my Github comments, we still need the tests in the suites:
telephony.oncallschanged handling > receiving a first incoming call
telephony.oncallschanged handling > receiving an extra incoming call

Comment 36

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8457945 [details]
21867.html

I included those additional tests and the comments you left in Github ;-) BTW, I left some comments regarding the addition of listeners in mock objects as well as using spies instead of updating properties in them.

If you consider it appropriate, please do not hesitate to contact me via IRC once you have a look at the patch again since it would let us speed the landing up :) Thanks!

Last but not least, I included a new commit with the latest changes as you always ask me to but I always forget. We can always squash all the commits once you set the r+ ;)

Comment 37

[Anthony Ricaud (:rik)]

Comment on attachment 8457945 [details]
21867.html

We still need one more test in handled_call.js that checks that we install the proper statechange listener.

Comment 38

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8457945 [details]
21867.html

Hi Anthony,
I updated an already existent test: https://github.com/mozilla-b2g/gaia/pull/21867/files#diff-08f2a55579da54c0d78bfff6b34abaa3L144
and included the suggested one: https://github.com/mozilla-b2g/gaia/pull/21867/files#diff-08f2a55579da54c0d78bfff6b34abaa3R148
Thanks!

Comment 39

[Germán Toro del Valle (:gtorodelvalle)]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/87a968f356a82e1123582a8dcd41e1df69f3c163

Comment 40

[Loli (:lolimartinezcr)]

Tested and working
2.1
Flame
Gecko-d5a4ba9
Gaia-80a41e0

2.0
Flame
Gecko-7f737a2
Gaia-6155d46

Hamachi
1.4
Gecko-63a5966
Gaia-31cd873

Comment 41

[Loli (:lolimartinezcr)]

(In reply to Loli from comment #40)
> Tested and working
> 2.1
> Flame
> Gecko-d5a4ba9
> Gaia-80a41e0
> 
> 2.0     ----> Add call button is ENABLED
> Flame
> Gecko-7f737a2
> Gaia-6155d46
> 
> Hamachi   ----> Add call button is ENABLED
> 1.4
> Gecko-63a5966
> Gaia-31cd873

In 2.0 and 1.4 should be fixed?

Comment 42

[Anthony Ricaud (:rik)]

I'll leave the needinfo for Maria in case she disagrees. I believe this shouldn't be fixed in 1.4 and 2.0 because it's a user experience refinement.

Comment 43

[Maria Angeles Oteo (:oteo)]

(In reply to Anthony Ricaud (:rik) from comment #42)
> I'll leave the needinfo for Maria in case she disagrees. I believe this
> shouldn't be fixed in 1.4 and 2.0 because it's a user experience refinement.
Completely agree with you, it's an enhacement, not necessary for older versions.
Thanks for asking!

Comment 44

[Loli (:lolimartinezcr)]

(In reply to Maria Angeles Oteo (:oteo) PTO 4th-25th Aug from comment #43)
> (In reply to Anthony Ricaud (:rik) from comment #42)
> > I'll leave the needinfo for Maria in case she disagrees. I believe this
> > shouldn't be fixed in 1.4 and 2.0 because it's a user experience refinement.
> Completely agree with you, it's an enhacement, not necessary for older
> versions.
> Thanks for asking!

In this case I change status to VERIFIED