Closed
Bug 1016807
Opened 11 years ago
Closed 10 years ago
B2G STK: Support for STK icon display (GAIA work for Bug 824145)
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(feature-b2g:2.2+, tracking-b2g:backlog, b2g-v2.2 fixed, b2g-master fixed)
RESOLVED
FIXED
2.2 S6 (20feb)
People
(Reporter: wesley_huang, Assigned: mancas)
References
Details
(Whiteboard: [caf priority: p2][CR 786371][FT:COMMS])
Attachments
(19 files, 8 obsolete files)
(deleted),
text/x-github-pull-request
|
Details | |
(deleted),
text/x-github-pull-request
|
timdream
:
review+
eragonj
:
review+
timdream
:
feedback+
bajaj
:
approval-gaia-v2.2+
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
jelee
:
ui-review+
|
Details |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
HHuang
:
ui-review+
|
Details |
(deleted),
text/x-github-pull-request
|
Details |
No description provided.
Reporter | ||
Updated•11 years ago
|
Comment 1•11 years ago
|
||
Depends on 824145, not blocks.
We cann't do the Gaia work since it is not supported by gecko ;)
Reporter | ||
Comment 3•10 years ago
|
||
Setting feature-b2g = 2.1 per FxOS V2.1 Release Planning and Feature Scope.
blocking-b2g: 2.1? → backlog
feature-b2g: --- → 2.1
Updated•10 years ago
|
Updated•10 years ago
|
QA Whiteboard: [COM=RIL]
Reporter | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
(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)
Comment 6•10 years ago
|
||
Changing NI to Jessica since she is working on the Gecko part
Flags: needinfo?(whuang) → needinfo?(jjong)
Comment 7•10 years ago
|
||
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)
Reporter | ||
Comment 8•10 years ago
|
||
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
Reporter | ||
Comment 9•10 years ago
|
||
Hi Fernando,
Are you able to take it and land before FL?
Branch day will be end of Aug.
Flags: needinfo?(frsela)
Comment 10•10 years ago
|
||
(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)
Comment 11•10 years ago
|
||
(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?
Comment 12•10 years ago
|
||
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)
Reporter | ||
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
(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)
Comment 16•10 years ago
|
||
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)
Comment 17•10 years ago
|
||
(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)
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
Comment on attachment 8478311 [details]
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)
Comment 20•10 years ago
|
||
(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 :)
Blocks: CAF-v2.2-metabug
Comment 21•10 years ago
|
||
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.
Comment 22•10 years ago
|
||
Hi Anshul.
Thank you !
Now I'm waiting for UX decission in how and were show the images. See comments #12, #16 & #17
Comment 24•10 years ago
|
||
Transfer ni to Jenny as she is the UX for Settings.
Flags: needinfo?(hhsu) → needinfo?(jelee)
Comment 25•10 years ago
|
||
(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)
Comment 26•10 years ago
|
||
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)
Comment 27•10 years ago
|
||
(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)
Updated•10 years ago
|
feature-b2g: 2.2? → 2.2+
Updated•10 years ago
|
Blocks: FxOS-v2.2-features
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(jelee)
Comment 28•10 years ago
|
||
Hi Wesley, per discussion, we will help ui-review the screens instead of providing layout spec. Thanks!
Flags: needinfo?(jelee)
Comment 29•10 years ago
|
||
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)
Updated•10 years ago
|
QA Whiteboard: [COM=RIL] → [COM=RIL][2.2-feature-qa+]
Comment 30•10 years ago
|
||
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)
Comment 31•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → b.mcb
Comment 32•10 years ago
|
||
(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
Comment 33•10 years ago
|
||
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)
Comment 34•10 years ago
|
||
(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.
Reporter | ||
Comment 35•10 years ago
|
||
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)
Comment 36•10 years ago
|
||
Hi Wesley,
We will confirm you the feasibility of that approach just after Christmas break. Thanks!
Flags: needinfo?(noemi.freiredecarlos)
Comment 37•10 years ago
|
||
Hi Wesley,
We will confirm you the feasibility of that approach just after Christmas break. Thanks!
Updated•10 years ago
|
Status: NEW → ASSIGNED
Updated•10 years ago
|
Flags: in-moztrap?(echang) → in-moztrap-
Assignee | ||
Comment 38•10 years ago
|
||
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 39•10 years ago
|
||
Comment on attachment 8545111 [details]
Proposed patch
You can land this as-is or take my recommendations, assuming this do what's intended.
Attachment #8545111 -
Flags: review?(timdream) → review+
Comment 40•10 years ago
|
||
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.
Comment 41•10 years ago
|
||
Comment 42•10 years ago
|
||
Comment 43•10 years ago
|
||
Assignee | ||
Comment 44•10 years ago
|
||
(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)
Comment 45•10 years ago
|
||
Manuel, please find a shanpshot of Android's STK menu with icon support as a reference.
Flags: needinfo?(anshulj)
Comment 46•10 years ago
|
||
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)
Assignee | ||
Comment 47•10 years ago
|
||
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)
Comment 48•10 years ago
|
||
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+
Assignee | ||
Comment 49•10 years ago
|
||
Attachment #8548155 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 50•10 years ago
|
||
Attachment #8548156 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 51•10 years ago
|
||
Attachment #8548157 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 52•10 years ago
|
||
Attachment #8548158 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 53•10 years ago
|
||
Attachment #8548159 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 54•10 years ago
|
||
Attachment #8548160 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 55•10 years ago
|
||
Attachment #8548162 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 56•10 years ago
|
||
Attachment #8548163 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 57•10 years ago
|
||
Attachment #8548164 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 58•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8545111 -
Flags: review+ → review?(timdream)
Comment 59•10 years ago
|
||
Adding ni to Anshul too, your feedback will be more than welcome!. Thanks.
Comment 60•10 years ago
|
||
Did anything changed since I review the patch last time?
Flags: needinfo?(b.mcb)
Comment 61•10 years ago
|
||
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 62•10 years ago
|
||
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 63•10 years ago
|
||
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 64•10 years ago
|
||
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 65•10 years ago
|
||
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 66•10 years ago
|
||
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 67•10 years ago
|
||
Comment on attachment 8548162 [details]
dialog_with_icon.png
Same comment with the footer.
Attachment #8548162 -
Flags: ui-review?(jelee) → ui-review-
Comment 68•10 years ago
|
||
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+
Comment 69•10 years ago
|
||
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+
Assignee | ||
Comment 70•10 years ago
|
||
(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)
Assignee | ||
Comment 71•10 years ago
|
||
(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.
Assignee | ||
Comment 72•10 years ago
|
||
(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?
Assignee | ||
Comment 73•10 years ago
|
||
(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)
Comment 74•10 years ago
|
||
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 75•10 years ago
|
||
Comment on attachment 8545111 [details]
Proposed patch
Please tell me what changed and set the review again, thanks!
Attachment #8545111 -
Flags: review?(timdream)
Comment 76•10 years ago
|
||
(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)
Comment 77•10 years ago
|
||
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)
Comment 78•10 years ago
|
||
Here is the visual spec for more information. Please let me know if any question.
Assignee | ||
Comment 79•10 years ago
|
||
Attachment #8548155 -
Attachment is obsolete: true
Flags: needinfo?(b.mcb)
Attachment #8553042 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 80•10 years ago
|
||
Attachment #8548156 -
Attachment is obsolete: true
Attachment #8553043 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 81•10 years ago
|
||
Attachment #8548157 -
Attachment is obsolete: true
Attachment #8553044 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 82•10 years ago
|
||
Attachment #8553045 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 83•10 years ago
|
||
Attachment #8548158 -
Attachment is obsolete: true
Attachment #8553047 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 84•10 years ago
|
||
Attachment #8548162 -
Attachment is obsolete: true
Attachment #8553048 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 85•10 years ago
|
||
Attachment #8548160 -
Attachment is obsolete: true
Attachment #8553049 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 86•10 years ago
|
||
Attachment #8548159 -
Attachment is obsolete: true
Attachment #8553050 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 87•10 years ago
|
||
Attachment #8548163 -
Attachment is obsolete: true
Attachment #8553051 -
Flags: ui-review?(hhuang)
Assignee | ||
Comment 88•10 years ago
|
||
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!
Updated•10 years ago
|
Attachment #8553042 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553043 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553044 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553045 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553047 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553048 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553049 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553050 -
Flags: ui-review?(hhuang) → ui-review+
Updated•10 years ago
|
Attachment #8553051 -
Flags: ui-review?(hhuang) → ui-review+
Comment 89•10 years ago
|
||
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)
Assignee | ||
Comment 90•10 years ago
|
||
Comment on attachment 8545111 [details]
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)
Updated•10 years ago
|
Comment 91•10 years ago
|
||
Hi,
Bug 1125046 has been opened to cover the dialog style change. Thanks!
Updated•10 years ago
|
Whiteboard: [FT:COMMS] → [CR 786371][FT:COMMS]
Updated•10 years ago
|
Whiteboard: [CR 786371][FT:COMMS] → [caf priority: p2][CR 786371][FT:COMMS]
Updated•10 years ago
|
Attachment #8545111 -
Flags: review?(arthur.chen)
Comment 92•10 years ago
|
||
Comment on attachment 8545111 [details]
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+
Assignee | ||
Comment 93•10 years ago
|
||
Comment on attachment 8545111 [details]
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)
Updated•10 years ago
|
Target Milestone: --- → 2.2 S5 (6feb)
Comment 94•10 years ago
|
||
Comment on attachment 8545111 [details]
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 95•10 years ago
|
||
Comment on attachment 8545111 [details]
Proposed patch
EJ, could you help review this? Thanks.
Attachment #8545111 -
Flags: review?(arthur.chen) → review?(ejchen)
Comment on attachment 8545111 [details]
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)
Assignee | ||
Comment 97•10 years ago
|
||
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)
Assignee | ||
Comment 99•10 years ago
|
||
Comment on attachment 8545111 [details]
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]
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)
Assignee | ||
Comment 101•10 years ago
|
||
Comment on attachment 8545111 [details]
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]
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+
Updated•10 years ago
|
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
Updated•10 years ago
|
Target Milestone: 2.2 S5 (6feb) → 2.2 S6 (20feb)
Assignee | ||
Comment 103•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 105•10 years ago
|
||
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/7e7569b838981ca6a24b5d170c3d16ed2d588f55
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Assignee | ||
Comment 106•10 years ago
|
||
Comment on attachment 8545111 [details]
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?
Updated•10 years ago
|
Attachment #8545111 -
Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Comment 108•10 years ago
|
||
Comment 109•10 years ago
|
||
Flags: needinfo?(b.mcb)
Blocks: CAF-v3.0-FL-metabug
No longer blocks: CAF-v3.0-FL-metabug
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•