966147 - Email Notification doesnt go away when I click on it

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Comment 1

[James Burke [:jrburke]]

This started happening because of a consequence of bug 890440, and a historical misunderstanding on what the spec thinks should happen for the system closing notifications. This is discussed in bug 930794, and my current understanding is if bug 930794 is fixed, then it means the system would also close notifications that were tapped/activated, as the user has "dealt with" the notification at that point.

So I would prefer to see bug 930794 fixed, as that will help all apps that may have also been affected by the changes in bug 890440. However, if that is unlikely to happen for 1.4, then I can put in a workaround in the email app to close them, even though I do not think for the overall platform it is good to do this, since other apps would still be affected by the change in behavior.

:mhenretty, to confirm with you:

* Will a fix for bug 930794 also mean that if the user taps a notification in the system tray/toast, the system will close it, the app does not need to explicitly close it?
* Is a fix likely for 1.4? If not, do you know of other apps affected by bug 89044, or is it mostly the email app now?

Comment 2

[Julien Wajsberg [:julienw]]

So, well, this behavior was really specified to work like this after long discussions between essentially Alexandre Lissy and me.

Let me explain some use cases.

Say, for email, that you have a notification like "you have 32 unread emails"
Do we want to remove it as soon as the user clicks it ?
or do we want to decrement "32" while the user is reading ?
I mean, both works, but this is someting the email app should control itself, this is an applicative decision. The system app should not take that decision.

Now, for WapPush, we need to remove the notification only when it's been deleted from the DB, otherwise we have no mean to show the message again as the WapPush app has no "inbox" view.

So you see that the current behavior is the only way to handle these use cases. It's more flexible.

So here Email is a special case, because you switched to using the new Notification API, but when it still had the old behavior. All other apps were switched to the new Notification API adter this behavior was implemented and taking this into account.

Therefore, I think this bug should be fixed by making the email app closing the notification itself.

That said, I agree that the most common use case is "the user clicks, the notification disappears", and therefore maybe the Notification API could include "hints" to indicate whether it's a sticky notification or not. Sadly it does not provide this yet.

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

Julien said it better than I could.

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

Nominating for 1.3. The reason is, bug 977593 is a cert blocker, which depends on bug 890440 being fixed. Since we are now need to uplift this (bug 890440), email notifications will break in 1.3 in the same way they did in master which caused this bug. So I'm afraid we will need this fixed in 1.3 now.

Comment 5

[James Burke [:jrburke]]

Attachment: GitHub pull request

When our notification handler gets called, get all the notifications, and find the matching one, then manually close it.

No test since this is likely needed for 1.3 and 1.3 has the less stable email test snapshot.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8390933 [details] [review]
GitHub pull request

I confirm via code inspection and device testing (buri/hamachi) that this causes us to close the notification tapped on.  In the case there were multiple notifications from e-mail, only the one tapped on goes away.

Comment 7

[James Burke [:jrburke]]

Merged in gaia master:
https://github.com/mozilla-b2g/gaia/commit/1e55e0c931d5ea7406cf8c2f41f4901e8feb3eda

from pull request:
https://github.com/mozilla-b2g/gaia/pull/17172

Comment 8

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Please request approval-gaia-v1.3 on this patch when you feel it is ready for 1.3 uplift.

Comment 9

[James Burke [:jrburke]]

Comment on attachment 8390933 [details] [review]
GitHub pull request

As comment 4 mentions, since bug 890440 is going to 1.3, this bug should go with it:

[Bug caused by] (feature/regressing bug #):
890440 uplift to 1.3

[User impact] if declined:
Email notifications will not clear from the notification tray when the user taps on the notification.

[Testing completed]:
Dev tested on device, and with multiple accounts, to confirm only the targeted account notification is closed.

[Risk to taking this patch] (and alternatives if risky):
Low. The patch feature detects all pieces used. The only notable risk would be any issues with the Notification.get API in 1.3.

If this patch is not taken, it means either being OK with the user impact or considering backing out of bug 890440 for 1.3. But if bug 890440 is in 1.3, this is the only way I know of to get the notification removed without user interaction.

[String changes made]:
None

Comment 10

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

v1.3: 5d111addc9148d737425cf37472fc9e16ad8273f

Comment 11

[Edward Chen[:edchen]]

[Environment]
Gaia      8f802237927c7d5e024fb7dca054dd5efef6b2e6
Gecko     https://hg.mozilla.org/mozilla-central/rev/907cacf958de
BuildID   20140316160201
Version   30.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

[Result]
PASS
Bug fixed, I mark it to "VERIFIED"

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Thank you for the fast turnaround here James!