1176297 - Add ability to end phone calls with power button

Status: VERIFIED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

User should be able to end the phone call by pressing the power button. This is an accessibility requirement.

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Francisco/Gabriele,
Can dialer team take this bug? 
This is a must-have feature to launch RedTai so we need a 2.2r (2.2 based) patch before Sep/End.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

I think this should be pretty trivial to add but I'd like to have UX feedback on this first. The reason is that this is a major behavioral change compared to the previous versions and I've found myself many times pushing the power button just to turn off the screen during a call (because the proximity sensor wasn't working or was misbehaving). I'll prepare a patch in the meantime and if UX gives us a green light we can land it.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

NI Harly to have a look.

Hi Eitan, 

I wonder why this behavior is an accessibility feature?
Maybe because it's easier to end a call with power button?

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button > mozilla-b2g:master

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

This is a PR that adds the requested functionality pref'd off. To enable it just add the dialer.power_button_hangup pref and set it to true. I've designed this to work similarly to what happens on iOS devices: upon pressing the power button all the current calls are hung up. After a certain delay the screen is then turned off.

The patch is not perfect nor does it have tests; I'll finish it only once we've got UX approval that this is exactly what we need or if it requires changes. Note that this patch applies to the system app basically as I have to "get in the way" of the screen manager to make this work.

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #3)
> NI Harly to have a look.
> 
> Hi Eitan, 
> 
> I wonder why this behavior is an accessibility feature?
> Maybe because it's easier to end a call with power button?

I'm honestly not sure. It allows easier operation, and it is a U.S. accessibility requirement.

Comment 7

[Harly Hsu[:harly]]

For general users, I don't really see a need for this, but I am fine with having this as long as it is an accessibility requirement.

Comment 8

[Wesly Huang (EPM)]

Hi Eitan:

Just a quick check, is this one still moving toward 9/25 ETA?

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to Wesly Huang (TAM) from comment #8)
> Hi Eitan:
> 
> Just a quick check, is this one still moving toward 9/25 ETA?

That would be a question to the person assigned to it. Gabriele?

Comment 10

[Wesly Huang (EPM)]

Thanks Eitan, my bad that not check it carefully  :)

@Gabriele, would you kindly share us the progress? Thank you!

(In reply to Eitan Isaacson [:eeejay] from comment #9)
> That would be a question to the person assigned to it. Gabriele?

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

As I mentioned in comment 2 I've got a working patch that uses the approach there (BTW I've just rebased it on top of the current master). I don't have any UX for this though and I've currently implemented it behind a pref because I don't know if we want this for master or not. If the approach I described in comment 2 is fine then I'll add tests, ask for reviews and land this but I'd really love to have UX feedback first.

Comment 12

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I think Harly in comment 7 shows the green light to land it.
As this is US regulation, 2.2R will be in need earlier even than master.

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

OK, I'll make a patch for v2.2r with this feature not behind a pref and then we'll see if we want it on master too.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button-v2.2r > mozilla-b2g:v2.2r

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

