873397 - [Buri][STK]idle mode text should still display after view

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P2)

Description

[sync-1]

SW12A
 AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.105
 Firefox os  v1.0.1
 Mozilla build ID:20130512070209
 
 
 +++ This bug was initially created as a clone of Bug #455177 +++
 
 DEFECT DESCRIPTION:
 [STK]idle mode text should still display after view
 
  REPRODUCING PROCEDURES:
 1.Load a simulated SIM card which can send "setup idle text";
 2.Run test case STM12002-->Short Text/Large Text;
 3.prompt message and icon display on the top bar, user drag down status bar ,can see the message detail information.if user click confirm button, the message and icon disappear.===>KO
 
 
  EXPECTED BEHAVIOUR:
 after "setup idle text" execute succefully, if user click confirm button, the message and icon should not disappear. 
 as spec 11.14 , setup idle mode text, the text shall be removed from the ME's memory and display if either: 
 - the ME is powered off or; 
 - the SIM is removed or electrically reset or; 
 - a REFRESH command occurs with "initialisation" or "reset".
 Any subsequent SET UP IDLE MODE TEXT command replaces the current idle mode text of the previous SET UP IDLE 
 MODE TEXT. The SET UP IDLE MODE TEXT command can also be used to remove an idle mode text from the ME.
 
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #455177 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[buri.blff]

This bug is GCF block

Comment 2

[bhavana bajaj [:bajaj]]

Adding qawanted here if they are any familiar with the test scenario being discussed in the description, as none of us in the triage were able to perform the actions as per the STR

Comment 3

[Tony Chung [:tchung]]

mozilla QA cannot setup a simulated SIM network as we dont have these types of tools to reproduce. If someone can share a real-world scenario to reproduce this, then please add qawanted back with STR. 

removing qawanted in the meantime as this is not actionable by our QA here.

Comment 4

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

cyang/frsela, not sure if you have any idea for this bug? thanks

Comment 5

[Carol Yang [:cyang]]

Hi sync-1,

The current behavior seems to be in line with how Notification bar should behave (once selected, the item is dismissed). Fernando can probably provide better insight on this though.

Also, do you know how Android behaves once the item is selected from Notification bar?

Comment 6

[buri.blff]

About this case:
   MTK android devices, the nodification item will not disappear;
   HTC android devices based on QCOM, the nodification item also will not disappear;

   According to the spec 11.14 , the item should not disappear only replace.
   Only disappear is as  EXPECTED BEHAVIOUR.
   This is different from SMS notificaiotn, we should test it according to the spec 11.14.
Thanks!

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

As Carol explained in comment #5, this depends in the notification helper code. We had some related bugs in the past, this is a DUPLICATE:

846966, 846968, ...

Comment 8

[Carol Yang [:cyang]]

Hey Fernando,

This seems slightly different from 846966 and 846968. Those two are tracking issues of UI not removing/replacing the existing idle mode text. I believe the issue reported by this current bug is the fact that once a user selects the existing idle mode text from the drop down notification bar, the message is removed.

Is there flexibility in the notification helper code that would allow a specific type of messages to remain in the notification bar after viewing?

Comment 9

[buri.blff]

Attachment: The modify for this case

Comment 10

[buri.blff]

we modified the notification.js for this case and it's ok;
we think this modification can't resolve the defect of the structure of notification.
we hope your modification on the structure of the notification, so that the app can control the notification.

Comment 11

[buri.blff]

the modifcation is as between 
 //*** add for pr:455177 according to TS 11.44 6.4.22
 //end

Comment 12

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Carol Yang from comment #8)
> Hey Fernando,
> 
> This seems slightly different from 846966 and 846968. Those two are tracking
> issues of UI not removing/replacing the existing idle mode text. I believe
> the issue reported by this current bug is the fact that once a user selects
> the existing idle mode text from the drop down notification bar, the message
> is removed.
> 
> Is there flexibility in the notification helper code that would allow a
> specific type of messages to remain in the notification bar after viewing?

This is the way of working of the notif. bar... so the 'bug' is no STK, is in notification bar/notification helper:

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/notification_helper.js

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to buri.blff from comment #10)
> we modified the notification.js for this case and it's ok;
> we think this modification can't resolve the defect of the structure of
> notification.
> we hope your modification on the structure of the notification, so that the
> app can control the notification.

