1068564 - [System] We should not close the Utility Tray when an application is launched in background

Status: VERIFIED FIXED

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

Description

[Loli (:lolimartinezcr)]

Platform version: 34.0a2
Build ID: 20140909143328
Git commit: 5d4fadda

Reproducible: 100%

Pre-requisites: CMAS alerts are received.

STRs:
1. Tap utility tray
2. Tap "Clear all"

Actual result: 
Utitlity try is closed.

Expected result:
Utility try *isn't* closed.

Comment 1

[Julien Wajsberg [:julienw]]

Jenny, I think we implemented the specified UX, can you please confirm?

Comment 2

[Jenny Lee]

A CMAS notification can be cleared. So "Clear all" should work as how it is supposed to work whether or not there's CMAS notification in the tray. 

Hi Rob, can you confirm the behavior of notification tray after tapping "clear all"? Thanks :)

Comment 3

[Rob MacDonald [:robmac]]

Thanks, Jenny. After the user taps "Clear all" the utility tray should remain open. Also flagging Guillaume as an FYI.

- Rob

Comment 4

[Julien Wajsberg [:julienw]]

Ah ok I understand the bug now.

Is it specific to CMAS alerts? I think I noticed the issue when dismissing only one notification (maybe it was a CMAS notification though, I don't recall).

Comment 5

[Steve Chung [:steveck]]

Hey Michael, after some investigation, I found the root cause is in notification.js that we dispatch mozContentNotificationEvent with type 'desktop-notification-close' while removing the notification. This notification close system message will launch the target app if the app is close, and utility tray will hide when window got 'launchapp' event. Since CMAS will close itself after notification dispatched, every notification removal will force the app launch again and cause utility tray close. It could happen on any other apps if we close the app before notification removed.

I'm not quite familiar with notification part, but it seems not much apps will need notification close event. Maybe we will need a mechanism like whitelist that only dispatch close event to app in the list.

Comment 6

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

IMO we should prevent the killed app to be opened again by the notification onclose event.

I am not sure how this should be fixed, here is one proposal:
* Make system app be able to know the relationship of notification and app (which app is firing this notification)
* System app shall know which app is running or killed, and then prevent to send the notification onclose mozContent event to gecko.

Comment 7

[Julien Wajsberg [:julienw]]

Actually, I don't know if we should send a system message for the "close" event. I think no app is using this at all, and instead is filtering using

    if (!message.clicked) {
      return;
    }

(we should check if every app does this of course)

It was added in the past for some reason but after so many months still nobody uses this, so maybe we can just remove it.

Comment 8

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

(In reply to Julien Wajsberg [:julienw] from comment #7)
> Actually, I don't know if we should send a system message for the "close"
> event. I think no app is using this at all, and instead is filtering using
> 
>     if (!message.clicked) {
>       return;
>     }
> 
> (we should check if every app does this of course)
> 
> It was added in the past for some reason but after so many months still
> nobody uses this, so maybe we can just remove it.

+1 for not telling app the notification is removed because I cannot figure out the use case (redispatch the notification again?), but we need to check why do we invent this before.

Comment 9

[Julien Wajsberg [:julienw]]

The code that added the "clicked" property is bug 836737. My first idea was to do what I suggested in comment 7, but Vivien thought it would be nice that the app knows when its notification is closed (comment 5, too bad I didn't add more information...).

Vivien, what do you think now?

Also, I think the behavior changed at one point (maybe the system app sent a new event to Gecko?) before bug 836737 but I don't know which bug did it.

Comment 10

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

I agree, I don't think we need to fire the system message for the close event. Julian already fixed this for the 'show' event [1], so doing so for 'close' wouldn't be too hard.

From a broader API perspective, it is a sorry state that listening for notification events is so drastically different between when an app is running and when it is closed. The more fixes we do like this, the more these diverge too. We should talk about this in the mailing list, but I think there are two possible ways forward:

- Allow registering for specific notification system messages in the manifest (ie. show, close, click), rather than the current all or nothing approach.

- Use a service worker as a proxy for all notification event registering. This was actually suggested by Jonas, and has the added advantage of applying to desktop tabs (ie. closed tabs) as well as b2g.


1.) http://hg.mozilla.org/mozilla-central/diff/8d52881891a5/b2g/components/AlertsService.js

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to Michael Henretty [:mhenretty] from comment #10)
> I agree, I don't think we need to fire the system message for the close
> event. Julian already fixed this for the 'show' event [1], so doing so for
> 'close' wouldn't be too hard.

Will wait for Vivien's answer, but I think we should do this now and uplift to v2.1 because CMAS is exposing this bug by closing itself. :(

> 
> From a broader API perspective, it is a sorry state that listening for
> notification events is so drastically different between when an app is
> running and when it is closed. The more fixes we do like this, the more
> these diverge too. We should talk about this in the mailing list, but I
> think there are two possible ways forward:
> 
> - Allow registering for specific notification system messages in the
> manifest (ie. show, close, click), rather than the current all or nothing
> approach.
> 
> - Use a service worker as a proxy for all notification event registering.
> This was actually suggested by Jonas, and has the added advantage of
> applying to desktop tabs (ie. closed tabs) as well as b2g.
> 

Agree with everything :)
Maybe the first step is to file a bug?

Comment 12

[Guillaume Marty [:gmarty]]

Clearing the ni?

Comment 13

[Robert Bindar]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> Actually, I don't know if we should send a system message for the "close"
> event. I think no app is using this at all, and instead is filtering using
> 
>     if (!message.clicked) {
>       return;
>     }
> 
> (we should check if every app does this of course)
> 
> It was added in the past for some reason but after so many months still
> nobody uses this, so maybe we can just remove it.

This is pretty much the only place where an app doesn't do that http://mxr.mozilla.org/gaia/source/apps/network-alerts/js/notification/notification.js#17
I didn't do any thorough investigation, so I'm not sure if that app *really* uses the "close" event on mozSetMessageHandler.

I think allowing apps to register for particular system messages sounds better than not sending the "close" event at all. Maybe we could link this to bug 912645 and use that to pass something like "wakeonclose: false".

Comment 14

[Julien Wajsberg [:julienw]]

I discussed about this with Vivien. Now I think we should keep the behavior of sending a system message when the user closes the notification. The reason is that we can have this event on the Notification object, so it makes sense to replicate it using the system messages mechanism (even if what Michael said in comment 10 will be better).

While discussing, it appeared we have a real issue in the System app: why do we close the Utility Tray if an application is launched in _background_ ? We shouldn't !

And actually, here is another STR that has nothing to do with notifications:

0. Prerequisite: have a working SIM in the device.
1. Close all applications that are in background
2. Open the Utility Tray
3. Sends a SMS to the device from another device
=> When we receive the SMS on the device, the Utility Tray is closed (because the SMS app is launched in backgroung when the system message "sms-received" is sent).

This proves well that we need to fix this at the system level: we should not close the Utility Tray if the application is launched in background.


[Blocking Requested - why for this release]: This issue likely existed before but is now exposed by the new Network Alerts application.

Comment 15

[Julien Wajsberg [:julienw]]

QA: can you please test the STR in comment 14 on branches 1.4 and 2.0?

Comment 16

[Julien Wajsberg [:julienw]]

(note to QA: I think we don't need a regression window it we find it's a regression)

Comment 17

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

This issue occurs on Flame KK 2.2, Flame KK 2.1, Flame KK 2.0, Flame 1.4, and the Flame v180 KK base.

Environmental Variables:
Device: Flame 2.2
BuildID: 20140923233152
Gaia: ff6dbb006e4e60edba697639aa2a2a199b07069b
Gecko: 1e2993c99323
Version: 35.0a1 (2.2) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20140924061258
Gaia: 020e6283a033e8fbcf65e7ed81c5b75ba0095f22
Gecko: d6b762814638
Version: 34.0a2 (2.1) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
Device: Flame 2.0
BuildID: 20140923195101
Gaia: 263e3b201dca967ec5346e35901aa981ca47dce7
Gecko: 35d791e16d31
Version: 32.0 (2.0) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

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

Comment 18

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

As a side not to this bug, the behavior of web notifications in regards to event listening (and service workers) is being actively discussed right now.

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-August/297368.html

Comment 19

[Gregor Wagner [:gwagner]]

Not a regression. Lets fix on master and uplift if needed.

Comment 20

[Julien Wajsberg [:julienw]]

[Blocking Requested - why for this release]: Renominating. This is not a regression because the bug was always there, but this is exposed by a new 2.1 feature (network-alerts). We can't keep the window opened because it's an attention window.

Comment 21

[Julien Wajsberg [:julienw]]

(retitling the bug to show the real issue)

Comment 22

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> [Blocking Requested - why for this release]: Renominating. This is not a
> regression because the bug was always there, but this is exposed by a new
> 2.1 feature (network-alerts). We can't keep the window opened because it's
> an attention window.

The user impact is that we close the utility tray when we receive a network alert?
During our team triage yesterday we decided to fix on master and decide based on the patch if we want to uplift but we won't hold the release for this.

Comment 23

[Julien Wajsberg [:julienw]]

The user impact is that we close the utility tray when we swipe/hide the network alert notification.

This will also happen when hiding any notification for an application that's closed (for example: a SMS notification after a reboot). As a user I find this very frustrating.

If it's not a blocker how can we ensure it will get fixed in a timely manner?

Comment 25

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

Attachment: [Gaia PR] keep utility tray open for background launches

Comment 26

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

Comment on attachment 8540868 [details]
[Gaia PR] keep utility tray open for background launches

Is this a PR?

'launchapp' event handler in utility tray has already checked if the config.stayBackground=true; I don't know what makes it not working but worthy to track.

Comment 27

[Julien Wajsberg [:julienw]]

Another instance of the issue:
* have Messages app forcibly closed
* open utility tray
* send a SMS to this phone
=> because the Message s

Comment 28

[Julien Wajsberg [:julienw]]

Another instance of the issue:
* have Messages app forcibly closed
* open utility tray
* send a SMS to this phone
=> because the Message app is launched in background, the utility tray is closed.

I think this can happen with any app that is launched because of system messages.

Comment 29

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

Attachment: [Gaia PR] keep utility tray open for background launches

Ah, botched copy/paste job. Sorry. Here is the real PR.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #26)
> 'launchapp' event handler in utility tray has already checked if the
> config.stayBackground=true; I don't know what makes it not working but
> worthy to track.

Are you sure? It looks like it was just checking to see if the app being launched was FindMyDevice (which I guess is generally launched in the background).

Comment 30

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

Comment on attachment 8541772 [details]
[Gaia PR] keep utility tray open for background launches

I am sure stayBackground equals to !showApp because
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window_factory.js#L132

So what you need is stayBackground here.

However, I wonder if there is an app which is launched as system message once, but also tries to launch itself will fail here. Because config.stayBackground is set to true when it is launched via 'openapp' request from gecko, but never being flipped after that even it is either launched by the user from task manager to launches itself via mozApps.getSelf().launch()

We need to
1. Use stayBackground (and check if this works for FMD...I don't really know how we launch it)
2. flip the stayBackground to false once the app window is opened.

What do you think?

Comment 31

[Chris Lord [:cwiiis]]

We didn't think this would block 2.1, but maybe 2.2?

Rationale: This is a weird bug that you encounter in a piece of UI that you interact with frequently, and that is always visible. To the user, it's just random behaviour and quite frustrating. This bug manifests frequently on phone boot, and on phone boot is exactly the sort of time you might check your notifications.

It would be great if we could fix all/most of the really obvious, top-level polish bugs in the UI.

Comment 32

[GitHub Autolander Bot]

Attachment: [gaia] mikehenrty:bug-1068564-no-close-utility-tray > mozilla-b2g:master

Comment 33

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

Comment on attachment 8575559 [details] [review]
[gaia] mikehenrty:bug-1068564-no-close-utility-tray > mozilla-b2g:master

Sorry for the long delay (3 months!) I have some reponses below.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #30)
> Comment on attachment 8541772 [details]
> [Gaia PR] keep utility tray open for background launches
> 
> I am sure stayBackground equals to !showApp because
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> app_window_factory.js#L132
> 
> So what you need is stayBackground here.

Ok, updated.


> However, I wonder if there is an app which is launched as system message
> once, but also tries to launch itself will fail here. Because
> config.stayBackground is set to true when it is launched via 'openapp'
> request from gecko, but never being flipped after that even it is either
> launched by the user from task manager to launches itself via
> mozApps.getSelf().launch()

This is ok, because Utility Tray will receive two launchapp events from the system app. One when an app, say sms, receieves the system message like sms-received. In this case, the event will be marked as stayBackground and so the utility tray will not be hidden. Then, when the app calls mozApps.getSelf().launch(), a new event will be triggered called webapps-launch which will in turn trigger the launchapp event in the utility tray, and this time stayBackgraound will be false. So when an app launches itself this way, the Utility Tray will indeed be closed.

> 
> We need to
> 1. Use stayBackground (and check if this works for FMD...I don't really know
> how we launch it)

stayBackground should not effect FMD since FMD is blacklisted and should never hide the utility tray. Since we are only added times where we don't close the utility tray, FMD will be unaffected here.

> 2. flip the stayBackground to false once the app window is opened.]

As stated above, I don't think we need to worry about this since there is a separate event for webapps-launch->launchapp where stayBackaground is false. What do you think?

Comment 34

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

Comment on attachment 8575559 [details] [review]
[gaia] mikehenrty:bug-1068564-no-close-utility-tray > mozilla-b2g:master

\o/

Comment 35

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

Comment on attachment 8575559 [details] [review]
[gaia] mikehenrty:bug-1068564-no-close-utility-tray > mozilla-b2g:master

Added a test for this case.

Comment 36

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

master: https://github.com/mozilla-b2g/gaia/commit/d7c9eaf578d33415b85d5c8cda7f000bba2b033b

Comment 37

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

Comment on attachment 8575559 [details] [review]
[gaia] mikehenrty:bug-1068564-no-close-utility-tray > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Not a regression, but a long standing UX issue.

[User impact] if declined:
Sometimes the Utility Tray will close without user action, poor UX.

[Testing completed]:
Manually tested, and additional unit test.

[Risk to taking this patch] (and alternatives if risky):
This patch makes it less frequent when we automatically close the Utility Tray. Low risk to causing other regressions.

[String changes made]: none.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/510abbe4c569434eb4a3bfea0ad612fb0ece5dab

Comment 39

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

This issue still reproduces on the latest Nightly Flame 3.0 and 2.2 builds.

Actual Results: The Utility Tray remains open when apps are opened in the background.

Environmental Variables:
Device: Flame 3.0 KK (319MB) (Full Flash)
BuildID: 20150313010238
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 42afc7ef5ccb
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2 KK (319MB) (Full Flash)
BuildID: 20150313002507
Gaia: 4aefc3f6f30a40ac67fdf841b7c90cd648b85369
Gecko: 049713f3b0ed
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 40

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

Sorry correction of comment 39:
This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds.  It does not still occur.

Comment 41

[Julien Wajsberg [:julienw]]

Thanks a lot ! I can't wait switching to v2.2 as my dogfooding phone ;)