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

VERIFIED FIXED

Status

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: lolimartinezcr, Assigned: gtorodelvalle)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.1 affected)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Flags: needinfo?(vpg)
(Reporter)

Comment 1

5 years ago
Also Keypad and microphone buttons should be disabled while call is "connecting"
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
Summary: While call is "connecting", "add other call" button should be disabled. → While call is "connecting", "Add other call", "Keypad" and "microphone" buttons should be disabled.
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
Yes, they have been implemented like that, but Loli is right, we should fix this.

Thanks!
Flags: needinfo?(vpg)
Paco, can you work on it?
Thanks a lot!
Flags: needinfo?(pacorampas)
(Reporter)

Updated

5 years ago
QA Contact: lolimartinezcr

Updated

5 years ago
Assignee: nobody → pacorampas
Flags: needinfo?(pacorampas)
Created attachment 8441921 [details] [review]
patch in github
Attachment #8441921 - Flags: review?(anthony)
Created attachment 8441935 [details]
new-call-disabled-enabled.png
Created attachment 8441937 [details]
new-call-disabled-enabled.png
Attachment #8441935 - Attachment is obsolete: true
Duplicate of this bug: 1021657
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)
Flags: needinfo?(cawang)
Attachment #8441921 - Flags: review?(anthony)
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.
(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)
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.
Flags: needinfo?(cawang)
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 ;)
Created attachment 8448569 [details]
patch in github
Attachment #8441921 - Attachment is obsolete: true
Created attachment 8448571 [details] [review]
patch in github
Attachment #8448569 - Attachment is obsolete: true

Updated

5 years ago
Attachment #8448571 - Flags: review?(anthony)
Now, it is only disable the add call button, like Carrie said.
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 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?
Attachment #8448571 - Flags: review?(anthony) → review-
Flags: needinfo?(htsai)
(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
(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.
Flags: needinfo?(htsai)
(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 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.
Attachment #8448571 - Flags: review- → review?(anthony)
Comment on attachment 8448571 [details] [review]
patch in github

Sorry, it is not :( Comments in Github.
Attachment #8448571 - Flags: review?(anthony) → review-
Assignee: pacorampas → gtorodelvalle
Created attachment 8457945 [details]
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 :)
Attachment #8448571 - Attachment is obsolete: true
Attachment #8457945 - Flags: review?(anthony)
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!
Flags: needinfo?(cawang)
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?
Flags: needinfo?(htsai)
Germán, Carrie: Let's open a new bug for that.
(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.
Flags: needinfo?(htsai)
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?
Attachment #8457945 - Flags: review?(anthony) → review-
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!
Flags: needinfo?(cawang)
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 ;)
Attachment #8457945 - Flags: review- → review?(anthony)
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.
Attachment #8457945 - Flags: review?(anthony) → review-
Comment on attachment 8457945 [details]
21867.html

Comments included in the patch ;) Thanks Anthony!
Attachment #8457945 - Flags: review- → review?(anthony)
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
Attachment #8457945 - Flags: review?(anthony) → review-
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+ ;)
Attachment #8457945 - Flags: review- → review?(anthony)
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.
Attachment #8457945 - Flags: review?(anthony) → review-
Attachment #8457945 - Flags: review?(anthony) → review+
Landed in master: https://github.com/mozilla-b2g/gaia/commit/87a968f356a82e1123582a8dcd41e1df69f3c163
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Depends on: 1050196
(Reporter)

Comment 40

4 years ago
Tested and working
2.1
Flame
Gecko-d5a4ba9
Gaia-80a41e0

2.0
Flame
Gecko-7f737a2
Gaia-6155d46

Hamachi
1.4
Gecko-63a5966
Gaia-31cd873
Status: RESOLVED → VERIFIED
(Reporter)

Comment 41

4 years ago
(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?
Status: VERIFIED → RESOLVED
Last Resolved: 4 years ago4 years ago
(Reporter)

Updated

4 years ago
Flags: needinfo?(oteo)
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.
status-b2g-v1.4: affected → wontfix
status-b2g-v2.0: affected → wontfix
(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!
Flags: needinfo?(oteo)
(Reporter)

Comment 44

4 years ago
(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
Status: RESOLVED → VERIFIED
Depends on: 1065953
You need to log in before you can comment on or make changes to this bug.