1105548 - [Messages] Notifications for messages will still be present when a message is received while in the message thread opened from contacts

Status: RESOLVED WONTFIX

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

Description

[Adam Alldredge [:AdamA][Inactive]]

Attachment: logcat

Description:
if the user goes to a contact and sends an sms from the link in the contact page they will be in the thread view for that conversation. If a text message is received on this thread view the user will still have a new message notification in the utility tray and on the lock screen if the phone is restarted.

Prerequisite:
Have a contact with a valid phone number

Repro Steps:
1) Update a Flame to 20141126040207
2) Go to Contacts
3) Enter a contact info page and choose to send a text
4) Send a text to the contact
5) Stay in the message thread
6) Receive a message from that message thread
7) Pull down utility tray
8) Observe new message notification in utility tray

Actual:
there is a notification present for a message that the user has read

Expected:
It is expected that there is no notification present for messages that have been seen

Environmental Variables:
Device: Flame 2.2 (319mb)(Full Flash)
Build ID: 20141126040207
Gaia: 41b7be7c67167f367c3c4982ff08651d55455373
Gecko: 7bcc6573d204
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Repro frequency: 5/5
See attached: video clip(http://youtu.be/OaWe-klzPG8), logcat
---------------------------------------------------------------------
This issue also occurs on 2.1 Flame, and the previous 2.2 Flame.

Environmental Variables:
Device: Flame 2.1 (319MB)(Full Flash)
BuildID: 20141126001202 (Full Flash)
Gaia: db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko: 211eae88f119
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 (319mb)(Full Flash)
Build ID: 20141125040209
Gaia: 824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko: acde07cb4e4d
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Result:
there is a notification present for a message that the user has read

Comment 1

[Peter Bylenga [:PBylenga]]

QAWanted for branch checks.

Comment 2

[Duane Dixon [:ddixon]]

Branch Check

Issue DOES occur in Flame 2.2, 2.1, 2.0  (shallow flash, eng. build, 319 MB memory). 

Actual Results: User receives a text notification despite already having seen the text message. 

Device: Flame 2.2
BuildID: 20141201032744
Gaia: 39214fb22c203e8849aaa1c27b773eeb73212921
Gecko: 08be3008650f
Version: 37.0a1 (2.2)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
----------------------------------------------
Device: Flame 2.1
BuildID: 20141128073618
Gaia: ccb49abe412c978a4045f0c75abff534372716c4
Gecko: 18fb67530b22
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
----------------------------------------------
Device: Flame 2.0
BuildID: 20141126072826
Gaia: 8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko: c756bd8bf3c3
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 3

[Julien Wajsberg [:julienw]]

Funny that I experienced this yesterday too :)

I'd like to know if this happens also when there is no Messages app in background (if it is please forcibly close it using task manager) before you receive the message.

Comment 4

[Adam Alldredge [:AdamA][Inactive]]

This issue still occurs without the message being open in the background prior to opening contacts.

Comment 5

[Julien Wajsberg [:julienw]]

QA, can we know the result on v1.3 and v1.4 too?

Comment 6

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

As 1.3 and 1.4 aren't available as Flame KK I checked this on JB.  I realize that makes the testing mostly invalid but hopefully it will help.  The issue DOES reproduce on the 1.3 v123 JB base and with 1.4 Flashed on top of it.

Environmental Variables:
Device: Flame 1.4
BuildID: 20141002180522
Gaia: b06fee13c1d80bd2a463be1f2fb1d59169af9298
Gecko: ea144d59c01e
Version: 30.0 (1.4) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Comment 7

[Joshua Mitchell (Inactive)]

Flipping tracking flag to 'affected' for 1.4 (there isn't even a 1.3 flag to flip).

Comment 8

[Julien Wajsberg [:julienw]]

Thanks a lot Jayme, the code should not have a different behavior with different base build, so the testing is not invalid. It helps showing that we always had this issue :)

Comment 9

[Oleg Zasypkin [:azasypkin]]

Hey Alive,

While investigating possible solutions for this bug I tried to subscribe for "sms-received" system message when app is run as inline activity (navigator.mozSetMessageHandler('sms-received', handler)), but seems that this handler is never called for inline activity. 'sms-received' system message is either handled by existing "main" application instance or if "main" instance isn't run - system runs it to handle system message.

When Messages app is run as inline-activity, entry point is "/index.html#activity-xxx" if it's useful to know.

So do you know if it's by design or some sort of bug? At first I thought it maybe related to bug 931339, but not sure.

Thanks!

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #9)
> Hey Alive,
> 
> While investigating possible solutions for this bug I tried to subscribe for
> "sms-received" system message when app is run as inline activity

This sounds a gecko bug, ni Fabrice - inline activity cannot get system message.

