966405 - [B2G][Wappush] No new message is arrived after closing SI/SL message by pressing the "Home" button

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Wappush, defect)

Description

[sarsenyev]

Attachment: log13114.txt

Description:
In Bug 934354 per comment 25, the expected behavior if user presses home key, the notification will be displayed on status bar again with audio indication which is like a new message is arrived, yet it says "Configuration message not processed..."., but no new message is arrived

Prerequisite: Using NowSMS(http://www.nowsms.com/) to send WAP Push messages to device.

Repro Steps:
1) Updated Buri to BuildID: 20140127004002
2) Enable WAP Push in Settings > Message setting on DUT.
3) Send a WAP Push message from NowSMS with text, WAP URL and SI push type values filled and selected on the software.
4) Scroll down notification bar and tap the received message.
5) Press home key.

Actual:
SI/SL message disappears and no new message says "Configuration message not processed..." is received

Expected:
After pressing the "Home" button, the previous message disappears but the new message is received says "Configuration message not processed..." 

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140127004002
Gaia: 25a45a836a4a21a30f63fa7b544b42e8b781180a
Gecko: c40099a42c1f
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Notes:
Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/cases/?filter-id=9723
Attachment: logcat

Comment 1

[Jason Smith [:jsmith]]

Does this reproduce on 1.2?

Comment 2

[sarsenyev]

It does reproduce on 1.2 as per Bug 934354, no any second notification is coming after open the SL/SI message from the device without canceling .
 The fix is landed on 1.3, it has status verified, but I still saw the issue on the previous test run, will try again on the current test run

Device: Buri 1.2 MOZ
BuildID: 20140210004002
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 2673f348598c
Version: 26.0
Firmware Version: v1.2-device.cfg

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

(In reply to sarsenyev from comment #2)
> It does reproduce on 1.2 as per Bug 934354, no any second notification is
> coming after open the SL/SI message from the device without canceling .
>  The fix is landed on 1.3, it has status verified, but I still saw the issue
> on the previous test run, will try again on the current test run

The fix landed in bug 934354 is only for CP messages. You won't see the behavior implemented in that bug for SI/SL messages.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

From what I understand this is not a bug. SI/SL messages do not carry any configuration information and they're always discarded when pressing the home key, that's expected behavior. Or am I missing something?

Comment 5

[sarsenyev]

Steps in TC9723 says:
1. Scroll down notification bar and tap the received message.
2. Press home key.
3. Message will be hidden in notification tray and can be opened again until user closes it.
This bug was created based on the actual result, so either TC case is invalid or it's a valid bug?

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to sarsenyev from comment #5)
> This bug was created based on the actual result, so either TC case is
> invalid or it's a valid bug?

Actually I've re-read the original UX design in attachment 784845 [details] and it seems that the TC is right. When tapping the home button the message should remain in the cards view so that the user can pick it up again until it closes it explicitly.

I can't believe we hadn't noticed that small text before, it seems that we've spent *significant* amounts of time working around an issue (close when the home button is pressed) that should never have been there in the first place. Sigh :-(

José, I'll be going ahead and will fix this. Once this is done this is likely to also solve our issue in bug 961064, right? I kind of feel sorry about this because it would have saved you some time when working on bug 934354.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)

> José, I'll be going ahead and will fix this.

Oh, perfect. Thanks!

> Once this is done this is likely to also solve our issue in bug 961064, right?

Correct, I guess you are going to use the new Notification API as you wanted for bug 934354.

> I kind of feel sorry
> about this because it would have saved you some time when working on bug
> 934354.

No worries, thanks again for your help in that bug anyway!

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Do not close the application when hidden if it's displaying a message

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Do not close the application when hidden if it's displaying a message

Updated patch; I've fixed a bug in the notification code which we never hit because we were closing the application too fast for it to be relevant.

I've modified the unit-tests to account for the new behavior and extensively tested this in the emulator with actual messages.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Do not close the application when hidden if it's displaying a message

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to sarsenyev from comment #5)
> Steps in TC9723 says:
> 1. Scroll down notification bar and tap the received message.
> 2. Press home key.
> 3. Message will be hidden in notification tray and can be opened again until
> user closes it.

But the app can be killed because of memory pressure. How do we handle this if the message is removed from the notification tray?

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> (In reply to sarsenyev from comment #5)
> But the app can be killed because of memory pressure. How do we handle this
> if the message is removed from the notification tray?

The only way to solve that is to remove the notification only if the user closes the application explicitly (tapping the 'x' button, not from the cards view) but that introduces more problems than it solves so I'd rather not go there.

The only class of messages we'll be doing that is CP messages but that's because the user has to interact with them so it makes sense to keep the notification until the interaction is finished. For SI/SL messages I think it's better to stick to the current behavior were the notification is removed as soon as the user taps it.

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8379846 [details] [diff] [review]
[PATCH v2] Do not close the application when hidden if it's displaying a message

In case this recalls you about something, you talked about closing the app when the user presses "home" in Bug 911247 comment 3. I won't comment more your choice here, you know better than me.

I'm reviewing it now.

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8379846 [details] [diff] [review]
[PATCH v2] Do not close the application when hidden if it's displaying a message

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

Some comments and questions, removing my r? request for now.

::: apps/wappush/js/wappush.js
@@ +101,5 @@
>    }
>  
>    /**
> +   * Prevents the application from being automatically closed when it's brought
> +   * into the foreground.

Maybe add some words about the fact that the closure is being scheduled when the user presses the cross?

@@ +186,5 @@
>              body: text,
>              tag: message.timestamp
>            };
>  
> +          var onClick = function wpm_notificationOnClick(event) {

nit: can't we simply use "function onNotificationClick(event) {...}" ?

@@ +193,4 @@
>            };
>  
>            var notification = new Notification(message.sender, options);
>            notification.addEventListener('click', onClick.bind(options.tag));

do we still want to bind ?

@@ +261,5 @@
>                CpScreenHelper.populateScreen(message);
>                break;
>            }
> +
> +          displayedMessage = message;

I don't understand why we need to keep the whole message; wouldn't a boolean be enough?
Also, I don't see where we reset it (except in the init). Don't we need to reset it at one point?

@@ +268,1 @@
>            SiSlScreenHelper.populateScreen(message);

nit: we don't need to pass "message" here, it's falsy anyway

@@ +280,5 @@
>     */
>    function wpm_finish() {
>      pendingMessages--;
>  
> +    if (document.hidden && !displayedMessage) {

Can you please explain this change a little more? I feel stupid today :)

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> In case this recalls you about something, you talked about closing the app
> when the user presses "home" in Bug 911247 comment 3. I won't comment more
> your choice here, you know better than me.

I misunderstood the original requirement which was to not show the app in the cards view when it had received a message but hadn't been made visible yet by tapping on the notification.

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Maybe add some words about the fact that the closure is being scheduled when
> the user presses the cross?

Good point, that'll make it clearer.

> @@ +186,5 @@
> >              body: text,
> >              tag: message.timestamp
> >            };
> >  
> > +          var onClick = function wpm_notificationOnClick(event) {
> 
> nit: can't we simply use "function onNotificationClick(event) {...}" ?

Yes :)

> @@ +193,4 @@
> >            };
> >  
> >            var notification = new Notification(message.sender, options);
> >            notification.addEventListener('click', onClick.bind(options.tag));
> 
> do we still want to bind ?

