If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

Firefox OS
General
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: vingtetun)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:-, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [land Gecko/Gaia at the same time])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Flags: needinfo?(21)
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.
Flags: needinfo?(21)
Justin do you have an opinion on that?
Flags: needinfo?(justin.lebar+bug)
> Should we fire an event when an application becomes foreground/background?

I wouldn't mind adding this to the mozbrowser API.
Flags: needinfo?(justin.lebar+bug)

Updated

4 years ago
blocking-b2g: leo? → -
tracking-b2g18: ? → +
Blocks: 832412
Created attachment 749878 [details] [diff] [review]
Gecko Part

Justin I tried to not add too many comments to save you some time :)
Attachment #749878 - Flags: review?(justin.lebar+bug)
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?
Flags: needinfo?(firefoxos-ux-bugzilla)
(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!
Created attachment 749917 [details] [diff] [review]
Gaia part
(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.
(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 on attachment 749917 [details] [diff] [review]
Gaia part

The patch works like a charm!
Attachment #749917 - Flags: review?(felash)
(Reporter)

Comment 11

4 years ago
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.

Updated

4 years ago
Flags: needinfo?(firefoxos-ux-bugzilla)
Fair enough. Should I file a bug for what I'm suggesting then for a 1.2 or later timeframe?
(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.
(Reporter)

Comment 14

4 years ago
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 ?
Attachment #749917 - Flags: review?(felash)
(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.
(Reporter)

Comment 16

4 years ago
Comment on attachment 749917 [details] [diff] [review]
Gaia part

r=me with the CustomEvent nit

thanks :)
Attachment #749917 - Flags: review+
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.
Attachment #749878 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/projects/birch/rev/31aeadc70844
https://github.com/mozilla-b2g/gaia/commit/6d11a1acab160b170081242290ae1bdebf0e022a
Status: NEW → ASSIGNED
Component: Gaia::System → General
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(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 :))
https://hg.mozilla.org/mozilla-central/rev/31aeadc70844
Assignee: nobody → 21
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

4 years ago
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.
Attachment #749878 - Flags: approval-mozilla-b2g18?
(Reporter)

Comment 22

4 years ago
Comment on attachment 749917 [details] [diff] [review]
Gaia part

same here, we need both the gaia and the gecko patches.
Attachment #749917 - Flags: approval-mozilla-b2g18?
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.
Keywords: verifyme
Whiteboard: [land Gecko/Gaia at the same time]

Updated

4 years ago
Attachment #749878 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment on attachment 749917 [details] [diff] [review]
Gaia part

approval-gaia-v1 isn't in this component, so I'll needinfo jhford
Attachment #749917 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

Updated

4 years ago
Flags: needinfo?(jhford)
https://hg.mozilla.org/releases/mozilla-b2g18/rev/cb4811e73a21
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
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?
Flags: needinfo?(jhford)
Flags: needinfo?(21)
Flags: in-moztrap?
(Reporter)

Comment 27

4 years ago
will uplift.
Flags: needinfo?(21) → needinfo?(felash)
(Reporter)

Comment 28

4 years ago
v1-train: f55dfaab52a23ee4443ab184dc5737127e204b34

turns out the big conflict was easy to resolve \o/ :)
status-b2g18: affected → fixed
Flags: needinfo?(felash)
1.1hd: f55dfaab52a23ee4443ab184dc5737127e204b34
status-b2g-v1.1hd: --- → fixed

Updated

4 years ago
Flags: in-moztrap? → in-moztrap?(dwatson)

Comment 30

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

https://moztrap.mozilla.org/manage/cases/?filter-id=9271
Flags: in-moztrap?(dwatson) → in-moztrap+
Status: RESOLVED → VERIFIED
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
Keywords: verifyme
Depends on: 988737
You need to log in before you can comment on or make changes to this bug.