OK, here's a v2.2r PR, I'll add the necessary tests and submit it for review ASAP.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8667831 [details] [review]
[gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button-v2.2r > mozilla-b2g:v2.2r

This is the v2.2r patch which is more urgent. I'd like to have a proper UX spec before landing this on master.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8667831 [details] [review]
[gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button-v2.2r > mozilla-b2g:v2.2r

I have concern on the setTimeout call -- you will likely cause races with user like a bug titled "The screen turned off in 2 sec after call ends regardless of user interaction".

What's the state of the screen at that moment? Should we keep the timeoutId and clearTimeout when we know we no longer want to turn the screen off?

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #17)
> I have concern on the setTimeout call -- you will likely cause races with
> user like a bug titled "The screen turned off in 2 sec after call ends
> regardless of user interaction".

I agree that it's annoying and race-prone but I don't have an UX spec for this bug and since normally we display the callscreen for two seconds at the end of the last call I thought that we should wait before turning off the screen for the same amount of time. I've checked iOS and it does something similar when you end a call via the power button (display the ended call for a couple of secs, then turn off the screen), not sure about Android though.

> What's the state of the screen at that moment? Should we keep the timeoutId
> and clearTimeout when we know we no longer want to turn the screen off?

If the screen is on then the callscreen will be displayed with information about the last call and then turned off. If the screen is already off then nothing should happen (unless the user turns it on again in the 2s window, in which case it will be turned off again). I might refine the logic there if you prefer changing behavior depending on the state of the screen or I might get rid of the delay entirely if you think that's the best way to go (and if the UX people agree).

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

Harly, we need clearer UX here before I can proceed. My current patch does the following:

1. Upon pressing the power button hang up all calls
2. Display the end-of-call callscreen visual for two seconds (the normal amount when hanging up the last call)
3. Turn off the screen after those two seconds

As Tim pointed out this tends to run into corner cases (what happens if the user presses the power button again before the two seconds have elapsed? Or if the screen is already off when the button was pressed and then it's turned on again?) so I think we need a clear UX spec before proceeding.

Comment 20

[Harly Hsu[:harly]]

Ni Morpheus to help on the spec as this is related to UX on phone calls.

Comment 21

[Morpheus Chen [:morpheus] UX]

Hi all, 

I've tested this feature on several major phones, including iOS, Android (Nexus, HTC, etc.), and I noticed some interesting differences. Here I list the main patterns below: 

iOS, HTC, etc 

1. 1st tap on PWR button sets ringtone mute but keep it calling.
2. 2nd tap on PWR button ends the phone call directly.
3. After call ends, screen is still on until timeout.

Nexus: 

Tapping on PWR button directly ends the phone call and turns off screen at the same time.

Based on the discoveries, I would suggest following Gabriele's proposal but making some changes. 

1. If sound profile isn't mute, 1st tap on PWR button sets ringtone mute but keep it calling. Then, 2nd tap on PWR button ends the phone call directly.

2. If sound profile has been muted already, tapping on PWR button ends the phone call directly.

3. Display the screen of end call for two seconds.

4. After two seconds, dismiss the screen of end call. Then go back to previous screen or show lock screen with missed call notification if screen lock on. 

5. The screen automatically sleeps after screen timeout or user can tap PWR again to turn off screen.

For corner cases in or after those 2 seconds, user taps PWR button to turn off screen immediately. 

Let me know if any questions, thanks.

Comment 22

[Gabriele Svelto [:gsvelto] (PTO)]

In your proposal the interaction is quite complex; for example I'm not sure if I can easily check the current ringtone profile without risking races with the rest of the code. I can implement it but that's adding quite a bit of risk to a last-minute patch.

Comment 23

[Morpheus Chen [:morpheus] UX]

For the conditions on 1& 2, in fact, iOS and HTC don't provide the mechanism to check the current ringtone profile. If sound profile is mute already, the first tap will do nothing, then the second tap will end the call. So do you think iOS has this problem too? 

If we don't provide the conditions to check sound profile, will this be more feasible?

Comment 24

[Gabriele Svelto [:gsvelto] (PTO)]

I studied the code a bit and it turns out that muting the ringtone is not as hard as I thought during the first tap. Detecting the volume is also possible so I'll try to implement the complete behavior you proposed in comment 21. If I can't we can fallback to the behavior where the first tap does nothing.

Comment 25

[Gabriele Svelto [:gsvelto] (PTO)]

Actually 1 is already happening, here's the relevant snippet:

https://github.com/mozilla-b2g/gaia/blob/fe8ddca526058c2fe3328b1dfc96cdbfdcf43148/apps/system/js/dialer_agent.js#L149

Comment 26

[Morpheus Chen [:morpheus] UX]

Sounds great, thanks for your clarification. Let me know if any questions, thanks.

Comment 27

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8638523 [details] [review]
[gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button > mozilla-b2g:master

Here's an updated, simplified PR that more closely match the UX in comment 22. In a nutshell I've implemented this by delegating the handling of the power button to the dialer agent while we're on a call. This prevents races with the screen manager code and yields robust behavior. I haven't updated the unit-tests yet as I want to be sure that my method is sound first.

I still need to figure out a few things though hence I've got another question for Morpheus. In this patch we've got the following behavior when we press the power button:

1) If the screen is on and the ringtone is playing mute the ringtone
2) If the screen is on and there's no ringtone or it's been muted already, hangup the calls
3) If the screen is off, turn on the screen

I'm not sure about 3, shall I treat the power button differently if the screen is off or not?

(quick note, I've just realized that Tim is OOO for a week, switching to Etienne since we'll need to land this ASAP)

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8638523 [details] [review]
[gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button > mozilla-b2g:master

I like it!
It's super clean, and looks like the telephony lookup is basically free and won't delay the actual turning on/off of the screen.

f+, ready to be unit tested! (which this approach makes easier too :))

Comment 29

[Morpheus Chen [:morpheus] UX]

> 3) If the screen is off, turn on the screen
> 
> I'm not sure about 3, shall I treat the power button differently if the
> screen is off or not?

