824549 - [Open_][SMS]The top bar still shows new message notification although all new messages have been read.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P3)

Description

[Firefox_Mozilla]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11

Steps to reproduce:

Assuming that a new message has been received and not be read yet.
1. Launch message application.
2. Read the new message.



Actual results:

The new message indicator in top bar still display, but in fact there is no any unread message.


Expected results:

The new message indicator in top bar should update when new message has beed read.

Build Information:
gecko:   	 revision="3cbade1974968bb1e0fbb0c3386239715244a7a7"
gaia: 	 	 revision="aab72f365d73f624ede32b522f27d072c409e42e"
gonk-misc:   revision="654358494ba601a46ef9838debc95417ae464cc6"
dalvik:      revision="ca1f327d5acc198bb4be62fa51db2c039032c9ce"
librecovery: revision="e1bd90051c9e937221eb1f91c94e3cde747311a7"
moztt:       revision="6ee1f8987ef36d688f97064c003ad57849dfadf2"
external/jsmin:    revision="cec896f0affaa0226c02605ad28d42df1bc0e393"
external/opensans: revision="b5b4c226ca1d71e936153cf679dda6d3d60e2354"
device/qcom/b2g_common/mozilla-b2g: revision="41c17a6abfd5f488ec99d9aa246f5b07583403c7"

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Not a security sensitive bug.

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

Triage: BB+, C3, P3 - in the target market where SMS is used often, this is severe usability issue

Comment 3

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

We can not do this without bug 782211 which is not a blocking basecamp bug. If you want to block on it you have to push on the platform issue as well. :)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to Vivien Nicolas (:vingtetun) from comment #3)
> We can not do this without bug 782211 which is not a blocking basecamp bug.

Is that really true? There's absolutely nothing we can do to mitigate this problem without implementing an entirely new notification system?

Comment 5

[Julien Wajsberg [:julienw]]

gavin> unfortunately the System app where notifications are doesn't know anything about what happens in the Sms app. And the Sms app can't do much on the notification ([1]).

[1] https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/notification/nsIDOMDesktopNotification.idl#21

Comment 6

[ayman maat :maat]

This is a critical UX bug. The implication is:

1) The system fails to clear notification of new messages when the message has been read
2) Therefore system misinforms the user that they have new messages when they do not
3) Therefore system ceases to inform end user that they have new messages as it has failed to clear notification of old read messages

I would say that the inability of the notification system to be informed of related user activity at the app level impacts upon the integrity and therefore viability of the product. For a viable launch we either need to either: 

a) enable the communication between notification and apps or 
b) scale back the notification system

to provide a product that completes in the market i would absolutely advise path a)

RFI to josh and Daniel because this situation is critical and requires escalation

Comment 7

[Julien Wajsberg [:julienw]]

As Vivien said in comment 3, you have to block on bug 782211 too if you want [a].

Comment 8

[Josh Carpenter [:jcarpenter]]

It doesn't sound like this is going to be resolved for v1, but this is flagged tracking-b2g18+ for high post Jan 15 prioritization. Agree 100% with Ayman that this is a must-fix. The current implementation fails to meet the basic requirements of a notification system.