Where are these changes?

Comment 14

[buri.blff]

the modifcation code is flag with follow flag:
 
 //*** add for pr:455177 according to TS 11.44 6.4.22
 //end

Comment 15

[buri.blff]

you can find the modification by searching "add for pr:455177 according to TS 11.44 6.4.22"

every modification program is end with "//end"

Comment 16

[buri.blff]

(In reply to Tony Chung [:tchung] from comment #3)
> mozilla QA cannot setup a simulated SIM network as we dont have these types
> of tools to reproduce. If someone can share a real-world scenario to
> reproduce this, then please add qawanted back with STR. 
> 
> removing qawanted in the meantime as this is not actionable by our QA here.

you can emulater an "setup idle text" command;

Comment 17

[buri.blff]

mozilla please update status

Comment 18

[Daniel Coloma:dcoloma]

What is current status here? Has buri created a vendor-specific patch that is applied to buri builds?

Comment 19

[sync-1]

Comment from Mozilla:What is current status here? Has buri created a vendor-specific patch that is applied to buri builds?

Comment 20

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

triage with partner, this bug is to proceed with waiver, leo? for next release consideration

Comment 21

[sync-1]

Comment from Mozilla:triage with partner, this bug is to proceed with waiver, leo? for next release consideration

Comment 22

[leo.bugzilla.gecko]

in triages, it's a blocker for leo.

Comment 23

[Noemí Freire (:noemi)]

Hi Etienne,

Could you please inform us about the current status of notification API regarding handling permanent notifications?.

Thanks,

Comment 24

[Etienne Segonzac (:etienne)]

(In reply to Noemí Freire (:noemi) from comment #23)
> Hi Etienne,
> 
> Could you please inform us about the current status of notification API
> regarding handling permanent notifications?.
> 
> Thanks,

It's not part of the spec AFAIK... [1]

[1] http://www.w3.org/TR/notifications/

Comment 25

[Wayne Chang [:wchang]]

Triage - changing to leo- given comment 24 and https://bugzilla.mozilla.org/show_bug.cgi?id=846966#c12 

Tracking.

Comment 26

[buri.blff]

we find the v1.1 has modify this pr;
but if not click OK button, prompt message will keep display no
matter click home key or any other action.
this is not accepted.

Comment 27

[buri.blff]

and there is some error of STK command;
V1.1 for all STk command, we will receive response immediately, this is not right,
some STK command need some delay to send response.
can you tell me who is response for STK application??
thanks.

Comment 28

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! This is Germán. Fernando is on PTO these days. I am taking his bugs until I am also on PTO :-p I would appreciate if possible a clear statement of the situation of this bug as detailed as possible for someone not working on this bug from the beginning, please, specially about comment 26:
   - v1.1 has modified which PR, the notification.js file? To which extent?
and comment 27:
   - More detailed information would be highly appreciated ;-)
Thanks!

Comment 29

[buri.blff]

we find the v1.1 has modified pr 873397.
the STK APP about this pr is not modified.
I am not sure whether the notification.js  has been modified?

Comment 30

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi,

I'm not sure the current notification.js status...

Regarding the inmediate response is only in commands wich didn't need user iteraction and is sent as soon as the message is processed. What's the minimal time to wait?

Comment 31

[buri.blff]

(In reply to Carol Yang [:cyang] from comment #5)
> Hi sync-1,
> 
> The current behavior seems to be in line with how Notification bar should
> behave (once selected, the item is dismissed). Fernando can probably provide
> better insight on this though.
> 
> Also, do you know how Android behaves once the item is selected from
> Notification bar?

the notification for this stk command with Android dosen't disappear after selected.
And the notification can be replaced with new idle text;
When receive idle text with '', then the notification disappear;

Comment 32

[buri.blff]

(In reply to Fernando R. Sela [:frsela] from comment #30)
> Hi,
> 
> I'm not sure the current notification.js status...
> 
> Regarding the inmediate response is only in commands wich didn't need user
> iteraction and is sent as soon as the message is processed. What's the
> minimal time to wait?

there is no time out for IDLE text STK command;
Now when user click the notification, we can see the alert, but when we press home, the alert can't go to background;
The alert should go to background when we press home.
if there is another idle text, the new notification should replace the old notification.  The old alert should be close and give the new alert.

Comment 33

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to buri.blff from comment #32)
> 
> there is no time out for IDLE text STK command;
> Now when user click the notification, we can see the alert, but when we
> press home, the alert can't go to background;
> The alert should go to background when we press home.
> if there is another idle text, the new notification should replace the old
> notification.  The old alert should be close and give the new alert.

Creating new alerts (to workaround the notification helper pending work) had been done in the past and wasn't landed cause the UX was awful.

I prefer to wait Notification Helper support for non-volatile notifications ;)

Comment 34

[Preeti Raghunath(:Preeti)]

Minus, we shipped 1.0/1.1/1.2 with this

Comment 35

[Jack Liu]

Dears, It's GCF blocker, OEM can't keep asking for waive in 1.3/1.4/1.5...

Comment 36

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Jack Liu from comment #35)
> Dears, It's GCF blocker, OEM can't keep asking for waive in 1.3/1.4/1.5...

There is an old workaround PR proposal which re-activates the notification each time it's consumed:

https://github.com/mozilla-b2g/gaia/pull/7924/files

but it was never landed. We can rebase it or look for a better solution than a workaround.

Comment 37

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #36)
> (In reply to Jack Liu from comment #35)
> > Dears, It's GCF blocker, OEM can't keep asking for waive in 1.3/1.4/1.5...
> 
> There is an old workaround PR proposal which re-activates the notification
> each time it's consumed:
> 
> https://github.com/mozilla-b2g/gaia/pull/7924/files
> 
> but it was never landed. We can rebase it or look for a better solution than
> a workaround.

Hi Fernando, could you help to rebase it on 1.3 such that TCL can give it a try to see if the patch works?

Thanks

Vance

Comment 38

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

I'll do a better thing ... since this old code is very ugly ... I'll move to the new landed Notification API (https://developer.mozilla.org/en-US/docs/WebAPI/Using_Web_Notifications) which maintain the notification into the notification bar.

I'll upload a new patch this morning

Comment 39

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: STK, Using the new Notification API

Removed Notification Helper dependency since it's deprecated and using the new and better Notification API

Comment 41

[Kunny Liu]

Hi Fernando:
I will merge this patch and test again.

Comment 42

[Kunny Liu]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #39)
> Created attachment 8387450 [details] [review]
> STK, Using the new Notification API
> 
> Removed Notification Helper dependency since it's deprecated and using the
> new and better Notification API

This patch still can not fix the problem.
After click notification item and close the alert message. The notification item disappear.

Comment 43

[Jack Liu]

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Dear, please turn to kunny([e-mail]) instead.

Comment 44

[Kunny Liu]

Hi Fernando:
I have fixed this issue in Notification.js. Thanks for you support!

Comment 45

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Kunny Liu from comment #42)
> This patch still can not fix the problem.
> After click notification item and close the alert message. The notification
> item disappear.

Hi,

Here you can see a video of this patch working.
The notification is maintained into the notification bar but not into the lock screen (the same as Android devices)

https://www.youtube.com/watch?v=0Q6SIIQVg1A&feature=youtu.be

Regards.

Comment 46

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Kunny Liu from comment #44)
> Hi Fernando:
> I have fixed this issue in Notification.js. Thanks for you support!

Hi Kunny,
Could you share your patch with us?

Comment 47

[Kunny Liu]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #46)
> (In reply to Kunny Liu from comment #44)
> > Hi Fernando:
> > I have fixed this issue in Notification.js. Thanks for you support!
> 
> Hi Kunny,
> Could you share your patch with us?

OK, I will attach patch latter.

Comment 48

[Noemí Freire (:noemi)]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #45)
> (In reply to Kunny Liu from comment #42)
> > This patch still can not fix the problem.
> > After click notification item and close the alert message. The notification
> > item disappear.
> 
> Hi,
> 
> Here you can see a video of this patch working.
> The notification is maintained into the notification bar but not into the
> lock screen (the same as Android devices)
> 
> https://www.youtube.com/watch?v=0Q6SIIQVg1A&feature=youtu.be
> 
> Regards.


We will ask for this patch review to be landed in master.
Clearing v1.3? flag since the partner already has a fix. Please feel free to re-nom if needed.

Comment 49

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Removing f- since it's not caused by bad patch:
https://bugzilla.mozilla.org/show_bug.cgi?id=873397#c43

Comment 50

[Kunny Liu]

Attachment: patch_for_Bug873397.txt

Comment 51

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

The code looks good to me. Flagging Alive as I'm not able to review code in system app. And please add unit tests for the case.

Comment 52

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

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Could you write unit test for icc_worker?
Also I don't think use system app icon as the notification icon works, how does UX think?

Comment 54

[Nivi]

This affects https://bugzilla.mozilla.org/show_bug.cgi?id=980802.
Marking it as blocking 1.3 because 980802 was reported on Sora.

Thanks,
Nivi.

Comment 55

[Jason Smith [:jsmith]]

This is an old bug from 1.1 which is not a regression, so will not block the release.

Comment 56

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Arthur Chen [:arthurcc] from comment #51)
> Comment on attachment 8387450 [details] [review]
> STK, Using the new Notification API
> 
> The code looks good to me. Flagging Alive as I'm not able to review code in
> system app. And please add unit tests for the case.

Thanks Arthur. I'll work on unit test this week.

Comment 57

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Hi Arthur,

Unit tests added, can you review it?
Thanks

Comment 58

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi Stephany,

We should decide which icon will be used for STK operator notifications. Currently we're using the system app icon as you can see in this video:

http://www.youtube.com/watch?v=0Q6SIIQVg1A&list=UUlPnj8O8nkBDxeWrXeRCT5w

But UX shall decide which is the best one ;)

Thanks in advance

Comment 59

[Stephany Wilkes]

Flagging Omega because this affects Settings.

Comment 60

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Redirect to the owner of system app.

Comment 61

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

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

See github.

Comment 62

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Unit tests added

Comment 63

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

Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Please remove the setTimeout call in mocks.
This will cause some race condition in tests. Please call onshow from your test directly.

Comment 64

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #63)
> Comment on attachment 8387450 [details] [review]
> STK, Using the new Notification API
> 
> Please remove the setTimeout call in mocks.
> This will cause some race condition in tests. Please call onshow from your
> test directly.

Thanks Alive.

Better this? - https://github.com/frsela/gaia/commit/c9988ca0630964136e9e32db673247e846b4fb54

Comment 65

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #64)
> (In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment
> #63)
> > Comment on attachment 8387450 [details] [review]
> > STK, Using the new Notification API
> > 
> > Please remove the setTimeout call in mocks.
> > This will cause some race condition in tests. Please call onshow from your
> > test directly.
> 
> Thanks Alive.
> 
> Better this? -
> https://github.com/frsela/gaia/commit/
> c9988ca0630964136e9e32db673247e846b4fb54

Sorry, this one:
https://github.com/frsela/gaia/commit/b6660d487c426c261af6e909af2cf152cbcd768c

Comment 66

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

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #65)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #64)
> > (In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment
> > #63)
> > > Comment on attachment 8387450 [details] [review]
> > > STK, Using the new Notification API
> > > 
> > > Please remove the setTimeout call in mocks.
> > > This will cause some race condition in tests. Please call onshow from your
> > > test directly.
> > 
> > Thanks Alive.
> > 
> > Better this? -
> > https://github.com/frsela/gaia/commit/
> > c9988ca0630964136e9e32db673247e846b4fb54
> 
> Sorry, this one:
> https://github.com/frsela/gaia/commit/
> b6660d487c426c261af6e909af2cf152cbcd768c

go ahead!

Comment 67

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #66)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #65)
> > (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> > comment #64)
> > > (In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment
> > > #63)
> > > > Comment on attachment 8387450 [details] [review]
> > > > STK, Using the new Notification API
> > > > 
> > > > Please remove the setTimeout call in mocks.
> > > > This will cause some race condition in tests. Please call onshow from your
> > > > test directly.
> > > 
> > > Thanks Alive.
> > > 
> > > Better this? -
> > > https://github.com/frsela/gaia/commit/
> > > c9988ca0630964136e9e32db673247e846b4fb54
> > 
> > Sorry, this one:
> > https://github.com/frsela/gaia/commit/
> > b6660d487c426c261af6e909af2cf152cbcd768c
> 
> go ahead!

Squazed and rebased. As soon as travis finish, I'll land it.

Thank you !

Comment 68

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Landed - https://github.com/mozilla-b2g/gaia/commit/10ea0c1f6367d7cc2c4f5d12d652aa3a0edde85d