we should not launch the app when we close a notification

VERIFIED FIXED in Firefox 21

Status

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
STR:
* launch the app
* receive a SMS
* kill the app (long press home button and swipe up the app)
* click on the "clear all notification" in the notification screen

Expected:
* the notifications are cleared and that's it

Actual:
* the notifications are cleared and the Sms app is opened

With the attached patch, I can see tha
(Assignee)

Updated

6 years ago
Assignee: nobody → felash
(Assignee)

Comment 1

6 years ago
Created attachment 708561 [details] [diff] [review]
patch v1

one liner which check the type of the notification and doesn't launch the app if the user merely closed it
Attachment #708561 - Flags: review?(21)
(Assignee)

Comment 2

6 years ago
Comment on attachment 708561 [details] [diff] [review]
patch v1

carry forward r+ from :vingtetun which reads my patch over my shoulders
Attachment #708561 - Flags: review?(21) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 3

6 years ago
We need a tef+ on that because this affects _all_ apps which uses notification handlers.
(Assignee)

Comment 4

6 years ago
+ this is very low risk as there are only 2 cases and I tested both cases.
(Assignee)

Comment 5

6 years ago
Comment on attachment 708561 [details] [diff] [review]
patch v1

obsoleting, we have a new use case that needs to be taken care of
Attachment #708561 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 832082
If we fix it the way we suggest, which is still sending back the event and fixing the listeners to check if the notification has been cancelled or not we would have to fix the following call sites:

apps/communications/dialer/js/dialer.js:  window.navigator.mozSetMessageHandler('notification', handleNotification);
apps/costcontrol/js/app.js:    window.navigator.mozSetMessageHandler('notification',
apps/sms/js/sms.js:  window.navigator.mozSetMessageHandler('notification',
Keywords: checkin-needed
Tracking, but we won't tef+ until we find/land a fix for v1.x.
tracking-b2g18: ? → +
This blocks a CS blocker so it should be tef+.
blocking-b2g: tef? → tef+
(Assignee)

Comment 9

6 years ago
Created attachment 710576 [details] [diff] [review]
gaia patch v1

We'll receive a notification if the user clicks on the notif or if he closes it,
but we want to do stuff only when the user clicks on it.

The gecko patch needs to land first.
---
 apps/communications/dialer/js/dialer.js |    6 ++++-
 apps/costcontrol/js/app.js              |    7 +++++-
 apps/sms/js/sms.js                      |   37 ++++++++++++++++++-------------
 3 files changed, 32 insertions(+), 18 deletions(-)
Attachment #710576 - Flags: review?(21)
(Assignee)

Comment 10

6 years ago
Created attachment 710577 [details] [diff] [review]
gecko patch v2

Bug 836737 - we should not launch the app when we close a notification a=tef+

add the topic in the message
Attachment #710577 - Flags: review?(21)
(Assignee)

Comment 11

6 years ago
Created attachment 710578 [details] [diff] [review]
gaia patch v1

(same patch, bigger context.)

We'll receive a notification if the user clicks on the notif or if he closes it,
but we want to do stuff only when the user clicks on it.

The gecko patch needs to land first.
---
 apps/communications/dialer/js/dialer.js |    6 ++++-
 apps/costcontrol/js/app.js              |    7 +++++-
 apps/sms/js/sms.js                      |   37 ++++++++++++++++++-------------
 3 files changed, 32 insertions(+), 18 deletions(-)
Attachment #710576 - Attachment is obsolete: true
Attachment #710576 - Flags: review?(21)
Attachment #710578 - Flags: review?(21)
Comment on attachment 710577 [details] [diff] [review]
gecko patch v2

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

Let's discuss about this flag before r+'ing it.

::: b2g/chrome/content/shell.js
@@ +677,2 @@
>          gSystemMessenger.sendMessage("notification", {
> +            topic: topic,

I wonder if we really want to pass the topic or if we want to pass a boolean flag instead?
Attachment #710577 - Flags: review?(21)
Comment on attachment 710578 [details] [diff] [review]
gaia patch v1

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

I'm fine with the idea of the patch - but this should be changed if we decided to use a boolean flag.
Attachment #710578 - Flags: review?(21)
(Assignee)

Comment 14

6 years ago
Created attachment 711296 [details] [diff] [review]
gaia patch v2

use a boolean instead of the topic value
Attachment #710578 - Attachment is obsolete: true
Attachment #711296 - Flags: review?(21)
(Assignee)

Comment 15

6 years ago
Created attachment 711297 [details] [diff] [review]
gecko patch v3

we use a boolean instead of the topic value now
Attachment #710577 - Attachment is obsolete: true
Attachment #711297 - Flags: review?(21)
(Assignee)

Comment 16

6 years ago
checkin-needed for the gecko patch.

I'll land the gaia patch myself after this.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24995693c6ea
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g18/rev/fd1acc2aa727
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/a9b537435d6b
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → B2G C4 (2jan on)
(Assignee)

Comment 20

6 years ago
reseting the flags as I need to land the gaia part now.
Status: RESOLVED → REOPENED
status-b2g18: fixed → affected
status-b2g18-v1.0.0: fixed → affected
status-firefox21: fixed → affected
Resolution: FIXED → ---
(Assignee)

Comment 21

6 years ago
gaia master: https://github.com/julienw/gaia/commit/7cec8abf54f661238ba4d4765e53f61485f54643

need uplift on the correct branches on gaia
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
status-firefox21: affected → fixed
(Assignee)

Comment 22

6 years ago
we need an uplift here.

(and I'd like an explanation of which flags need to be set to reliably trigger an uplift :) )
Keywords: checkin-needed
(Assignee)

Comment 23

6 years ago
changing components as this may make this bug visible :)
Component: General → Gaia
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
status-b2g18-v1.0.1: --- → affected
(In reply to Julien Wajsberg [:julienw] from comment #21)
> gaia master:
> https://github.com/julienw/gaia/commit/
> 7cec8abf54f661238ba4d4765e53f61485f54643
> 
> need uplift on the correct branches on gaia

Looks like this commit is on the required branches:

$ git branch --contains 7cec8abf54f661238ba4d4765e53f61485f54643
  master
  v1-train
  v1.0.0
* v1.0.1
status-b2g18: affected → fixed
status-b2g18-v1.0.0: affected → fixed
status-b2g18-v1.0.1: affected → fixed
Keywords: checkin-needed
(Assignee)

Comment 26

6 years ago
Seems like I've put a wrong commit.

The good commit is : https://github.com/julienw/gaia/commit/8683cf6d9cc428b73b438bbed24e4f48970bd8a1

sorry about that.
status-b2g18: fixed → affected
status-b2g18-v1.0.0: fixed → affected
status-b2g18-v1.0.1: fixed → affected
I thought it was a little weird that the bug numbers didn't match up.

v1-train: 8d413db447d50281853b9005374373f7d2fd2b5e
v1.0.1: 9eb15be2aee695828abcc3321fb7c0e5e4bb87a6
v1.0.0: 95ca8df0bff9295b71de967465f60ccf42d2390f
status-b2g18: affected → fixed
status-b2g18-v1.0.0: affected → fixed
status-b2g18-v1.0.1: affected → fixed

Comment 28

6 years ago
Verified Fixed in Unagi build #20130215070202
Dec 5th Kernel
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a9e4f8912607
Gaia   21ba59d933c66024cb351c2379315301d5352e0c
Update Channel: Nightly
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.