I might need to double confirm here. I thought there will be no case 3 since there is an incoming call, and it means the screen is automatically on and displaying calling screen. Is there something I misunderstand here?

Comment 30

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Morpheus Chen [:morpheus]  UX from comment #29)
> I might need to double confirm here. I thought there will be no case 3 since
> there is an incoming call, and it means the screen is automatically on and
> displaying calling screen. Is there something I misunderstand here?

In my patch the power button hangs up a call even when we're during a call. Maybe I've misunderstood it; I though that pressing the power button would hang up any call, not just the incoming ones. I've seen that other devices do it so I thought we would go along.

Comment 31

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8638523 [details] [review]
[gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button > mozilla-b2g:master

Thanks for the feedback, here's the patch with unit-tests added. You'll notice that I've done more modifications to the unit-tests than just adding the new ones. I've replaced whenever possible the old custom mocks that were being used with the shared one; I've ensured that the setup and teardown methods of those mocks are properly called and finally I've modified tests that were leaking event listeners by forcing them to unregister whatever event listeners had been previously registered.

Comment 32

[Morpheus Chen [:morpheus] UX]

(In reply to Gabriele Svelto [:gsvelto] from comment #30)

> In my patch the power button hangs up a call even when we're during a call.
> Maybe I've misunderstood it; I though that pressing the power button would
> hang up any call, not just the incoming ones. I've seen that other devices
> do it so I thought we would go along.

Hi Gabriele,

I've checked other phones, including iOS, Nexus and HTC. It seems most phones hang up calls when pressing PWR, only HTC turns off the screen when pressing PWR. Based on these discoveries, I suggest if screen is off, turning on the screen instead of hanging up call, which is exactly your proposal. This will prevent the situation that user might accidentally hang up call.

Comment 33

[Gabriele Svelto [:gsvelto] (PTO)]

Excellent, thanks for the feedback Morpheus.

Comment 34

[Etienne Segonzac (:etienne)]

Comment on attachment 8638523 [details] [review]
[gaia] gabrielesvelto:bug-1176297-hangup-calls-via-power-button > mozilla-b2g:master

r=me with nits :)

Comment 35

[Gabriele Svelto [:gsvelto] (PTO)]

Thanks for the review, I've addressed the nits and pushed again. I'll wait for a green try before landing.

Comment 36

[Gabriele Svelto [:gsvelto] (PTO)]

Merged to gaia/master 87014859cb97cc8a75c614aadbf2b91d99908296

https://github.com/mozilla-b2g/gaia/commit/87014859cb97cc8a75c614aadbf2b91d99908296

I'll be uplifting to v2.2r shortly.

Comment 37

[Jessica]

Attachment: verified_Aries KK v2.5.3gp

According to the STR of Comment 0, this bug has been verified as pass on latest Aries KK v2.5 and Flame KK v2.5.

Actual results: User is able to end the phone call by pressing the power button.
See attachment: verified_Aries KK v2.5.3gp
Reproduce rate: 0/6

Device: Aries KK v2.5 (Pass)
Build ID               20151015193337
Gaia Revision          8ea9029190af2ffeb04dcd97b323738125e31a0e
Gaia Date              2015-10-15 14:30:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d374d16cbb251c9dac5af69f8e186e821ce82fe2
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151015.185317
Firmware Date          Thu Oct 15 18:53:25 UTC 2015
Bootloader             s1

Device: Flame KK v2.5 (Pass)
Build ID               20151015150343
Gaia Revision          8ea9029190af2ffeb04dcd97b323738125e31a0e
Gaia Date              2015-10-15 14:30:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d374d16cbb251c9dac5af69f8e186e821ce82fe2
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151015.183044
Firmware Date          Thu Oct 15 18:30:54 EDT 2015
Firmware Version       v18D v4 
Bootloader             L1TC000118D0

Comment 38

[Gabriele Svelto [:gsvelto] (PTO)]

Uplifted to v2.2r/128316f250ac28b094659cbc8e9b2d5d7aa3a555

https://github.com/mozilla-b2g/gaia/commit/128316f250ac28b094659cbc8e9b2d5d7aa3a555

Comment 39

[Ram Dayal Vaishnav [:ram]]

I dont see any way by which we can disable this option, but I feel there should be an option in settings to disable this feature. Logged separate bug for this - https://bugzilla.mozilla.org/show_bug.cgi?id=1219817