Screen turns on on notification during a call

RESOLVED FIXED in Firefox 45

Status

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gerard-majax, Assigned: gsvelto, NeedInfo)

Tracking

(Blocks 1 bug, {foxfood})

unspecified
2.6 S2 - 12/4
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox45 fixed, b2g-v2.5 affected, b2g-master verified)

Details

Attachments

(3 attachments, 3 obsolete attachments)

[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
Is this something that should be done on System app or Dialer side?
Flags: needinfo?(gsvelto)
Flags: needinfo?(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.
Flags: needinfo?(etienne)
Quick fix incoming.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Flags: needinfo?(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?
Attachment #8665942 - Flags: review?(etienne)
Attachment #8665942 - Flags: feedback?(lissyx+mozillians)
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)
Attachment #8665942 - Flags: feedback?(lissyx+mozillians)
(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.
(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.
(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 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.
Attachment #8665942 - Flags: review?(etienne)
Seems like a dogfood issue we need to fix.
blocking-b2g: 2.5? → 2.5+
Priority: -- → P2
(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?
Gabriele, what are the next steps here?
Ask UX for feedback. Sorry for the delay, I completely forgot this was a 2.5+ blocker.
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?
Flags: needinfo?(rmacdonald)
See Also: → 1216525
See Also: 1216525
Duplicate of this bug: 1216525
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 ?
(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.
Adding UX team as NI
Flags: needinfo?(firefoxos-ux-bugzilla)
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?
Flags: needinfo?(rmacdonald)
Removing UX NI as Rob responded. Thanks!
Flags: needinfo?(firefoxos-ux-bugzilla)
Excellent, tomorrow I'll modify the code so that vibrations can happen even when the screen is off.
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.
Attachment #8665942 - Flags: review?(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.
Attachment #8665942 - Flags: review?(etienne)
(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...
Flags: needinfo?(rmacdonald)
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.
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.
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.
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.
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).
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.
Attachment #8683692 - Attachment is obsolete: true
Attachment #8684251 - Flags: review?(dhylands)
Attachment #8684251 - Flags: review?(bzbarsky)
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.
Attachment #8665942 - Flags: review?(etienne)
Blocks: 1222467
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.
Attachment #8684251 - Flags: review?(bzbarsky) → review+
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.
Attachment #8684251 - Flags: review?(dhylands) → review+
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 :)
Attachment #8665942 - Flags: review?(etienne) → review+
(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.
Oh, making it a permission sounds nice.  I like that.
gecko patch with nits addressed, follow-up for the permission work coming along
Attachment #8684251 - Attachment is obsolete: true
Attachment #8684892 - Flags: review+
Attachment #8684892 - Attachment description: [PATCH v2] Do not prevent the system app from vibrating when it is hidden → [PATCH v3] Do not prevent the system app from vibrating when it is hidden
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.
Attachment #8665942 - Flags: review+ → review?(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 :)
Attachment #8665942 - Flags: review?(gsvelto) → review?(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 :)
Attachment #8665942 - Flags: review?(etienne) → review+
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.
Broke an integration test, gotta fix it first.
Try is finally green, ready to merge.
Merged to gaia/master 426ee5e5582083e124ec95e180eb78a7d3fe8256

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

Keeping the bug open in order to land the gecko patch.
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.
Keywords: checkin-needed
I forgot, the try-run for the gecko patch is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3558e1e99aea
https://hg.mozilla.org/integration/b2g-inbound/rev/fa99f07ef603692363a70eebee9b198144bbd62a
Backed out changeset 83f811be5016 (bug 1207221) for B2G JB emulator bustage due to warning
(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!
Flags: needinfo?(nigelbabu)
Try two, we need to land attachment 8688322 [details] [diff] [review].
Keywords: checkin-needed
Blocks: 1225808
https://hg.mozilla.org/mozilla-central/rev/5106b290f12a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S2 - 12/4
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
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.