Closed Bug 1016807 Opened 7 years ago Closed 6 years ago

B2G STK: Support for STK icon display (GAIA work for Bug 824145)

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.2+, tracking-b2g:backlog, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S6 (20feb)
feature-b2g 2.2+
tracking-b2g backlog
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: wesley_huang, Assigned: mancas)

References

Details

(Whiteboard: [caf priority: p2][CR 786371][FT:COMMS])

Attachments

(19 files, 8 obsolete files)

46 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
timdream
: review+
eragonj
: review+
timdream
: feedback+
Details | Review
26.28 KB, image/png
Details
14.23 KB, image/png
Details
236.33 KB, image/png
Details
37.42 KB, image/png
Details
36.52 KB, image/png
jelee
: ui-review+
Details
874.00 KB, image/jpeg
Details
632.97 KB, image/jpeg
Details
50.49 KB, image/png
HHuang
: ui-review+
Details
50.17 KB, image/png
HHuang
: ui-review+
Details
17.20 KB, image/png
HHuang
: ui-review+
Details
21.63 KB, image/png
HHuang
: ui-review+
Details
21.20 KB, image/png
HHuang
: ui-review+
Details
17.83 KB, image/png
HHuang
: ui-review+
Details
20.18 KB, image/png
HHuang
: ui-review+
Details
20.55 KB, image/png
HHuang
: ui-review+
Details
13.74 KB, image/png
HHuang
: ui-review+
Details
46 bytes, text/x-github-pull-request
Details | Review
No description provided.
Blocks: 824145
blocking-b2g: --- → 2.1?
Whiteboard: [FT:COMMS]
Depends on 824145, not blocks.
We cann't do the Gaia work since it is not supported by gecko ;)
No longer blocks: 824145
Depends on: 824145
Duplicate of this bug: 1031190
Setting feature-b2g = 2.1 per FxOS V2.1 Release Planning and Feature Scope.
blocking-b2g: 2.1? → backlog
feature-b2g: --- → 2.1
See Also: → 869745
QA Whiteboard: [COM=RIL]
The GECKO dependency (bug 824145) is planned to be landed in sprint2. 
Considering PTOs, are we confident to commit from GAIA side in sprint3?
Flags: needinfo?(oteo)
Flags: needinfo?(frsela)
(In reply to Wesley Huang [:wesley_huang] from comment #4)
> The GECKO dependency (bug 824145) is planned to be landed in sprint2. 
> Considering PTOs, are we confident to commit from GAIA side in sprint3?

Oh, great news ! :)

We'll do our best to add support in GAIA ASAP.

Can you provide me some sample STK messages with the ICON as will be sent by Gecko in order to start implementing it now?

Thanks in advance.
Flags: needinfo?(whuang)
Flags: needinfo?(oteo)
Flags: needinfo?(frsela)
Changing NI to Jessica since she is working on the Gecko part
Flags: needinfo?(whuang) → needinfo?(jjong)
Hi Fernando, based on the latest webidl, see Part 1, v3 in Bug 824145, a STK_CMD_DISPLAY_TEXT message would look like this:

{"commandNumber":1,
 "typeOfCommand":0x21,
 "commandQualifier":0x80,
 "rilMessageType":"stkcommand",
 "options":{"text":"Basic Icon",
            "userClear":true,
            "iconSelfExplanatory":true,
            "icons":[{"pixels":[0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,
                                0x000000FF,0x000000FF,0x000000FF,0x000000FF,0x000000FF,0x000000FF,0xFFFFFFFF,0xFFFFFFFF,
                                0xFFFFFFFF,0x000000FF,0xFFFFFFFF,0x000000FF,0x000000FF,0xFFFFFFFF,0x000000FF,0xFFFFFFFF,
                                0xFFFFFFFF,0x000000FF,0x000000FF,0xFFFFFFFF,0xFFFFFFFF,0x000000FF,0x000000FF,0xFFFFFFFF,
                                0xFFFFFFFF,0x000000FF,0x000000FF,0xFFFFFFFF,0xFFFFFFFF,0x000000FF,0x000000FF,0xFFFFFFFF,
                                0xFFFFFFFF,0x000000FF,0xFFFFFFFF,0x000000FF,0x000000FF,0xFFFFFFFF,0x000000FF,0xFFFFFFFF,
                                0xFFFFFFFF,0xFFFFFFFF,0x000000FF,0x000000FF,0x000000FF,0x000000FF,0xFFFFFFFF,0xFFFFFFFF,
                                0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF],
                      "codingScheme":"basic",
                      "width":8,
                      "height":8}]},
 "rilMessageClientId":0}


