856074 - Controls on the call screen should be disabled until the call is reported connecting when dialing emergency number in airplane mode

Status: RESOLVED FIXED

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

Description

[Anshul]

STR:
Prerequisites are that fixes for bug 839363 and bug 849185 should be available in the build.

1. Enable Airplane mode
2. Make an emergency call from the dialer
3. Before gecko could place the emergency call, press the end key.

Observed behavior: The oncall screen goes away but no RIL:Hangup request is sent to gecko. So gecko continues to place the call after airplane mode is turned off and signal is available.

Expected Behavior: I think the possible fix here could be to keep the disconnect option disabled until the call is reported dialing. Possibly need to involve UX folks.

Comment 1

[Wayne Chang [:wchang]]

can we get a video of this issue?
Also was this done on the ril build or Mozril?

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Jamie can you weigh in on the expected behaviour here?

Comment 3

[Anshul]

Wayne, this was done with commercial RIL but I can easily reproduce this issue with Moz RIL. Will upload the video shortly.

Comment 4

[Wayne Chang [:wchang]]

:etienne - Is this something you can check from dialer first?

Comment 5

[Anshul]

I have uploaded a video at http://youtu.be/LM4FhEp75dg. There are two issues I have captured.

1. As soon as the airplane mode is enabled and before the call is actually connected, if I press the hangup button, the RIL:Hangup request is not sent down to gecko so when the voice service is available the telephony code places the call and that is when you would see the call screen showing up again. I think we should disable the hangup button until the call is actually connected (this is how Android behaves).

2. I see an incoming call like UI with an answer button and an end button instead of outgoing call UI.

I pulled in the patch at bug 839363 to enable the UI to place an emergency call. I wasn't able to reproduce the scenario with Moz RIL as with Moz RIL dial request itself is failing.

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Anshul from comment #5)
> I have uploaded a video at http://youtu.be/LM4FhEp75dg. There are two issues
> I have captured.
> 
> 1. As soon as the airplane mode is enabled and before the call is actually
> connected, if I press the hangup button, the RIL:Hangup request is not sent
> down to gecko so when the voice service is available the telephony code
> places the call and that is when you would see the call screen showing up
> again. I think we should disable the hangup button until the call is
> actually connected (this is how Android behaves).
> 
> 2. I see an incoming call like UI with an answer button and an end button
> instead of outgoing call UI.
> 
> I pulled in the patch at bug 839363 to enable the UI to place an emergency
> call. I wasn't able to reproduce the scenario with Moz RIL as with Moz RIL
> dial request itself is failing.

Interesting... looks like by the time the first call screen is fully loaded the call is not there yet. And by the time the call does come in the call screen is already closed so we open a new one.

Maybe we should add a bit of logic in the call screen where, if the radio is disabled, we wait for the settingchange event telling us it's enabled before displaying the controls.
(Android does something similar I think where they show an "activating GSM..." message.)

But of course if we do this we need to make sure that the call screen never gets open in cases where the radio is disabled and will stay disabled.

Comment 7

[Anshul]

Eitenne, I see the same issue when dialing an emergency number through the emergency dialer in the phone lock screen in airplane mode. So would you be working on this bug?

Also, the fix https://github.com/mozilla-b2g/gaia/pull/8887 for bug 850119 doesn't even let user make a call when in APM.

Comment 8

[jachen]

https://bugzilla.mozilla.org/show_bug.cgi?id=856074#c7

I'm concerned about the bug listed in that comment doesn't allow user to make an emergency call in APM.

I'm fine that the controls should be disabled until the call is reported connected.

Comment 9

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Steal it ;)

I can reproduce this on master today.

Comment 10

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: WIP patch

WIP patch here.

Etienne, could you give some feedback for this change? looks not so smooth, maybe phone is busy for dialing.

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 745093 [details] [diff] [review]
WIP patch

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

Looks like a hidden |setTimeout(, 200)| :)
And it's delaying the opening of the call screen, which we should definitely avoid.