> (navigator.mozSetMessageHandler('sms-received', handler)), but seems that
> this handler is never called for inline activity. 'sms-received' system
> message is either handled by existing "main" application instance or if
> "main" instance isn't run - system runs it to handle system message.
> 
> When Messages app is run as inline-activity, entry point is
> "/index.html#activity-xxx" if it's useful to know.

Okay this might be the problem - gecko does not identify the hash here. It only see index.html and does not invoke your system message handler in index.html#xxxxx

> 
> So do you know if it's by design or some sort of bug? At first I thought it
> maybe related to bug 931339, but not sure.
> 
> Thanks!

I tend to think this is a system message design issue. And I am afraid we have no way to workaround it in gaia side.

Comment 11

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (experimental)

Experimental WIP that uses InterInstanceEventDispatcher (SharedWorker) to query information (visible or not, current panel) about other active Messages app instances.

Comment 13

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8544424 [details] [review]
GitHub pull request URL (experimental)

Hey guys,

It's kind of experimental and a bit overcomplicated patch without much optimizations. But I'd be happy to get your early (but not urgent at all) feedback about the idea in general.

This patch relies on the fact that only "main" instance can get "sms-received" system message.

Known issue: when message is received while in Thread opened from Contacts, user will hear only sound signal, for some reason vibration doesn't work - will dig into it if we stick with this idea. 

Thanks!

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #13)
> 
> Known issue: when message is received while in Thread opened from Contacts,
> user will hear only sound signal, for some reason vibration doesn't work -
> will dig into it if we stick with this idea. 

You need to look whether the sound is sent by the Messages app or by the System app.
If it's the Messages app, maybe we don't vibrate if the app's window is not visible right now.

Comment 15

[Steve Chung [:steveck]]

In notify.js the vibration will still check the visibility of message app and only vibrate when it's in foreground. Maybe you can reuse the InterInstanceEventDispatcher in Notify.vibrate or simply add "force" parameter for vibrate.

Comment 16

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #15)
> In notify.js the vibration will still check the visibility of message app
> and only vibrate when it's in foreground. 

Right! It seems when app is hidden vibration is handled by system(?) when notification is created.

> Maybe you can reuse the InterInstanceEventDispatcher in Notify.vibrate or simply add "force"
> parameter for vibrate.

Yep, sounds like a plan :) Will try.

Comment 17

[Oleg Zasypkin [:azasypkin]]

Okay, hidden document can't call vibrate per spec [1] - "If the hidden attribute [PAGE-VISIBILITY] is set to true, then return false and terminate these steps.". 

But there is note about privileged apps - "A trusted (also known as privileged) application that integrates closely with the operating system's functionality may vibrate the device even if such an application is not visible at all, and thus may ignore the previous step. "

Looking at implementation at [2], looks like we don't respect this note.

I'll think how to overcome this.

[1] http://www.w3.org/TR/vibration/
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#795

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #17)
> Okay, hidden document can't call vibrate per spec [1] - "If the hidden
> attribute [PAGE-VISIBILITY] is set to true, then return false and terminate
> these steps.". 
> 
> But there is note about privileged apps - "A trusted (also known as
> privileged) application that integrates closely with the operating system's
> functionality may vibrate the device even if such an application is not
> visible at all, and thus may ignore the previous step. "
> 
> Looking at implementation at [2], looks like we don't respect this note.
> 

Is it worth filing a separate bug and asking the DOM API peers?

Comment 19

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #18)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #17)
> > Okay, hidden document can't call vibrate per spec [1] - "If the hidden
> > attribute [PAGE-VISIBILITY] is set to true, then return false and terminate
> > these steps.". 
> > 
> > But there is note about privileged apps - "A trusted (also known as
> > privileged) application that integrates closely with the operating system's
> > functionality may vibrate the device even if such an application is not
> > visible at all, and thus may ignore the previous step. "
> > 
> > Looking at implementation at [2], looks like we don't respect this note.
> > 
> 
> Is it worth filing a separate bug and asking the DOM API peers?

Yep, let's try, filed a bug 1121376

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> 
> I tend to think this is a system message design issue. And I am afraid we
> have no way to workaround it in gaia side.

The relevant part of the sms manifest is:
"activities": {
    "new": {
      "href": "/index.html#activity-new",
      "filters": {
        "type": "websms/sms",
        "number": {
          "pattern":"[\\w\\s+#*().-]{0,50}"
        }
       },
      "disposition": "inline",
      "returnValue": true
    },
    "share": {
      "href": "/index.html#activity-share",
      "filters": {
        "type": ["image/*", "audio/*", "video/*", "url"],
        "number": {
          "max": 5
        }
       },
      "disposition": "inline",
      "returnValue": true
    }
  },
  "messages": [
     { "alarm": "/index.html" },
     { "sms-received": "/index.html" },
     { "notification": "/index.html#notification" }
  ],