Note that this icon is 8x8 and is black and white only. The webidl is still under review, this is just to give you a preview of how it would look like, hope it helps! :)
Flags: needinfo?(jjong)
This isn't a feature blocker in 2.1, so let me set it to 2.2 at this moment.
If not being made before 2.1 FL, partner can cherry pick from later versions.
feature-b2g: 2.1 → 2.2
Hi Fernando,
Are you able to take it and land before FL?
Branch day will be end of Aug.
Flags: needinfo?(frsela)
(In reply to Wesley Huang [:wesley_huang] from comment #9)
> Hi Fernando,
> Are you able to take it and land before FL?
> Branch day will be end of Aug.

Hi Wesley,

I just return from my last PTO period. I'll check pending work today and then I'll work on this topic.
Flags: needinfo?(frsela)
(In reply to Wesley Huang [:wesley_huang] from comment #8)
> This isn't a feature blocker in 2.1, so let me set it to 2.2 at this moment.
> If not being made before 2.1 FL, partner can cherry pick from later versions.

Hi Wesley, We don't have time to do the Gaia work before 2.1 FL (1 Sept), anyway this feature is for 2.2, isn't it?
This PR only has the decoding function.

Pending add the icon to each STK message. I think UX should define where to shown each one in each STK screen.

--

From TS 101.267 Section 6.6, the proactive commands that may contain an icon are:
- DISPLAY TEXT
- GET INKEY
- GET INPUT
- PLAY TONE
- SET-UP MENU*
- SELECT ITEM*
- SEND SHORT MESSAGE/SS/USSD
- SET UP CALL
- SET UP IDEL MODE TEXT
- RUN AT COMMAND
- SEND DTMF COMMAND
- LAUNCH BROWSER (user confirmation phase)
- OPEN/CLOSE CHANNEL
- RECEIVE/SEND DATA
Attachment #8478311 - Flags: feedback?(jjong)
Flags: needinfo?(hhsu)
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Jessica is taking PTO so I'm forwarding the ni to HsinYi.

Hi Fernando,
I see you have a WIP. 
Do you want to take this bug? 
And, can we expect to have it landed before branch (which means end of this week).
Flags: needinfo?(frsela)
(In reply to Wesley Huang [:wesley_huang] from comment #14)
> Jessica is taking PTO so I'm forwarding the ni to HsinYi.
> 
> Hi Fernando,
> I see you have a WIP. 
> Do you want to take this bug? 

No problem on my side to take the bug...

> And, can we expect to have it landed before branch (which means end of this
> week).

Sorry, but I've more urgent task on the table and this is a 2.2.
As addressed by María Angeles in comment #11, we don't have enough time to finish this week. Sorry for that.

If you have someone in your team who can finish this WIP faster than us feel free to get my code and continue with it.

The patch is only the decoding function for basic images (the unique example we've from the new API). It's needed to change the STK screens in order to show these new icons. Not sure if UX should be involved here since we need to combine icons + text and only icon dialogs (iconSelfExplanatory parameter)
Flags: needinfo?(frsela)
Hi Fernando,
Could you provide some background on the STK? So that UX/Visual could have better idea on how to help out here.
Thanks
Flags: needinfo?(hhsu) → needinfo?(frsela)
(In reply to Harly Hsu from comment #16)
> Hi Fernando,
> Could you provide some background on the STK? So that UX/Visual could have
> better idea on how to help out here.
> Thanks

Hi Harly.

With this new patch, we well support STK Icons, in other words, the SIM card will send images that should be showed to the user.

Also, the SIM Card sents textual information. In some cases both (image and text) should be showed but in other cases only image (if supported) will be showed (depends on the iconSelfExplanatory value).

Finally, the STK commands that can send an icon will be:

- DISPLAY TEXT -> This opens a full-screen dialog with the text as this screenshot: [1]
- GET INKEY -> This opens a full-screen dialog asking for a key press (like the PIN dialog)
- GET INPUT -> This opens a full-screen dialog for user input text. See [2]
- PLAY TONE -> Now this command only plays a sound. Optionaly will show a text in the same way as DISPLAY_TEXT
- SET-UP MENU* -> This is the MAIN STK menu showed in Settings when you enter in the Operators menu
- SELECT ITEM* -> This is a second (or more) level STK menu when selected one option in the main stk menu. Both are now displayed as a list in Settings app.
- SEND SHORT MESSAGE/SS/USSD -> Optionally will show a confirmation message like [3]
- SET UP CALL -> Optionally will show a confirmation message like [3]
- SET UP IDEL MODE TEXT -> This command will show a message into the Notification Bar
- RUN AT COMMAND -> Not implemented yet
- SEND DTMF COMMAND -> Optionally will show a confirmation message like [3]
- LAUNCH BROWSER (user confirmation phase) -> Before open the browser, a confirmation message like [3]
- OPEN/CLOSE CHANNEL -> Not implemented yet
- RECEIVE/SEND DATA -> Not implemented yet

[1] https://bug1009254.bugzilla.mozilla.org/attachment.cgi?id=8429258
[2] https://bug1009254.bugzilla.mozilla.org/attachment.cgi?id=8429263
[3] https://bug1009254.bugzilla.mozilla.org/attachment.cgi?id=8429264
Flags: needinfo?(frsela)
Hi Harly,

Let me give you more concrete data about STK icons. According to the 3gpp spec, the icon's width/height could be up to 255 (1 byte). I haven't seen any real sim cards with STK icon support, but from the real test data I've ever seen, the width/height is 16. But note, this is just one example :(
Comment on attachment 8478311 [details] [review]
WIP: Function to decode STK Images

The decoding function looks good to me, and it should work for both basic and color/transparent images, since they are both in RGBA format. You can refer to the latest interface at [1]. 

Would like to check it again after the icons can be shown. Thanks!
Please note that there is follow-up work in Bug 1061535.


[1] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozStkCommandEvent.webidl#9
Attachment #8478311 - Flags: feedback?(jjong)
(In reply to Jessica Jong [:jjong] [:jessica] from comment #19)

Thank you for your feedback

> The decoding function looks good to me, and it should work for both basic
> and color/transparent images, since they are both in RGBA format. You can
> refer to the latest interface at [1]. 

In that case, I can remove this check:
  https://github.com/mozilla-b2g/gaia/pull/23274/files#diff-7b4cd981b7b9c4669c33a8cecd441af2R597

that I added to add a switch...case for the different codingSchemes but since all of them are equally coded, I didn't it anymore ... cool :)
Fernando, I have finished the implementation of this feature on our side and am ready to test the Gaia change. The current gaia PR doesn't look complete however so will wait for your new patch.
Hi Anshul.

Thank you !

Now I'm waiting for UX decission in how and were show the images. See comments #12, #16 & #17
ni Harly in case he didn't see the asks.
Flags: needinfo?(hhsu)
Transfer ni to Jenny as she is the UX for Settings.
Flags: needinfo?(hhsu) → needinfo?(jelee)
(In reply to Harly Hsu from comment #24)
> Transfer ni to Jenny as she is the UX for Settings.

Hello!
I don't think there's need to specially support layout for STK, under normal circumstances, applying common component should be enough (and the layout would fit our building block standard). If there's any page that might not be able to simply apply common component to, we will need screenshots, assets of those pages to do a layout. thanks!
Flags: needinfo?(jelee)
frsela, what are the next steps here per Jenny's feedback ? CAF is requesting this for 2.2 and want your confirmation that this can/will be fixed in the next few weeks before setting expectation.
Flags: needinfo?(frsela)
(In reply to bhavana bajaj [:bajaj] from comment #26)
> frsela, what are the next steps here per Jenny's feedback ? CAF is
> requesting this for 2.2 and want your confirmation that this can/will be
> fixed in the next few weeks before setting expectation.

Hi,

The proposed patch, decodes the received icon from STK. Next step is HOW and WHERE we should display these icons in the STK menus and dialogs.

So, we need some wireframes or similar from UX team.

The icons could appear on these commands:

- DISPLAY TEXT -> An "alert box" is displayed to the user
- GET INKEY -> An "input box" is displayed to the user
- GET INPUT -> Idem.
- PLAY TONE -> Same as display text
- SET-UP MENU* -> The STK menu entry in settings app
- SELECT ITEM* -> The STK apps entries in settings app
- SEND SHORT MESSAGE/SS/USSD -> A "confirmation box" is displayed to the user
- SET UP CALL -> A "confirmation box" is displayed to the user
- SET UP IDEL MODE TEXT -> A message in the notification bar is displayed
- RUN AT COMMAND -> Not implemented yet
- SEND DTMF COMMAND -> A "confirmation box" is displayed to the user
- LAUNCH BROWSER (user confirmation phase) -> A "confirmation box" is displayed to the user
- OPEN/CLOSE CHANNEL -> Not implemented yet
- RECEIVE/SEND DATA -> Not implemented yet

Regards
Flags: needinfo?(frsela)
feature-b2g: 2.2? → 2.2+
Flags: needinfo?(jelee)
Hi Wesley, per discussion, we will help ui-review the screens instead of providing layout spec. Thanks!
Flags: needinfo?(jelee)
Hi Maria,
Can you help out on getting a spec for the change needed here?  Jenny above can help review if you have any questions.

Thanks.
Flags: needinfo?(oteo)
QA Whiteboard: [COM=RIL] → [COM=RIL][2.2-feature-qa+]
Hi Bruce,

We don't have a specific guideline to be followed here, ni to Carol, maybe she can shed some light on the specific requirements. Thanks!
Flags: needinfo?(oteo) → needinfo?(cyang)
(In reply to Noemí Freire (:noemi) from comment #30)
> Hi Bruce,
> 
> We don't have a specific guideline to be followed here, ni to Carol, maybe
> she can shed some light on the specific requirements. Thanks!

Nothing specific that I know of regarding how the icons should be displayed. We just follow 3GPP TS 11.14 for how to decode.
Flags: needinfo?(cyang)
Assignee: nobody → b.mcb
(In reply to Carol Yang [:cyang] from comment #31)
> (In reply to Noemí Freire (:noemi) from comment #30)
> > Hi Bruce,
> > 
> > We don't have a specific guideline to be followed here, ni to Carol, maybe
> > she can shed some light on the specific requirements. Thanks!
> 
> Nothing specific that I know of regarding how the icons should be displayed.
> We just follow 3GPP TS 11.14 for how to decode.

Carol, Many thanks for your quick response. Then, we could try to come up with some proposal but our UX team is overloaded right now so it might not be possible within 2.2 timeframe
Q1. Test with emulator, is this the only way to test currnetly? Any supported SIM?

2. Original STK test cases for regression.
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-status=active&filter-productversion=196&filter-suite=214
3. Exploratory on STK for regression.
Flags: in-moztrap?(echang)
(In reply to Eric Chang [:ericcc] [:echang] from comment #33)
> Q1. Test with emulator, is this the only way to test currnetly? Any
> supported SIM?
> 

Our Tef friends tried so hard to find us some but in vain... so, testing with emulator is the only way I know for now.

> 2. Original STK test cases for regression.
> https://moztrap.mozilla.org/manage/cases/
> ?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-
> status=active&filter-productversion=196&filter-suite=214
> 3. Exploratory on STK for regression.
Hi Noemi,

Although the branch date of v2.2 is Jan12, I believe uplift is acceptable before the actual FL milestone (Feb23). To allocate some buffer, landing around early February still works, IMO.
Do you think UX team can support with this schedule?
Flags: needinfo?(noemi.freiredecarlos)
Hi Wesley,

We will confirm you the feasibility of that approach just after Christmas break. Thanks!
Flags: needinfo?(noemi.freiredecarlos)
Hi Wesley,

We will confirm you the feasibility of that approach just after Christmas break. Thanks!
Status: NEW → ASSIGNED
Flags: in-moztrap?(echang) → in-moztrap-
Attached file Proposed patch
Hey Tim, could you review the new patch? I've used the previous work made by frsela in order to add icons to every STK dialog.
Attachment #8545111 - Flags: review?(timdream)
Comment on attachment 8545111 [details] [review]
Proposed patch

You can land this as-is or take my recommendations, assuming this do what's intended.
Attachment #8545111 - Flags: review?(timdream) → review+
Manuel, I took the liberty to test your gaia patch and have the following observations.

1. Black and white icon for a command like get input is working as expected. I didn't try all the STK commands though.
2. Setup menu doesn't seem to support the icons with your change. Is that intentional?
3. Color icon displayed is smaller in size and the placement of the icon seems weird as compared to Android. Please find the snapshots attached. I think Android is stretching the icon, please evaluate.
(In reply to Anshul from comment #40)
> Manuel, I took the liberty to test your gaia patch and have the following
> observations.
> 
> 1. Black and white icon for a command like get input is working as expected.
> I didn't try all the STK commands though.
> 2. Setup menu doesn't seem to support the icons with your change. Is that
> intentional?
> 3. Color icon displayed is smaller in size and the placement of the icon
> seems weird as compared to Android. Please find the snapshots attached. I
> think Android is stretching the icon, please evaluate.

Thanks for testing it Anshul! I've not implemented yet the support of the icons for the setup menu command. Do you know how will be the final command? I would like to know if each item of the menu will have its own icon or the command will send a list of icons.

In relation with the color icons, I will take a look.
Flags: needinfo?(anshulj)
Manuel, please find a shanpshot of Android's STK menu with icon support as a reference.
Flags: needinfo?(anshulj)
Manuel, other observation is that the icon info I am sending is for an 8 x 8 icon but its being displayed as a 12 x 12 icon, which is causing the icon to be stretched and loosing its symmetrical shape a little. Is that expected?
Flags: needinfo?(b.mcb)
Anshul, what you said is not expected. The icon dimension is retrieved from the command:

{"commandNumber":1,
 "typeOfCommand":0x21,
 "commandQualifier":0x80,
 "rilMessageType":"stkcommand",
 "options":{"text":"Basic Icon",
            "userClear":true,
            "iconSelfExplanatory":true,
            "icons":[{"pixels":[..],
                      "codingScheme":"basic",
                      "width":8, <-- WIDTH
                      "height":8}]}, <-- HEIGHT
 "rilMessageClientId":0}

What command are you using for test the patch?
Flags: needinfo?(b.mcb) → needinfo?(anshulj)
Rgression for moztrap+

2. Original STK test cases for regression.
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-status=active&filter-productversion=196&filter-suite=214
3. Exploratory on STK for regression.
Flags: in-moztrap- → in-moztrap+
Attached image dialog_with_text_and_multiple_icons.png (obsolete) —
Attachment #8548155 - Flags: ui-review?(jelee)
Attached image dialog_with_text_and_icon.png (obsolete) —
Attachment #8548156 - Flags: ui-review?(jelee)
Attached image dialog_with_multiple_icons.png (obsolete) —
Attachment #8548157 - Flags: ui-review?(jelee)
Attached image dialog_with_input_text_and_icon.png (obsolete) —
Attachment #8548158 - Flags: ui-review?(jelee)
Attached image dialog_with_input_and_multiple_icons.png (obsolete) —
Attachment #8548159 - Flags: ui-review?(jelee)
Attached image dialog_with_input and icon.png (obsolete) —
Attachment #8548160 - Flags: ui-review?(jelee)
Attached image dialog_with_icon.png (obsolete) —
Attachment #8548162 - Flags: ui-review?(jelee)
Attached image list_icon_align.png (obsolete) —
Attachment #8548163 - Flags: ui-review?(jelee)
Attached image menu_icon.png
Attachment #8548164 - Flags: ui-review?(jelee)
It's time to make a brief resume of what we've done here. Firstly the patch allow us to handle STK messages with icons. This messages looks like the one in comment 7. As you can see, we could receive more than one icon.

Also, the SET-UP MENU command and the SELECT ITEM handle icons too. However, if we receive more than one icon per menu or item, we choose the first one in order to represent the entry.

In respect with the SDK dialogs, we have a few cases because the icons could be present or not and they could be self explanatory. Those cases are:

1 - Dialog with icon and text
2 - Dialog with icon
3 - Dialog with multiple icons
4 - Dialog with text
5 - Dialog with text and multiple icons
6 - Dialog with input, text and icon
7 - Dialog with input and text
8 - Dialog with input and icon
9 - Dialog wit input and multiple icons

The next step is to get the approval from the Mozilla UX team, and then, we will take care of minor issues. The important decision here is the place where the icons will be placed.
Flags: needinfo?(jelee)
Attachment #8545111 - Flags: review+ → review?(timdream)
Adding ni to Anshul too, your feedback will be more than welcome!. Thanks.
Did anything changed since I review the patch last time?
Flags: needinfo?(b.mcb)
Comment on attachment 8548155 [details]
dialog_with_text_and_multiple_icons.png

The "yes, no" button styling at the bottom is only used in a dialog. Normally we would use cancel (icon "x") and "ok" button on header on a page like this.
Flags: needinfo?(jelee)
Attachment #8548155 - Flags: ui-review?(jelee) → ui-review-
Comment on attachment 8548156 [details]
dialog_with_text_and_icon.png

The "yes, no" button styling at the bottom is only used in a dialog. Normally we would use cancel (icon "x") and "ok" button on header on a page like this.
Attachment #8548156 - Flags: ui-review?(jelee) → ui-review-
Comment on attachment 8548157 [details]
dialog_with_multiple_icons.png

The "yes, no" button styling at the bottom is only used in a dialog. Normally we would use cancel (icon "x") and "ok" button on header on a page like this.
Attachment #8548157 - Flags: ui-review?(jelee) → ui-review-
Comment on attachment 8548158 [details]
dialog_with_input_text_and_icon.png

Same comment with the footer. 
Also the shaking transition of the text input field is not suggested.
Attachment #8548158 - Flags: ui-review?(jelee) → ui-review-
Comment on attachment 8548159 [details]
dialog_with_input_and_multiple_icons.png

Same comment with the footer. Icon is not suggested to be center-aligned.
Attachment #8548159 - Flags: ui-review?(jelee) → ui-review-
Comment on attachment 8548160 [details]
dialog_with_input and icon.png

Same comment with the footer. Icon is not suggested to be center-aligned.
Attachment #8548160 - Flags: ui-review?(jelee) → ui-review-
Comment on attachment 8548162 [details]
dialog_with_icon.png

Same comment with the footer.
Attachment #8548162 - Flags: ui-review?(jelee) → ui-review-
Comment on attachment 8548163 [details]
list_icon_align.png

It is preferred that if icon is used for one list item, all list items should have corresponding icon too.
Attachment #8548164 - Flags: ui-review?(jelee) → ui-review+
Hello Manuel,

I've given my rough feedback for each of the attachment. Please follow up with visual designer Helen for correct layout including icon size and placement. Thanks!

Hello Helen,
Please take some time to provide the layout reference needed here. Thank you =)
Flags: needinfo?(hhuang)
Attachment #8548163 - Flags: ui-review?(jelee) → ui-review+
(In reply to Jenny Lee from comment #62)
> Comment on attachment 8548156 [details]
> dialog_with_text_and_icon.png
> 
> The "yes, no" button styling at the bottom is only used in a dialog.
> Normally we would use cancel (icon "x") and "ok" button on header on a page
> like this.

Hi Jenny, in reply to comment 62 to comment 63:
This was implemented by this way, from the beginning. If you want to change the footer, please, open a new bug, because this issue is not related with the footer or the dialog style and we don't have any inconvenience if you want to change it.

We should focus on the size of the icons, the placement, etc.
Flags: needinfo?(b.mcb)
(In reply to Jenny Lee from comment #64)
> Comment on attachment 8548158 [details]
> dialog_with_input_text_and_icon.png
> 
> Same comment with the footer. 
> Also the shaking transition of the text input field is not suggested.

The shaking transition was implemented to act as a visual notification for the user in order to catch his attention. Again, if you want to remove this animation, open a new bug, please.
(In reply to Jenny Lee from comment #65)
> Comment on attachment 8548159 [details]
> dialog_with_input_and_multiple_icons.png
> 
> Same comment with the footer. Icon is not suggested to be center-aligned.

(In reply to Jenny Lee from comment #66)
> Comment on attachment 8548160 [details]
> dialog_with_input and icon.png
> 
> Same comment with the footer. Icon is not suggested to be center-aligned.

Please refer to the comment 70 for further information about the footer.

We have no guidelines about where we should place the icons, so please take this screenshots as a first approach. You said that the icon is not suggested to be center-aligned, so it should be left-aligned, shouldn't it?
(In reply to Jenny Lee from comment #68)
> Comment on attachment 8548163 [details]
> list_icon_align.png
> 
> It is preferred that if icon is used for one list item, all list items
> should have corresponding icon too.

I understand you, but we just display the icons that the command contains. The responsible of sending these icons is the operator. 

What I mean is that we have no control over the command or the icons, and the only thing we could do is leave a free space if the item hasn't got an icon.

So now, let's wait for Helen's feedback

Thanks Jenny!
Flags: needinfo?(jelee)
Manuel, take into account changes made in Bug #1117663.
You shall rebase this patch

https://bug1117663.bugzilla.mozilla.org/attachment.cgi?id=8544968
Depends on: 1117663
Flags: needinfo?(b.mcb)
Comment on attachment 8545111 [details] [review]
Proposed patch

Please tell me what changed and set the review again, thanks!
Attachment #8545111 - Flags: review?(timdream)
(In reply to Manuel Casas Barrado [:mancas] from comment #47)
> Anshul, what you said is not expected. The icon dimension is retrieved from
> the command:
> 
> {"commandNumber":1,
>  "typeOfCommand":0x21,
>  "commandQualifier":0x80,
>  "rilMessageType":"stkcommand",
>  "options":{"text":"Basic Icon",
>             "userClear":true,
>             "iconSelfExplanatory":true,
>             "icons":[{"pixels":[..],
>                       "codingScheme":"basic",
>                       "width":8, <-- WIDTH
>                       "height":8}]}, <-- HEIGHT
>  "rilMessageClientId":0}
> 
> What command are you using for test the patch?

Please find the command I am sending below.

{ commandNumber : 1, 
  typeOfCommand : 33, 
  commandQualifier : 128, 
  options : { text : 'Colour Icon', 
              userClear : true, 
              iconSelfExplanatory : true, 
              icons : [ {codingScheme : 'color', 
                         width : 8, 
                         height : 8, 
                         pixels : [...]}]}}
Flags: needinfo?(anshulj)
Attached image Reference mockups.jpg
Hi, I've checked the screenshots, and it feels some pages didn't align the OS guideline, so I make the mockups for reference.
Flags: needinfo?(hhuang)
Attached image Visual spec.jpg
Here is the visual spec for more information. Please let me know if any question.
Attachment #8548155 - Attachment is obsolete: true
Flags: needinfo?(b.mcb)
Attachment #8553042 - Flags: ui-review?(hhuang)
Attachment #8548156 - Attachment is obsolete: true
Attachment #8553043 - Flags: ui-review?(hhuang)
Attachment #8548157 - Attachment is obsolete: true
Attachment #8553044 - Flags: ui-review?(hhuang)
Attachment #8548158 - Attachment is obsolete: true
Attachment #8553047 - Flags: ui-review?(hhuang)
Attached image dialog_with_icon.png
Attachment #8548162 - Attachment is obsolete: true
Attachment #8553048 - Flags: ui-review?(hhuang)
Attachment #8548160 - Attachment is obsolete: true
Attachment #8553049 - Flags: ui-review?(hhuang)
Attachment #8548159 - Attachment is obsolete: true
Attachment #8553050 - Flags: ui-review?(hhuang)
Attached image list_icon_align.png
Attachment #8548163 - Attachment is obsolete: true
Attachment #8553051 - Flags: ui-review?(hhuang)
Hi Helen, thanks for your excellent work! Please take a look at the new screenshots to check if they fit the new visual requirements. Notice that the dialog style will be fixed in a follow-up of this bug, let's focus on the stk icons.

Just a comment, the icon size is set by the operator so we would suggest to keep it. 
Notice that in the current patch the icon size is kept to the size specified by the operator so no resize algorithm is applied.

Thanks again!
Attachment #8553042 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553043 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553044 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553045 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553047 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553048 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553049 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553050 - Flags: ui-review?(hhuang) → ui-review+
Attachment #8553051 - Flags: ui-review?(hhuang) → ui-review+
Hi, Thank you for the information, I think those icons on the screen look good, it works for me. 
Just a reminder, if the dialog style has been fixed please set ui review to me, thanks.
Flags: needinfo?(jelee)
Comment on attachment 8545111 [details] [review]
Proposed patch

Hi Tim, could you review the patch when you get a chance? It has changed a lot since your last review:

I've added a new helper class which decodes the icons from the commands. Also, I've made a refactor of the |icc.js| class in order to add and clean the icons when a dialog is displayed.

The STK menu supports  commands with icons. Of course the unit tests has been modified to fit the new requirements.

Finally you should take a look at the new CSS rules that I've used.Please you should have in mind that the dialog is going to be changed in a follow-up because the style they have is not the expected one.

Thanks!
Attachment #8545111 - Flags: review?(timdream)
See Also: → 1125046
Hi,

Bug 1125046 has been opened to cover the dialog style change. Thanks!
No longer depends on: 1117663
See Also: → 1117663
Whiteboard: [FT:COMMS] → [CR 786371][FT:COMMS]
Whiteboard: [CR 786371][FT:COMMS] → [caf priority: p2][CR 786371][FT:COMMS]
Comment on attachment 8545111 [details] [review]
Proposed patch

I didn't check the style and visual.

It's sad that there are so many conditional checks scatter in icc_worker.js to do various last minute amendments, like

-- if there is no notification text, provide a default message
-- if there is an self-explantory icon, remove the text
-- if the receiving message is a string instead of an object, use the string as the message etc.

I wonder if we could reduce the duplication be factoring out more processing helper as separate modules. I also wonder if every condition is asserted with a unit test (since there are so many) -- that however can be checked by coverage tool.
Attachment #8545111 - Flags: review?(timdream) → review+
Comment on attachment 8545111 [details] [review]
Proposed patch

Hey Tim, I took into account your comments, so I've made a helper class that contains all the comparisons that we need to do in |icc_worker|. I think this will make the code more readable. Take a look at the patch please.

Thanks!
Attachment #8545111 - Flags: feedback?(timdream)
Target Milestone: --- → 2.2 S5 (6feb)
Comment on attachment 8545111 [details] [review]
Proposed patch

Nice! Since the STKHelper is a stateless helper util, it's fine to keep it as a singleton. Just remember not to make it stateful afterward...
Attachment #8545111 - Flags: feedback?(timdream) → feedback+
Comment on attachment 8545111 [details] [review]
Proposed patch

EJ, could you help review this? Thanks.
Attachment #8545111 - Flags: review?(arthur.chen) → review?(ejchen)
Comment on attachment 8545111 [details] [review]
Proposed patch

Hi Manuel,

I did leave some comments on Github. Basically this patch looks okay to me but there are some places needed to be fixed first. In order to make you clearly understand what my comments mean, I quickly make a branch to you for reference.

Thanks !!
Attachment #8545111 - Flags: review?(ejchen)
EJ can you check my comments in github? I have some troubles with the unit tests.

Thanks!
Flags: needinfo?(ejchen)
OKay ! I just left comments about the shim on GitHub, please go check it :)

Thanks Manuel ++
Flags: needinfo?(ejchen)
Comment on attachment 8545111 [details] [review]
Proposed patch

Thanks for your quick response. I took into account your comments on github, so you can check the patch whenever you want

Thanks for your help!!
Attachment #8545111 - Flags: review?(ejchen)
Comment on attachment 8545111 [details] [review]
Proposed patch

Thanks Manuel, I think we are quite close to the target !

I just left some comments including details about what need to be fixed on GitHub, can you help me check them ? For me, this should be the last comment before giving r+.

Thanks again for your hard works !! :)
Attachment #8545111 - Flags: review?(ejchen)
Comment on attachment 8545111 [details] [review]
Proposed patch

EJ, I think everything is ok, now. Take a quick look at the patch to be sure we don't miss anything.

Thanks for your time and patience! =)
Attachment #8545111 - Flags: review?(ejchen)
Comment on attachment 8545111 [details] [review]
Proposed patch

Thanks Manuel, this patch looks nice to me ! Please make sure CI is green before merging.

r+++ :)
Attachment #8545111 - Flags: review?(ejchen) → review+
Target Milestone: 2.2 S5 (6feb) → 2.2 S6 (20feb)
EJ, after rebase the master branch, I had to solve a few conflicts. Now, the |icc_test.js| in settings is failing because the |stk_helper| file is no loaded properly. In the console I get this output:

"Error while loading: " Array [ "shared/stk_helper" ]

Do you know what could be the cause of this issue?

Thanks!
Flags: needinfo?(ejchen)
Hi Manuel,

because icc.js is still not a formal AMD module, you have to override its `require` to make sure we can pass the MockStkHelper to it.

In order not to make your works got blocked and rebased this patch for too many times, I quickly made a patch for you and please give it a try - https://gist.github.com/EragonJ/1578818c090a9f2ea77a

Hope this helps ! Please make sure CI is green before merging, thanks :)
Flags: needinfo?(ejchen)
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8545111 [details] [review]
Proposed patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): New feature
[User impact] if declined: The user will not be able to see the STK icons
[Testing completed]: Manual and unit test
[Risk to taking this patch] (and alternatives if risky): Medium
[String changes made]: None
Attachment #8545111 - Flags: approval-gaia-v2.2?
Attachment #8545111 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Needs rebasing for v2.2 uplift.
Flags: needinfo?(b.mcb)
No longer blocks: CAF-v3.0-FL-metabug
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.