I'm not in favor of adding an arbitrary timeout for this, but if we had too it would be better to just enable the hang up button after a timeout rather than delaying the whole call screen.

This looks like one of the numerous bug that would benefit from bug 823958 being fixed.

Comment 12

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

I moved toggleScreen from setup() to onCallsChanged() because I found another problem which is we show two different layouts when emergency call. first one have two buttons, second only have one button, please see this:

https://docs.google.com/a/mozilla.com/file/d/0B-FHjXny08MtTTJSWGR6cG9qNEU/edit

root cause is we take too long time from disable airplane mode to dial emergency call. that's why I wrote this patch for sovling two problems in one commit...

but you are right, I should just disable hang up button until addCall (or a timeout).

Comment 13

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: pull request: https://github.com/mozilla-b2g/gaia/pull/9566

I will file a follow-up bug for two different layouts when emergency call.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 745775 [details]
pull request: https://github.com/mozilla-b2g/gaia/pull/9566

We can't do this without risking completely blocking the phone (until reboot) in cases like bug 860614.

We _always_ need to let the user close the call screen "no questions asks" since, without bug 823958, we can't guarantee that the call screen being open strictly means we have at least one call.

Comment 15

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

this bug blocked by bug 823958.

Comment 16

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

un-assigned this because I'm working on unpredictably ceritification bugs for cusotmization.

Comment 17

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

Hi guys! I was considering assigning this bug to me but I am not able to reproduce it. The point is that having a look at the code, I see no reason why I am not able to reproduce it but for timing ones. This is, I do not get the "Airplane mode" deactivated and the attention screen shown so I can hang up the call before Gecko has already established the call. Am I missing anything here? Or do we just want to protect the Dialer from the case that I do not manage to reproduce from happening? Thanks!

Comment 18

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

BTW, I even deleted the 0.5s from the animation to try to speed the showing of the attention screen but no even in this case I am able to get the attention screen shown after the airplane mode has been deactivated but before Gecko has established the call :-(

Comment 19

[Anshul]

I tested again with the latest gecko/gaia and I still see both the issues I reported in comment #5. Although issue #1 reported in comment #5 is not reproducible every single time now. I still see the issue #2. I have attached the video of the issue with commercial RIL. With Moz RIL the DIAL request is failing so I am not able to reproduce the issue.

Comment 20

[Anshul]

Attachment: Hang up dialing emergency call when in APM mode

Comment 21

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

Geez! That's a kind of long emergency number you have around there :-O

BTW, this is the behavior I am getting right now (latest Gecko and Gaia) in Spain: https://www.dropbox.com/s/jzkff4k9qmo65nw/2013-05-30%2010.10.25.mp4 (the size of the video was bigger than the one Bugzilla allows for attachments :-( )

Comment 22

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

Apart from this, in your video the animations shown to move from the Settings app to the home screen and then to the Communications app does not seem to be the latest ones :-O Please, no offense but are you sure you are using latest Gecko and Gaia? :-) Or the video you just attached is an old one but the behavior persists using latest Gecko and Gaia :-)

Comment 23

[Carlos Martínez Toral [:carlosmartinez]]

Hi all, I´ve tested in a unagi device with Gecko-517ec8d.Gaia-6849f19 from master and I´m getting the same behaviour described in comment #21. If you hang up fast enough, before the call is connected, device is rebooted as well.

Comment 24

[Anshul]

I still see the same issue as I reported in comment #20 with the tip of gecko and gaia from 1.1.

gecko: 1287520f4ae5ef7711a7e8ca307ebbe3c2f0fdaf
gaia: 5919649b3ab60759ad2b55dc7e38b4cfaec7d7b9

Could you please try with commercial RIL. I suspect the issue could be the timing as commercial RIL starts the dial request only after there is a confirmation that the call can be made unlike moz RIL which might attempt to dial a call sooner.

Comment 25

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

