868816 - Remove the notifications for the displayed app when we go out of the lock screen

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Julien Wajsberg [:julienw]]

in Bug 824549 we remove all notifications for one specific app when that app is being opened.

However, there is a STR where this isn't used and therefore we still see the notifications, whereas we could remove them because we have everything needed:

* launch the Sms app
* lock the phone by pressing the power button
* receive a SMS
* unlock the phone
=> the app Sms is displayed, but the notification for the new Sms is still shown.

This happens because the Sms app is "hidden" while the phone is locked, and therefore a notification is sent (which is normal), but when we unlock, we should remove the notification for the app that is currently displayed.

Vivien, do you have a suggestion on how this could be done without calling "closeNotification" directly ?

Asking leo? because this should be easy to do and would alleviate a little more the fact that we don't have the new notification spec.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I do wonder. Should we fire an event when an application becomes foreground/background? That could be used instead of a app open event to delete the related notifications. If feel like this would also be a good thing for writing tests for all the cases where the app needs to be foreground/background.

So one solution could be to implement this at the window manager level or to add an API to the mozbrowser frames so one could listen for visibilitychanged events on them directly.This way if someone outside of the window manager alter the visible state of the app it will be easier to know it. 

Then an abstraction layer in the system app could convert them in foreground/background events dispatched to the rest of the system app.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Justin do you have an opinion on that?

Comment 3

[Justin Lebar (not reading bugmail)]

> Should we fire an event when an application becomes foreground/background?

I wouldn't mind adding this to the mozbrowser API.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Gecko Part

Justin I tried to not add too many comments to save you some time :)

Comment 5

[Jason Smith [:jsmith]]

I think the proposal on this bug needs UX input before we land a fix. I question if the bug proposed here is the right behavior we want to do. For example, what does Android do in this situation? What should we do from UX's perspective?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jason Smith [:jsmith] from comment #5)
> I think the proposal on this bug needs UX input before we land a fix. 

Why? Notifications should be cleared in this case based on all the backlog of how the notification sworks with the current limitation of the spec.

So if the patch is reviewed/landed before any UX input I really don't see why we should block. If they answer before the patch land that's good. Now if UX feels strongly after the patch lands they will be able to open a bug to change the behavior!

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Gaia part

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7)
> Created attachment 749917 [details] [diff] [review]
> Gaia part

I have not tried it yet. But this is more or less how it should work.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > I think the proposal on this bug needs UX input before we land a fix. 
> 
> Why? Notifications should be cleared in this case based on all the backlog
> of how the notification sworks with the current limitation of the spec.

Because I might want to still have my notifications when I go back to the displayed app. The notifications makes sense to remove when the user actually reads the content the notifications have referenced. But if that hasn't happened, and we pull notification too early, then the user loses the opportunity to navigate from notification to the content needing to be viewed. This goes in an entirely different direction from the recent work we did on the calendar app for these use cases, so if we land this, I could see someone immediately filing a bug, calling it a regression, and noming it to block.

I'd rather avoid churn here if possible about a back and forth bug filing cycle.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> So if the patch is reviewed/landed before any UX input I really don't see
> why we should block. If they answer before the patch land that's good. Now
> if UX feels strongly after the patch lands they will be able to open a bug
> to change the behavior!

I don't know how I feel about that. This is one of those situations where someone could file a bug to reverse the direction immediately, so we could end up in a churn spin.

At a minimum, explain why the opposite case of leaving the notification behind does not make sense here when viewing the displayed app, even if the content underlying that notification has not been viewed.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 749917 [details] [diff] [review]
Gaia part

The patch works like a charm!

Comment 11

[Julien Wajsberg [:julienw]]

Jason, we're currently removing the notifications for an app when we launch that app (either from the homescreen or from a notification). That was Bug 824549. So it's just consistency.

When we'll have the new notification spec, we'll be able to do what you're suggesting, which makes sense, but that's not for 1.1. Here we're only trying to do the best we can with the tools we have.

Comment 12

[Jason Smith [:jsmith]]

Fair enough. Should I file a bug for what I'm suggesting then for a 1.2 or later timeframe?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jason Smith [:jsmith] from comment #12)
> Fair enough. Should I file a bug for what I'm suggesting then for a 1.2 or
> later timeframe?

I'm pretty sure there is already bug filed about this. The UX team is also aware of the new notification specs coming and have tons of idea for it as far as I know.

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 749917 [details] [diff] [review]
Gaia part

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

next time, please use `git format-patch -U8` instead of `git diff` :-)

some questions and a nit

