930607 - [dialer] The phone number is emptied even when the call can't be made

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

STR:
* enable airplane mode or remove the SIM card
* launch dialer
* enter a phone number
* try to call

Expected:
* there is an alert explaining why a call can't be made
* the phone number is _not_ emptied

Actual:
* there is an alert explaining why a call can't be made
* the phone number is emptied

This is IMO important because when the user is in a location with a bad reception (say, in a house, or a shop), he will probably want to try the call again in another location, and will be pissed off because he'll have to enter the phone number again.

Asking a blocking for 1.3, and NI the UX team.

Comment 1

[Stephany Wilkes]

Flagging Jacqueline, who is Dialer on Comms for 1.3.

Comment 2

[Jacqueline Savory [:jsavory] UX]

I agree with the expected result here, the phone number should not empty when an alert appears so that the user can easily try again when they have changed their settings.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

triage: should not block reelase. please land it in master when ready

Comment 4

[Julien Wajsberg [:julienw]]

Etienne, should we convert this to a mentoring bug ?

Comment 5

[Etienne Segonzac (:etienne)]

Thanks Julien, great idea!

Comment 6

[Sunny [:darkowlzz]]

Hi, 

I am working on this bug.

I found that the PhoneView is cleared by `connected` & `disconnected` in [1], which are passed to `TelephonyHelper.call` as parameters later.

On TelephonyHelper side,
`call.onconnected` and `call.ondisconnected` are called at [2], which clears the PhoneView.

On commenting out the above, PhoneView retains it's value even after a successful call, which is undesirable.
`call.onerror` seems to be invoked very late. Moving it above call.onconnected doesn't help.

Can I get some hint on how should I organize things in order to get the desired behaviour?

[1]: http://mxr.mozilla.org/gaia/source/apps/communications/dialer/js/dialer.js#250
[2]: http://mxr.mozilla.org/gaia/source/apps/communications/dialer/js/telephony_helper.js#101

Comment 7

[Etienne Segonzac (:etienne)]

Hey Sunny, thanks for looking at this bug.

And you're right, we will always get the disconnected callback, there's a nice documentation showing the various call states on the WebTelephony API page [1].

The complexity of this bug is that we still want to clean the phone number when somebody hangs up right away (before the call is connected).

So I think our best bet here is to call |KeypadManager.updatePhoneNumber| again in the |error| callback to put the number back in place (we have the number as a parameter of the |call| function).

Hsin-Yi, do you know if the call.onerror callback is guaranteed to be triggered _after_ the call.ondisconnected? (looks like it's the case).

[1] https://wiki.mozilla.org/WebAPI/WebTelephony

Comment 8

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

(In reply to Etienne Segonzac (:etienne) from comment #7)
> Hey Sunny, thanks for looking at this bug.
> 
> And you're right, we will always get the disconnected callback, there's a
> nice documentation showing the various call states on the WebTelephony API
> page [1].
> 
> The complexity of this bug is that we still want to clean the phone number
> when somebody hangs up right away (before the call is connected).
> 
> So I think our best bet here is to call |KeypadManager.updatePhoneNumber|
> again in the |error| callback to put the number back in place (we have the
> number as a parameter of the |call| function).
> 
> Hsin-Yi, do you know if the call.onerror callback is guaranteed to be
> triggered _after_ the call.ondisconnected? (looks like it's the case).
> 
> [1] https://wiki.mozilla.org/WebAPI/WebTelephony

call.onerror is triggered only when a call is released abnormally.
So Etienne's suggestion should work for this bug!

Comment 9

[Sunny [:darkowlzz]]

Attachment: Link to pull request 15477

Comment 10

[Sunny [:darkowlzz]]

Attachment: Link to pull request 15479

Comment 11

[Sunny [:darkowlzz]]

It seems that both the things I tried failed in Travis CI [1] & [2]. I don't understand what went wrong. Could someone help me out?

[1]: https://github.com/mozilla-b2g/gaia/pull/15477
[2]: https://github.com/mozilla-b2g/gaia/pull/15479

Comment 12

[Julien Wajsberg [:julienw]]

[1] is not your fault, but [2] probably is :)

for [2] look at https://travis-ci.org/mozilla-b2g/gaia/jobs/17149116 and you'll see the errors.

Comment 13

[Sunny [:darkowlzz]]

Yea, I have seen that
TypeError: number is undefined

So, the 1st PR was fine.
Sending it again.

Thanks Julien.

Comment 14

[Sunny [:darkowlzz]]

Attachment: Link to pull request 15520

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8362429 [details] [review]
Link to pull request 15520

Thanks for the update, 2 issues:

* The telephony helper can be used from other places (eg contacts app) where the |KeypadManager| isn't defined. Your change needs to live in |dialer.js|, in the error callback that we pass to the TelephonyHelper [1].

* We should use the number and not the sanitized number otherwise we might loose formatting, good thing is, you have access to it more easily from dialer.js :)

[1] https://github.com/darkowlzz/gaia/blob/bc64b2ed9c847c39995fbcb798dd985e6973b45f/apps/communications/dialer/js/dialer.js#L257

Comment 16

[Sunny [:darkowlzz]]

Attachment: Link to pull request 15541

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8362788 [details] [review]
Link to pull request 15541

A simple patch in the end, but you got to see how one of the most critical file in the dialer works :)

r=me
Thank you for fixing this bug!

Comment 18

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/f4dd126585c49245a3d3a1bb28886323a0d07651

Comment 19

[Julien Wajsberg [:julienw]]

(I'm so happy)