Hi guys! and specially to Anshul ;-) You are right, using Gecko-b7e2734.Gaia-e1c59ba from 1.1 I am able to get the same behavior as you. Thank you very much for your comments!

Taking into consideration the solution provided to bug 823958 (although it appears reopen due to some regression issues if I am not wrong), I would say that it would be enough to move the toggle screen code from https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/oncall.js#L244 to https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/oncall.js#L364 (just before the closing '}'). This way we will be showing the attention screen with the right rendering and once the user can act upon it (to hang it up or whatever). I have tested it and it works and the showing of the attention screen does not seem to be penalized (regarding the time taken until it is shown).

As suggested by Etienne, another option would be to show the attention screen ASAP (leaving the toggle screen code where it is now) including disabled controls and to show the final rendering (and controls) once the callschanged is notified and the call is added (addCall()). But for this I guess we need some magic UX touch and help ;-)

Etienne, would you be still in favor of the second option or would you like to try the first one first (I could prepare a PR)? :-)

Comment 26

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

Ups, forgot to request info from Etienne ;-) Thanks!

Comment 27

[Etienne Segonzac (:etienne)]

(In reply to gtorodelvalle from comment #25)
> Hi guys! and specially to Anshul ;-) You are right, using
> Gecko-b7e2734.Gaia-e1c59ba from 1.1 I am able to get the same behavior as
> you. Thank you very much for your comments!
> 
> Taking into consideration the solution provided to bug 823958 (although it
> appears reopen due to some regression issues if I am not wrong), I would say
> that it would be enough to move the toggle screen code from
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/oncall.js#L244 to
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/oncall.js#L364 (just before the closing '}'). This way we will be showing
> the attention screen with the right rendering and once the user can act upon
> it (to hang it up or whatever). I have tested it and it works and the
> showing of the attention screen does not seem to be penalized (regarding the
> time taken until it is shown).
> 
> As suggested by Etienne, another option would be to show the attention
> screen ASAP (leaving the toggle screen code where it is now) including
> disabled controls and to show the final rendering (and controls) once the
> callschanged is notified and the call is added (addCall()). But for this I
> guess we need some magic UX touch and help ;-)
> 
> Etienne, would you be still in favor of the second option or would you like
> to try the first one first (I could prepare a PR)? :-)

Thanks to the incredible work on bug 823958 we're now guaranteed to get |onCallsChanged()| called at least once.

So at the end of the |onCallsChanged()| function we can now:
- toggle the callscreen if we have some handledCalls and the screen is not displayed yet
- close the attention screen if we don't have any handledCalls (we're already doing that).

Sounds good?

Comment 28

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

Attachment: Associated PR.

Comment 29

[Etienne Segonzac (:etienne)]

(In reply to gtorodelvalle from comment #25)
> I have tested it and it works and the
> showing of the attention screen does not seem to be penalized (regarding the
> time taken until it is shown).
> 

Wow.
Did some testing with the last patch and I'm having very different results...
Looks like it's adding 1 or 2 rings of delay.

I'll try with a more recent gecko. But we may need to profile this a bit more rigorously, and potentially rethink the solution.

Comment 30

[Etienne Segonzac (:etienne)]

(In reply to Etienne Segonzac (:etienne) from comment #29)
> But we may need to profile this a bit
> more rigorously

The first result are actually very good, we're opening the call screen _sooner_ with your patch :)

(I think my weird delay comes from the SIP phone I'm using to call the B2G phone, digging...)

Comment 31

[Etienne Segonzac (:etienne)]

Comment on attachment 757863 [details]
Associated PR.

Sorry for delaying the review.

Did some thorough testing and this works wonderfully, thanks :)

Comment 32

[Etienne Segonzac (:etienne)]

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

Comment 33

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted d579fe58cf38b1d83dfd87aaca43988a8bb25037 to:
v1-train: c0e93b4f0ad432b8b1f69e71f0a86b62e580cb26

Comment 34

[Anshul]

Etienne, I tested this patch and it works very well. The issue is resolved. Thanks for your support.