::: apps/system/js/window_manager.js
@@ +486,5 @@
>  
> +  windows.addEventListener('mozbrowservisibilitychange',
> +    function visibilitychange(e) {
> +      var manifestURL = e.target.getAttribute('mozapp');
> +      var evt = document.createEvent('CustomEvent');

please use `new CustomEvent` instead of createEvent + initEvent, this is less verbose.

see https://developer.mozilla.org/en-US/docs/Web/API/Event/CustomEvent

@@ +488,5 @@
> +    function visibilitychange(e) {
> +      var manifestURL = e.target.getAttribute('mozapp');
> +      var evt = document.createEvent('CustomEvent');
> +      evt.initCustomEvent(e.detail.visible ? 'foreground' : 'background',
> +                        true, false, { manifestURL: manifestURL });

maybe a dumb idea, but wouldn't we want the entry point as well here ?

I know we don't use it in the notifications (yet ?) but I'm worried that the manifest URL is not unique. There is also the case of an launched-by-an-activity app...

ie: should we remove the dialer notifications when we launch the contacts app ?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Comment on attachment 749917 [details] [diff] [review]
> Gaia part
> 
> Review of attachment 749917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> next time, please use `git format-patch -U8` instead of `git diff` :-)
> 
> some questions and a nit
> 
> ::: apps/system/js/window_manager.js
> @@ +486,5 @@
> >  
> > +  windows.addEventListener('mozbrowservisibilitychange',
> > +    function visibilitychange(e) {
> > +      var manifestURL = e.target.getAttribute('mozapp');
> > +      var evt = document.createEvent('CustomEvent');
> 
> please use `new CustomEvent` instead of createEvent + initEvent, this is
> less verbose.
> 
> see https://developer.mozilla.org/en-US/docs/Web/API/Event/CustomEvent
> 
> @@ +488,5 @@
> > +    function visibilitychange(e) {
> > +      var manifestURL = e.target.getAttribute('mozapp');
> > +      var evt = document.createEvent('CustomEvent');
> > +      evt.initCustomEvent(e.detail.visible ? 'foreground' : 'background',
> > +                        true, false, { manifestURL: manifestURL });
> 
> maybe a dumb idea, but wouldn't we want the entry point as well here ?
> 
> I know we don't use it in the notifications (yet ?) but I'm worried that the
> manifest URL is not unique. There is also the case of an
> launched-by-an-activity app...
> 
> ie: should we remove the dialer notifications when we launch the contacts
> app ?

I can only agree. But this sounds out of the scope of this bug that does not change any behavior. Sounds like a new bug to me.

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 749917 [details] [diff] [review]
Gaia part

r=me with the CustomEvent nit

thanks :)

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 749878 [details] [diff] [review]
Gecko Part

Sorry it took me so long to get to this review; I was stuck on tef bugs, and then I had to move out of my apartment.

This patch looks great.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/projects/birch/rev/31aeadc70844
https://github.com/mozilla-b2g/gaia/commit/6d11a1acab160b170081242290ae1bdebf0e022a

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Justin Lebar [:jlebar] from comment #17)
> Comment on attachment 749878 [details] [diff] [review]
> Gecko Part
> 
> Sorry it took me so long to get to this review; I was stuck on tef bugs, and
> then I had to move out of my apartment.
> 
> This patch looks great.

Thanks for all the time spent looking at my code last week Justin. Really appreciated (you're one of my favorite reviewer - even nitting/r- my language :))

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/31aeadc70844

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 749878 [details] [diff] [review]
Gecko Part

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: the notifications stay whereas the user thinks they should be removed.
Testing completed: Yes 
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

This will probably fix most sources of notification inconsistencies that have already been asked to be fixed by our partners, and that we can't because we don't have the new Notification API. Therefore this fix will helps us doing as much as possible with the tools we have.

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 749917 [details] [diff] [review]
Gaia part

same here, we need both the gaia and the gecko patches.

Comment 23

[Alex Keybl [:akeybl]]

Would be great to get verification on the STR by QA once this lands. We expect outside of those STR, this will be verified as part of normal test plans.

Comment 24

[Alex Keybl [:akeybl]]

Comment on attachment 749917 [details] [diff] [review]
Gaia part

approval-gaia-v1 isn't in this component, so I'll needinfo jhford

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/cb4811e73a21

Comment 26

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

Vivien, there was a fairly large merge conflict landing when cherry-picking to v1-train in apps/system/js/window_manager.js.  

Can you please take a look at it?

Comment 27

[Julien Wajsberg [:julienw]]

will uplift.

Comment 28

[Julien Wajsberg [:julienw]]

v1-train: f55dfaab52a23ee4443ab184dc5737127e204b34

turns out the big conflict was easy to resolve \o/ :)

Comment 29

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

1.1hd: f55dfaab52a23ee4443ab184dc5737127e204b34

Comment 30

[Darren]

Created a generic test case to cover all apps, but used SMS as example steps.

https://moztrap.mozilla.org/manage/cases/?filter-id=9271

Comment 31

[Nikolai Khristoforov]

This bug has been verified for the 1.1 build. The notification gets removed.

Device: Leo v1.1 Moz RIL
BuildID: 20131030041204
Gaia: 39b0203fa9809052c8c4d4332fef03bbaf0426fc
Gecko: 41c15ddb7216
Version: 18.0