Closed Bug 873397 Opened 8 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog, b2g18+, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S6 (25apr)
tracking-b2g backlog
Tracking Status
b2g18 + ---
b2g-v2.0 --- fixed

People

(Reporter: sync-1, Assigned: frsela)

References

Details

Attachments

(3 files)

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:
This bug is GCF block
blocking-b2g: --- → tef?
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
Keywords: qawanted
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.
Keywords: qawanted
cyang/frsela, not sure if you have any idea for this bug? thanks
Flags: needinfo?(frsela)
Flags: needinfo?(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?
Flags: needinfo?(cyang) → needinfo?(sync-1)
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!
Flags: needinfo?(sync-1)
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, ...
Flags: needinfo?(frsela)
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?
Flags: needinfo?(frsela)
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.
the modifcation is as between 
 //*** add for pr:455177 according to TS 11.44 6.4.22
 //end
(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
Flags: needinfo?(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?
the modifcation code is flag with follow flag:
 
 //*** add for pr:455177 according to TS 11.44 6.4.22
 //end
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"
(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;
mozilla please update status
What is current status here? Has buri created a vendor-specific patch that is applied to buri builds?
Flags: needinfo?(sync-1)
Comment from Mozilla:What is current status here? Has buri created a vendor-specific patch that is applied to buri builds?
triage with partner, this bug is to proceed with waiver, leo? for next release consideration
blocking-b2g: tef? → leo?
Comment from Mozilla:triage with partner, this bug is to proceed with waiver, leo? for next release consideration
in triages, it's a blocker for leo.
blocking-b2g: leo? → leo+
Hi Etienne,

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

Thanks,
Flags: needinfo?(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/
Flags: needinfo?(etienne)
Blocks: 846966
Triage - changing to leo- given comment 24 and https://bugzilla.mozilla.org/show_bug.cgi?id=846966#c12 

Tracking.
blocking-b2g: leo+ → ---
tracking-b2g18: --- → +
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.
Flags: needinfo?(sync-1)
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.
Flags: needinfo?(frsela)
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!
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?
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?
Flags: needinfo?(frsela) → needinfo?(sync-1)
(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;
(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.
Flags: needinfo?(sync-1)
(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 ;)
blocking-b2g: --- → 1.3?
Minus, we shipped 1.0/1.1/1.2 with this
blocking-b2g: 1.3? → ---
Dears, It's GCF blocker, OEM can't keep asking for waive in 1.3/1.4/1.5...
blocking-b2g: --- → 1.3?
Flags: needinfo?(vchen)
(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.
(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
Flags: needinfo?(vchen) → needinfo?(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
Flags: needinfo?(frsela)
Assignee: nobody → frsela
Removed Notification Helper dependency since it's deprecated and using the new and better Notification API
Duplicate of this bug: 980803
Attachment #8387450 - Flags: feedback?(liuyongming)
Hi Fernando:
I will merge this patch and test again.
(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 on attachment 8387450 [details] [review]
STK, Using the new Notification API

Dear, please turn to kunny(liukun@tcl.com) instead.
Attachment #8387450 - Flags: feedback?(liuyongming) → feedback-
Hi Fernando:
I have fixed this issue in Notification.js. Thanks for you support!
(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.
(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?
(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.
(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.
blocking-b2g: 1.3? → ---
Attachment #8387450 - Flags: review?(arthur.chen)
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
Attachment #8387450 - Flags: feedback-
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.
Attachment #8387450 - Flags: review?(arthur.chen)
Attachment #8387450 - Flags: review?(alive)
Attachment #8387450 - Flags: feedback+
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?
Attachment #8387450 - Flags: review?(alive)
Duplicate of this bug: 846966
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.
blocking-b2g: --- → 1.3?
This is an old bug from 1.1 which is not a regression, so will not block the release.
blocking-b2g: 1.3? → backlog
(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 on attachment 8387450 [details] [review]
STK, Using the new Notification API

Hi Arthur,

Unit tests added, can you review it?
Thanks
Attachment #8387450 - Flags: review?(arthur.chen)
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
Flags: needinfo?(swilkes)
Flagging Omega because this affects Settings.
Flags: needinfo?(swilkes) → needinfo?(ofeng)
Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Redirect to the owner of system app.
Attachment #8387450 - Flags: review?(arthur.chen) → review?(alive)
Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

See github.
Attachment #8387450 - Flags: review?(alive)
Comment on attachment 8387450 [details] [review]
STK, Using the new Notification API

Unit tests added
Attachment #8387450 - Flags: review?(arthur.chen)
Attachment #8387450 - Flags: review?(arthur.chen) → review?(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.
Attachment #8387450 - Flags: review?(alive) → review+
(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
(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
Flags: needinfo?(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!
Flags: needinfo?(alive)
(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 !
Target Milestone: --- → 1.4 S6 (25apr)
Landed - https://github.com/mozilla-b2g/gaia/commit/10ea0c1f6367d7cc2c4f5d12d652aa3a0edde85d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: needinfo?(ofeng)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.