1207221 - Screen turns on on notification during a call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Status bar, Utility tray, Notification, defect, P2)

Description

[:gerard-majax]

[Blocking Requested - why for this release]: huge pain on daily use

Be on a call, receive a notification (Calendar for example). Get annoyed because the screen has been re-enabled and your accidently enabled mute mode. Repeat often.

STR:
 0. Place or receive a call
 1. Get a notification

Expected:
 My screen stays off because I am on a call and I don't want to accidentally enable some features

Actual
 My screen turns on and I have several times enabled mute or speaker mode

Comment 1

[:gerard-majax]

Is this something that should be done on System app or Dialer side?

Comment 2

[Etienne Segonzac (:etienne)]

(In reply to Alexandre LISSY :gerard-majax from comment #1)
> Is this something that should be done on System app or Dialer side?

System. Probably screen_manager.js.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

Comment 4

[Gabriele Svelto [:gsvelto]]

Quick fix incoming.

Comment 5

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

This prevents the notification manager from turning on the screen if any calls are present. Note that I haven't checked explicitly if there's an active call or not, with this patch the screen won't be turned on even if all the currently connected calls are on hold. Let me know if you want this changed, the fix would be trivial enough (i.e. check for telephony.active instead of the number of calls).

Alexandre can you give this a spin and see if it fixes the problem for you?

Comment 6

[:gerard-majax]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

I don't really understand why, but testing this it looks like it will delay the vibration of the device until I turn on the screen myself (by unblocking proximity sensor for example)

Comment 7

[Gabriele Svelto [:gsvelto]]

(In reply to Alexandre LISSY :gerard-majax from comment #6)
> I don't really understand why, but testing this it looks like it will delay
> the vibration of the device until I turn on the screen myself (by unblocking
> proximity sensor for example)

That is really bizarre as the two should be completely unrelated. I wonder if that's caused by wakelocks or something, I'll have to check it myself.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> (In reply to Alexandre LISSY :gerard-majax from comment #6)
> > I don't really understand why, but testing this it looks like it will delay
> > the vibration of the device until I turn on the screen myself (by unblocking
> > proximity sensor for example)
> 
> That is really bizarre as the two should be completely unrelated. I wonder
> if that's caused by wakelocks or something, I'll have to check it myself.

We can't vibrate with the screen off.
Or at least we couldn't originally.
So we probably still have some code waiting for a mozvisibilitychange to trigger the mozVibrate call.

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Etienne Segonzac (:etienne) from comment #8)
> We can't vibrate with the screen off.
> Or at least we couldn't originally.

We should check that, I think it would be good UX to just vibrate the phone without waking up the screen. It would notify the user that something happened without bothering it.

> So we probably still have some code waiting for a mozvisibilitychange to
> trigger the mozVibrate call.

I'll have another look at the code and see if I can spot where it need to be fixed.

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

> > So we probably still have some code waiting for a mozvisibilitychange to
> > trigger the mozVibrate call.
> 
> I'll have another look at the code and see if I can spot where it need to be
> fixed.

Yep, right there:

https://github.com/mozilla-b2g/gaia/pull/32060/files#diff-f73efe7ff5306e1d8f7c36679bbb4c0aR597

Looks like there's a couple of bugs referenced too.

Comment 11

[Gregor Wagner [:gwagner]]

Seems like a dogfood issue we need to fix.

Comment 12

[Gabriele Svelto [:gsvelto]]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> Yep, right there:
> 
> https://github.com/mozilla-b2g/gaia/pull/32060/files#diff-
> f73efe7ff5306e1d8f7c36679bbb4c0aR597
> 
> Looks like there's a couple of bugs referenced too.

Ugh. So what would be the correct fix? Just leave things as-is because that's the expected behavior (vibration only when the screen turns on)? Or ask UX if we can vibrate with the screen off and get rid of that silly limitation?

Comment 13

[Gregor Wagner [:gwagner]]

Gabriele, what are the next steps here?

Comment 14

[Gabriele Svelto [:gsvelto]]

Ask UX for feedback. Sorry for the delay, I completely forgot this was a 2.5+ blocker.

Comment 15

[Gabriele Svelto [:gsvelto]]

From https://wiki.mozilla.org/Gaia/UXTeam I can see that Rob is in charge of the UX for notifications so I'm NI him directly, please change this if I'm mistaken here.

The issue here is about vibration when the screen is off. Apparently in the past we decided not to allow notifications to vibrate the phone if the screen was turned off. The vibration would be delayed to when the screen was turned on. This design choice turned out to be problematic and in bug 1030310 and bug 1050023 the vibration-when-you-turn-on-the-screen behavior was turned on.

For this bug with my patch applied we have the issue that when the user receives a notification during a call he won't feel the vibration until the screen turns on again. My take on this would be to allow all notifications to vibrate the phone when the screen is off: this is common on other mobile OSes and it allows the user to be notified even when the phone has ringtones turned off. How should we proceed?

Comment 17

[Julien Wajsberg [:julienw]]

I also thought we could have an issue when the screen turns on while the phone is in the pocket. Would this bug changes this as well ?

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> I also thought we could have an issue when the screen turns on while the
> phone is in the pocket. Would this bug changes this as well ?

No, I've limited myself to the scenario of being on a call. Personally I'd prefer that the screen doesn't turn on at all on notifications (but vibrates instead if the appropriate pref is set) but that would require additional UX feedback.

Comment 19

[Mahendra Potharaju [:mahe]]

Adding UX team as NI

Comment 20

[Rob MacDonald [:robmac]]

Sorry for the delay... Here's my quick take...

Vibration, if on, should be triggered when the event takes place... even if the screen is off. The notification should also turn on the screen temporarily. 

During calls, if the screen is off due to the proximity sensor, the screen should remain off although the vibration, if on, should still be triggered.

Ping me if you have any follow up questions.

Thanks!
Rob



(In reply to Gabriele Svelto [:gsvelto] from comment #15)
> From https://wiki.mozilla.org/Gaia/UXTeam I can see that Rob is in charge of
> the UX for notifications so I'm NI him directly, please change this if I'm
> mistaken here.
> 
> The issue here is about vibration when the screen is off. Apparently in the
> past we decided not to allow notifications to vibrate the phone if the
> screen was turned off. The vibration would be delayed to when the screen was
> turned on. This design choice turned out to be problematic and in bug
> 1030310 and bug 1050023 the vibration-when-you-turn-on-the-screen behavior
> was turned on.
> 
> For this bug with my patch applied we have the issue that when the user
> receives a notification during a call he won't feel the vibration until the
> screen turns on again. My take on this would be to allow all notifications
> to vibrate the phone when the screen is off: this is common on other mobile
> OSes and it allows the user to be notified even when the phone has ringtones
> turned off. How should we proceed?

Comment 21

[Tiffanie Shakespeare [:tif]]

Removing UX NI as Rob responded. Thanks!

Comment 22

[Gabriele Svelto [:gsvelto]]

Excellent, tomorrow I'll modify the code so that vibrations can happen even when the screen is off.

Comment 23

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

Quick change to vibrate even when the screen's off.

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

Wait. Do we have a gecko patch to enable vibrating with the screen turned off?

Because AFAIK (well, I just tested so I know for sure) even the system app can't vibrate with the screen off currently. The |navigator.vibrate| call returns false and nothing happens.

Comment 25

[Julien Wajsberg [:julienw]]

(In reply to Rob MacDonald [:robmac] from comment #20)
> Sorry for the delay... Here's my quick take...
> 
> Vibration, if on, should be triggered when the event takes place... even if
> the screen is off. The notification should also turn on the screen
> temporarily. 

> 
> During calls, if the screen is off due to the proximity sensor, the screen
> should remain off although the vibration, if on, should still be triggered.


Rob, what do you think of having this behavior also when we're not in a call ? My use case is when the phone is in a pocket...

Comment 26

[Gabriele Svelto [:gsvelto]]

So I've dug a little bit in the HAL code and found out the reason why we can't vibrate when the screen is off:

http://hg.mozilla.org/mozilla-central/file/451a18579143/hal/Hal.cpp#l127

In a nutshell vibration is disabled when requested from windows which are hidden and since when the screen is turned off all windows are hidden (duh) then we can't vibrate. The decision for this was taken in bug 679966 which is a fair bit before I got in charge of HAL and it kind of makes sense (it was mostly done in the context of a browser, not of Firefox OS).

However there seem to be a provision in there so that the check applies only to content processes (which would exclude the system app) so I have to further investigate how this code works. Since this bug is a blocker and the vibration change is not strictly needed we might even consider splitting the bug in two parts with this one only dealing with the screen behavior.

Comment 27

[Gabriele Svelto [:gsvelto]]

OK, this is mighty bizarre. The bit of code I've pointed to above is not the reason why we're not vibrating when the screen is off... still digging.

Comment 28

[Gabriele Svelto [:gsvelto]]

I think I found our culprit:

http://hg.mozilla.org/mozilla-central/file/451a18579143/dom/base/Navigator.cpp#l860

It seems like we have two policies to decide who should vibrate and who shouldn't, one in the navigator object and one in the HAL. I'll try to figure out how to harmonize them.

Comment 29

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Do not prevent the system app from vibrating when it is hidden

This is a gecko patch that allows the system app to vibrate even when it's hidden. I'm not sure if this is the best way to implement it (it's quite crude) so I'll be asking for feedback especially because this makes the behavior of this code different between desktop/mobile Firefox and Firefox OS. I've also removed the duplicate condition checks we had both under the Navigator object and the HAL implementation.

Comment 30

[Gabriele Svelto [:gsvelto]]

Quick update, I'm slowly digging through to code to figure out why the previous code was behaving like it was with the IPC check in hal::Vibrate() and the redundant check in Navigator::Vibrate(). My guess is that we had different callers back then, i.e. hal::Vibrate() was being called directly rather than via navigator.vibrate() (or navigator.mozVibrator() before it was standardized).

Comment 31

[Gabriele Svelto [:gsvelto]]

Attachment: PATCH v2] Do not prevent the system app from vibrating when it is hidden

So this patch builds upon the previous one and changes our policy regarding vibration in the following way:

- On gonk/fxos the system app is always allowed to vibrate, independently of if it calls navigator.vibrate() or hal::Vibrate() directly

- On all other platforms the current behavior is unchanged (double visibility check, once in the navigator object and once in the HAL itself if the call has originated in the main process and did not arrive via IPC)

I'll post a follow up to investigate why we need the double-check and if we can get rid of it. My feeling is that we used to have direct hal::Vibrate() callers before (in Android/Fennec) so we needed the double check to enforce the policy but from what I can tell they're not there anymore. It might be worth removing that check because it would allow us to tear down a significant chunk of infrastructure within HAL that is used solely for it.

Comment 32

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

Now that we have the gecko part ready to it's time to review the gaia part too.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8684251 [details] [diff] [review]
PATCH v2] Do not prevent the system app from vibrating when it is hidden

> +MayVibrate(nsCOMPtr<nsIDocument>& doc) {

The argument should be nsIDocument*.

This is kind of icky, with the ifdefs.  Maybe we should add a (sufficiently privileged) setter on document to allow setting a boolean on it to indicate that it can vibrate even if hidden?  Would the system app be able to call such a setter if it's not exposed to the web?

r=me; I'm ok with a followup for the cleanup.

Comment 34

[Dave Hylands [:dhylands]]

Comment on attachment 8684251 [details] [diff] [review]
PATCH v2] Do not prevent the system app from vibrating when it is hidden

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

looks good to me.

Comment 35

[Etienne Segonzac (:etienne)]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

Cool, r=me with nits that I'm sure you'll like :)

Comment 36

[Gabriele Svelto [:gsvelto]]

(In reply to Boris Zbarsky [:bz] from comment #33)
> This is kind of icky, with the ifdefs.

Yes, it's pretty icky.

> Maybe we should add a (sufficiently
> privileged) setter on document to allow setting a boolean on it to indicate
> that it can vibrate even if hidden?  Would the system app be able to call
> such a setter if it's not exposed to the web?

I suppose so. It might even be a permission instead of a field. I'll file a follow up for that. Thanks for the review, I'll post an updated patch soon.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Oh, making it a permission sounds nice.  I like that.

Comment 38

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Do not prevent the system app from vibrating when it is hidden

gecko patch with nits addressed, follow-up for the permission work coming along

Comment 39

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

I've addressed the review comments including turning the open lines check into a service state and pushed a patch on top of the PR. r? again because the changes are significant enough IMHO.

Comment 40

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

Oops, wrong review request :)

Comment 41

[Etienne Segonzac (:etienne)]

Comment on attachment 8665942 [details] [review]
[gaia] gabrielesvelto:bug-1207221-do-not-turn-on-screen-during-a-call > mozilla-b2g:master

Nice! r=me with some new nits :)

Comment 42

[Gabriele Svelto [:gsvelto]]

Thanks for the review, addressed the nits and pushed again. I'll be waiting for the try run to finish then I'll land the gaia patch first followed by the gecko one.

Comment 43

[Gabriele Svelto [:gsvelto]]

Broke an integration test, gotta fix it first.

Comment 44

[Gabriele Svelto [:gsvelto]]

Try is finally green, ready to merge.

Comment 45

[Gabriele Svelto [:gsvelto]]

Merged to gaia/master 426ee5e5582083e124ec95e180eb78a7d3fe8256

https://github.com/mozilla-b2g/gaia/commit/426ee5e5582083e124ec95e180eb78a7d3fe8256

Keeping the bug open in order to land the gecko patch.

Comment 46

[Gabriele Svelto [:gsvelto]]

Now that the gaia part has landed we need to land attachment 8684892 [details] [diff] [review] in gecko then we can close the bug and proceed with uplifting both parts.

Comment 47

[Gabriele Svelto [:gsvelto]]

I forgot, the try-run for the gecko patch is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3558e1e99aea

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/83f811be5016

Comment 49

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fa99f07ef603692363a70eebee9b198144bbd62a
Backed out changeset 83f811be5016 (bug 1207221) for B2G JB emulator bustage due to warning

Comment 50

[:gerard-majax]

(In reply to Nigel Babu [:nigelb] from comment #49)
> https://hg.mozilla.org/integration/b2g-inbound/rev/
> fa99f07ef603692363a70eebee9b198144bbd62a
> Backed out changeset 83f811be5016 (bug 1207221) for B2G JB emulator bustage
> due to warning

Can you give a link to the failure? Thanks!

Comment 51

[Nigel Babu [:nigelb]]

Here you go https://treeherder.mozilla.org/logviewer.html#?job_id=3351246&repo=b2g-inbound

Comment 52

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Do not prevent the system app from vibrating when it is hidden

Revised patch, the try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8869e1615b8

Comment 53

[Gabriele Svelto [:gsvelto]]

Try two, we need to land attachment 8688322 [details] [diff] [review].

Comment 54

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/5106b290f12a

Comment 55

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5106b290f12a

Comment 56

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on the latest build of Flame KK master and Aries KK master by the STR in comment 0.

Actual results: When the screen is off and the noification receives in a call, phone just vibrates without waking up the screen.
See attachment: verified_Flame_master.3gp
Reproduce rate: 0/10


Device: Flame KK master_512mb (Pass)
Build ID               20151126172432
Gaia Revision          86959c405348d27ba5686956ae3a8ffc274d3db8
Gaia Date              2015-11-26 06:53:43
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/74c7941a9e22d50057800771ebae07f69deecc9f
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151126.164303
Firmware Date          Thu Nov 26 16:43:13 UTC 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK master (Pass)
Build ID               20151126173500
Gaia Revision          86959c405348d27ba5686956ae3a8ffc274d3db8
Gaia Date              2015-11-26 06:53:43
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/74c7941a9e22d50057800771ebae07f69deecc9f
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151126.165407
Firmware Date          Thu Nov 26 16:54:15 UTC 2015
Bootloader             s1

Comment 57

[Shally Li（leave from Mozilla）]

Attachment: verified_Flame_master.3gp