So the system message is routed properly to /index.html only. Note that the notification one would be routed to /index.html#notification. In your case it seems that you would like it to be routed to a set of possible targets?
I guess we could do something like:
{ "sms-received": ["/index.html", "/index.html#activity-new", "/index.html#activity-share"] }
we would then try to send the message to the first already opened target, and fallback to the first entry if none of the urls are already opened.

Comment 21

[Oleg Zasypkin [:azasypkin]]

(In reply to Fabrice Desré [:fabrice] from comment #20)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> > 
> > I tend to think this is a system message design issue. And I am afraid we
> > have no way to workaround it in gaia side.
> 
> The relevant part of the sms manifest is:
> "activities": {
>     "new": {
>       "href": "/index.html#activity-new",
>       "filters": {
>         "type": "websms/sms",
>         "number": {
>           "pattern":"[\\w\\s+#*().-]{0,50}"
>         }
>        },
>       "disposition": "inline",
>       "returnValue": true
>     },
>     "share": {
>       "href": "/index.html#activity-share",
>       "filters": {
>         "type": ["image/*", "audio/*", "video/*", "url"],
>         "number": {
>           "max": 5
>         }
>        },
>       "disposition": "inline",
>       "returnValue": true
>     }
>   },
>   "messages": [
>      { "alarm": "/index.html" },
>      { "sms-received": "/index.html" },
>      { "notification": "/index.html#notification" }
>   ],
> 
> So the system message is routed properly to /index.html only. Note that the
> notification one would be routed to /index.html#notification. 

So does that mean that when start changing app URL hashes for different panels to support app recovering from the last known URL, we won't receive system messages registered for hash-less URL? Like if we have app://sms.gaiamobile.org/index.html#composer and "sms-received" is registered for "/index.html"? That would be quite ugly to enumerate all possible app sub-links in manifest.

> In your case it seems that you would like it to be routed to a set of possible targets?
> I guess we could do something like:
> { "sms-received": ["/index.html", "/index.html#activity-new",
> "/index.html#activity-share"] }
> we would then try to send the message to the first already opened target,
> and fallback to the first entry if none of the urls are already opened.

That doesn't seem to work (system message is just ignored entirely). Was it just a guess or it should work and I see a bug? And what's the rationale behind handling system message by first "target" only and not by every or at least first-visible (eg. we'd like to call navigator.vibrate on sms-received, but it doesn't work for app that is hidden)? Of course we can workaround that with SharedWorker\BroadcastChannel (once it's landed), but IMO it's not the best thing to do.

Comment 22

[Julien Wajsberg [:julienw]]

Related issues:
* bug 818000 (see esp comment 33)
* bug 931339

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Oleg Zasypkin [:azasypkin] from comment #21)

> > In your case it seems that you would like it to be routed to a set of possible targets?
> > I guess we could do something like:
> > { "sms-received": ["/index.html", "/index.html#activity-new",
> > "/index.html#activity-share"] }
> > we would then try to send the message to the first already opened target,
> > and fallback to the first entry if none of the urls are already opened.
> 
> That doesn't seem to work (system message is just ignored entirely). Was it
> just a guess or it should work and I see a bug? And what's the rationale
> behind handling system message by first "target" only and not by every or at
> least first-visible (eg. we'd like to call navigator.vibrate on
> sms-received, but it doesn't work for app that is hidden)? Of course we can
> workaround that with SharedWorker\BroadcastChannel (once it's landed), but
> IMO it's not the best thing to do.

That was a proposal to modify our current implementation. Of course that doesn't work today at all.
If we need more changes to the way we dispatch system messages, we need to discuss that on dev-webapi, not in a bug.

Comment 24

[Oleg Zasypkin [:azasypkin]]

Okay, thanks Fabrice!

I'd say we can try first to workaround that in Messages app to have more knowledge about real use cases before we make any good proposal to dev-webapi.

Comment 25

[Oleg Zasypkin [:azasypkin]]

Finally updated experimental PR so that "vibration" issue is fixed too :)

Comment 26

[Josh Cheng [:josh]]

Still need to fix issue in earliest possible. nominate to 2.2?

Comment 27

[Julien Wajsberg [:julienw]]

Assigning Oleg as he worked on some patch.

Still I'd rather take time to think of a proper solution instead of rushing a workaround. We lived with this for a long time and I don't think this really should be a blocker.

Comment 28

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: non-blocker, but please ask for approval to uplift.

Comment 29

[Oleg Zasypkin [:azasypkin]]

Not a blocker anymore, so removing this bug from my priority-list for now. We can get back to it later if needed.

Comment 30

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 31

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(