No, you're right, it's useless.

> @@ +261,5 @@
> >                CpScreenHelper.populateScreen(message);
> >                break;
> >            }
> > +
> > +          displayedMessage = message;
> 
> I don't understand why we need to keep the whole message; wouldn't a boolean
> be enough?
> Also, I don't see where we reset it (except in the init). Don't we need to
> reset it at one point?

We set it whenever a new message is displayed so if the app is alive there's always a message being displayed. The other way out is by closing the app so we don't really need to reset it. Also I kept all the message and not just a boolean because I'm going to need it in the future for fixing bug 942076 and bug 942080.

> @@ +268,1 @@
> >            SiSlScreenHelper.populateScreen(message);
> 
> nit: we don't need to pass "message" here, it's falsy anyway

Good point.

> @@ +280,5 @@
> >     */
> >    function wpm_finish() {
> >      pendingMessages--;
> >  
> > +    if (document.hidden && !displayedMessage) {
> 
> Can you please explain this change a little more? I feel stupid today :)

If the document is hidden and there's no displayed message it means that the app was launched to execute the system message handler because a new message has arrived. In this case we don't want the app to be shown in the cards view so we run the delayed close flow.

On the other hand if the app is visible or if it's hidden but there's a displayed message (which means the app was opened by the user) we don't want to close it so we don't execute wpm_close().

I'll refresh the patch with your suggestions and ask for review again.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Do not close the application when hidden if it's displaying a message

New patch with the changes suggested by Julien in comment 14. I'm not asking for review just yet because a new annoying problem has cropped up: after bug 939809 has landed all applications with the role set to "system" are not shown in the cards view, this includes the WAP Push app. Dropping the system role does fix this particular problem but causes an annoying side-effect: the app icon gets shown on the homescreen instead. Since either way we get an undesirable behavior I'll have to find a fix, possibly whitelisting the WAP Push app unless bug 967405 lands soon enough to fix the issue.

Comment 17

[Julien Wajsberg [:julienw]]

We can also back out bug 939809, although I think that apps with role=system not showing is a good thing. Maybe you need another role :)

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> We can also back out bug 939809, although I think that apps with role=system
> not showing is a good thing. Maybe you need another role :)

Bug 939809 is pretty large and the change there makes sense especially since it doesn't actually break any existing behavior (the wappush app wasn't visible anyway in the cards view before this patch). I'll see what kind of work is needed in bug 967405 and what are our priorities in respect for this bug and if this is not really urgent I'll just block on bug 967405. Worst case I'll have to deploy a quick workaround in the system app.

Comment 19

[Julien Wajsberg [:julienw]]

Ok, I'd say all this can go in another bug though!

Comment 20

[Julien Wajsberg [:julienw]]

Ok then just ask review when you're ready.

Comment 22

[liangxin]

Hi,everyone.Is there any advance?

Comment 23

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to liangxin from comment #22)
> Hi,everyone.Is there any advance?

Not unless the dependent bug 967405 gets fixed or we change how the UX works. In one of the previous comments I mentioned that this could be fixed by leaving the notifications in the tray until the user *explicitly* closes the message. This would differ from the original spec though so we'd need some feedback from UX to go there.

Comment 24

[liangxin]

Hi,everyone.Is there any advance?

Comment 25

[Gabriele Svelto [:gsvelto] (PTO)]

Bug 967405 is being worked on, once that's fixed I can finish my patch here.

Comment 26

[BMO Automation]

Firefox OS is not being worked on