Comment 10

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> (In reply to Vivien Nicolas (:vingtetun) from comment #3)
> > We can not do this without bug 782211 which is not a blocking basecamp bug.
> 
> Is that really true? There's absolutely nothing we can do to mitigate this
> problem without implementing an entirely new notification system?

Not 100%. If we have the manifestURL of the application then we can close the notifications when the application is launched. I'm unsure we have the necessary informations in Gaia though - I will check tomorrow...

Comment 11

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

Let's renominate it so it will be on the radar of triagers again (fwiw I agree that this having something here is important, even a basic fix that clear notifications when the app is opened).

Comment 12

[Julien Wajsberg [:julienw]]

actually we already clear the notification when the user clicks on it. I really think we can't do much more without platform work.

Comment 13

[Julien Wajsberg [:julienw]]

talked with Vivien and here is what we can try to do :
When an application is displayed, we clear any notification it may own. This would solve most cases for v1.

Comment 14

[Daniel Coloma:dcoloma]

I think(In reply to Julien Wajsberg [:julienw] from comment #13)
> talked with Vivien and here is what we can try to do :
> When an application is displayed, we clear any notification it may own. This
> would solve most cases for v1.

For me that is good enough, Ayman, wdyt?

Comment 15

[Josh Carpenter [:jcarpenter]]

Ayman, I'll come find you today to discuss, and perhaps we can partially resolve this for v1 with some sort of rudimentary patch.

Comment 16

[ayman maat :maat]

(In reply to Daniel Coloma:dcoloma from comment #14)
> I think(In reply to Julien Wajsberg [:julienw] from comment #13)
> > talked with Vivien and here is what we can try to do :
> > When an application is displayed, we clear any notification it may own. This
> > would solve most cases for v1.
> 
> For me that is good enough, Ayman, wdyt?

For me that is perfect for V1 Daniel.

Comment 17

[Julien Wajsberg [:julienw]]

renominating then.

For the drivers: we found a way to make it work for most cases for v1 without expensive platform work, so let's do it.

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

Triage: TEF-, tracking-b2g18+, won't block release but would take a patch

Comment 19

[Beatriz Rodríguez [:brg]]

renominating for TEF +
This is important for certification. Thanks.

Comment 20

[David Scravaglieri [:scravag]]

Isabel, what is the certification requirement ? We would like to be certain if it's important or necessary for certification.

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Sorry, this was the only Bugzilla page that was open during Bugzilla problems so I worked on it.

We add a manifestURL in the notification node dataset so that we can query on it
and remove them at appopen.

We also send manifestURL in appopen now.

For that we need a platform patch (Bug 831393).
---
 apps/system/js/notifications.js  |   13 +++++++++++++
 apps/system/js/window_manager.js |    7 ++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Comment 22

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

Comment on attachment 702913 [details] [diff] [review]
patch v1

Very safe change for a big win. Let's land that and turn 1.0 into a great product. r+a=me.

Comment 23

[Beatriz Rodríguez [:brg]]

(In reply to ayman maat :maat from comment #16)
> (In reply to Daniel Coloma:dcoloma from comment #14)
> > I think(In reply to Julien Wajsberg [:julienw] from comment #13)
> > > talked with Vivien and here is what we can try to do :
> > > When an application is displayed, we clear any notification it may own. This
> > > would solve most cases for v1.
> > 
> > For me that is good enough, Ayman, wdyt?
> 
> For me that is perfect for V1 Daniel.

This is Beatriz, not Isabel :-). The need is the behaviour accepted in this comment for V1.

Comment 24

[Julien Wajsberg [:julienw]]

https://github.com/mozilla-b2g/gaia/commit/7cec8abf54f661238ba4d4765e53f61485f54643

should we wait for Bug 831393 before marking resolved fixed ?

Comment 25

[Dietrich Ayala (:dietrich)]

Really want, would take on patch approval, but not holding the release for it - minus.

Comment 26

[David Scravaglieri [:scravag]]

We will take an approval.

Comment 27

[Julien Wajsberg [:julienw]]

Hey guys, we already had an approval and it already landed.

We need an approval on Bug 831393 now.

Comment 28

[Julien Wajsberg [:julienw]]

Bug 831393 is fixed.

Comment 29

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #27)
> Hey guys, we already had an approval and it already landed.
> 
> We need an approval on Bug 831393 now.

I don't think it is pretty good for module 'sms'. Imagine this: we close the app sms, and then we receive several messages from other people. Then we open the app 'sms' again. Terrible thing is happen. All of the messages without read, but their notification is disappear! I think it's much better to clear a 'sms' notification after a end user read it.

Comment 30

[Julien Wajsberg [:julienw]]

田旻, agreed, this is what we want to do later when we have the new notification implementation. I think it landed on moz-central already, but we can't use this in v1.

The current behaviour is not perfect but it is close enough to what we want. The user can see in the Sms application that a message is not read.

Please file another bug so that we change this in v2 (as we should !).

Comment 31

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #30)
> 田旻, agreed, this is what we want to do later when we have the new
> notification implementation. I think it landed on moz-central already, but
> we can't use this in v1.
> 
> The current behavior is not perfect but it is close enough to what we want.
> The user can see in the sms application that a message is not read.
> 
> Please file another bug so that we change this in v2 (as we should !).

But if the end user close sms app again, he will forgot there are still a lot of messages still not read. What's more, the event "handleAppopen" will just been triggered when we start the  sms app. Imagine this, we are just in this sms application, and we receive a lot short messages. Then we read all of these now messages. All of the notifications are still display on the toorbar Without trigger the event "handleAppopen" although we have read all of them and they are not new any longger.

Comment 32

[Julien Wajsberg [:julienw]]

We're aware of that, we initially decided it is good enough for v1, but please file a new bug to have a discussion about this, maybe we can decide otherwise now. (note: I doubt it).

Comment 33

[田旻]

I have file a new bug marked 855165 in mozilla bugzilla.