1009254 - [STK] No option on UI to terminate/abort the proactive session

Status: RESOLVED FIXED

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

Description

[Carol Yang [:cyang]]

Currently, for a GET_INKEY command, the options of 'Help', 'Back', and 'OK' are available on the UI. This looks like a regression as there used to be a 'Close' button for GET_INKEY to indicate that the proactive session is to be terminated.

Comment 1

[Carol Yang [:cyang]]

Hey Fernando, thought you were the best person to look at this.

Comment 2

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

Hi Carol, I'll check this

So it's supposed to show only a close button for INKEY operations

Comment 3

[Nivi]

Hi Fernando,

No it should show all other buttons along with close button.

The spec says a user can 

1. terminate the session (Close button)
2. Go back (back button)
3. Enter a digit/text that will be passed by a  terminal response (on clicking OK button)
4. Help button should be displayed and enabled if help is requested. Right now, if help is not requested, the UI shows the help button but greys it out. I am not very sure about this behavior. We could either show the Help button or not show it at all. Maybe Carol can add to this.

Thanks,
Nivi.

Comment 4

[Carol Yang [:cyang]]

(In reply to Nivi from comment #3)
> Hi Fernando,
> 
> No it should show all other buttons along with close button.
> 
> The spec says a user can 
> 
> 1. terminate the session (Close button)
> 2. Go back (back button)
> 3. Enter a digit/text that will be passed by a  terminal response (on
> clicking OK button)

Agree with Nivi here. These buttons should all be made available for a GET_INKEY proactive command.

> 4. Help button should be displayed and enabled if help is requested. Right
> now, if help is not requested, the UI shows the help button but greys it
> out. I am not very sure about this behavior. We could either show the Help
> button or not show it at all. Maybe Carol can add to this.

The Help button depends on the command details byte. If specified by the command details byte, "isHelpAvailable:true" will be sent. When this is not sent, the 'Help' button can either be removed completely or grayed out. IMO, that's a Gaia decision.

> 
> Thanks,
> Nivi.

Comment 5

[Jason Smith [:jsmith]]

Carol - Has this present on past releases? What's the partner-specific need for 1.4?

Comment 6

[Carol Yang [:cyang]]

(In reply to Jason Smith [:jsmith] from comment #5)
> Carol - Has this present on past releases? What's the partner-specific need
> for 1.4?

This is to the tech spec and was added since v1 release. It has since regressed. For your reference, bug 826508 added the 'Close' button in the first place.

Comment 7

[Preeti Raghunath(:Preeti)]

(In reply to Carol Yang [:cyang] from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > Carol - Has this present on past releases? What's the partner-specific need
> > for 1.4?
> 
> This is to the tech spec and was added since v1 release. It has since
> regressed. For your reference, bug 826508 added the 'Close' button in the
> first place.

Carol,

Can we please have the regression window?

Comment 8

[howie [:howie]]

Assign to Fernando for now, thanks.

Comment 9

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

Before implement the path I want to be sure we are on same page:

Currently, INKEY can show 3 or 4 buttons as codec here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L1142
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/icc.js#L397

  HELP, BACK and OK

or if the option isYesNoRequired is received

  HELP, BACK, NO and YES

So what we want is to add a new CLOSE bottom leaving it as:

  HELP, CLOSE, BACK and OK

or if isYesNoRequired:

  HELP, CLOSE, BACK, NO and YES

Is this correct? Should we put 5 buttons?

Comment 10

[Carol Yang [:cyang]]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #9)
> Before implement the path I want to be sure we are on same page:
> 
> Currently, INKEY can show 3 or 4 buttons as codec here:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L1142
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/icc.js#L397
> 
>   HELP, BACK and OK
> 
> or if the option isYesNoRequired is received
> 
>   HELP, BACK, NO and YES
> 
> So what we want is to add a new CLOSE bottom leaving it as:
> 
>   HELP, CLOSE, BACK and OK
> 
> or if isYesNoRequired:
> 
>   HELP, CLOSE, BACK, NO and YES
> 
> Is this correct? Should we put 5 buttons?

Thanks for explicitly listing out the options! At the minimum, the CLOSE and BACK button will be displayed as these are options always available to the user during a GET_INKEY proactive session.

To answer your question on 5 buttons, yes, it does look like there could be a scenario in which all 5 buttons need to be displayed (when command details byte is 0x0C). This is based on info from section 12.6 of 3GPP2 TS 11.14

 GET INKEY
 bit 1: 0 = digits (0 9, *, # and +) only
        1 = alphabet set;
 bit 2: 0 = SMS default alphabet
        1 = UCS2 alphabet
 bit 3: 0 = character sets defined by bit 1 and bit 2 are enabled
        1 = character sets defined by bit 1 and bit 2 are disabled and the "Yes/No" response is requested
 bits 4-7: = RFU
 bit 8: 0 = no help information available
        1 = help information available

I'm not quite sure how you'll want to fit 5 buttons on the screen but it does look like there is a use case for that.

Comment 11

[Preeti Raghunath(:Preeti)]

Fernando,

Can you provide next steps?

Comment 12

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

(In reply to Preeti Raghunath(:Preeti) from comment #11)
> Fernando,
> 
> Can you provide next steps?

Working on it today. I've a patch which I'm testing and adjusting CSS for 4 and 5 buttons.

As soon as I tested all use cases I'll upload it :)

Comment 13

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

Attachment: Patch for master branch

Comment 14

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

Attachment: Patch for 1.4 branch

Comment 15

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

Attachment: INPUT_4_BTN.png

New STK INPUT dialog with 4 buttons

Comment 16

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

Attachment: INPUT_5_BTN.png

New STK INPUT dialog with 5 buttons

Comment 17

[Preeti Raghunath(:Preeti)]

Fernando,

Is the patch ready for review?

Comment 18

[howie [:howie]]

Hi Fernando, could you put ETA on this CS blocker? thanks.

Comment 19

[Carol Yang [:cyang]]

Comment on attachment 8424699 [details] [review]
Patch for 1.4 branch

Hi Fernando,

Everything looks good except that pressing the Help button does nothing. Like the other buttons, it should send the corresponding terminal response down. In this case, it should send '13' for Help information required by the user.

Thanks,
Carol

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8424699 [details] [review]
Patch for 1.4 branch

I guess I will do the review when the patch fix the issue mentioned :)

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8424698 [details] [review]
Patch for master branch

Same comment as the 1.4 version.

Comment 22

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

(In reply to Carol Yang [:cyang] from comment #19)
> Comment on attachment 8424699 [details] [review]
> Patch for 1.4 branch
> 
> Hi Fernando,
> 
> Everything looks good except that pressing the Help button does nothing.
> Like the other buttons, it should send the corresponding terminal response
> down. In this case, it should send '13' for Help information required by the
> user.
> 
> Thanks,
> Carol

Hi Carol,

I tested into my device and it's working well:

I/GeckoDump(  622): [system] STK sendStkResponse -- # response = {"resultCode":19}

The call is made here:
https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/system/js/icc.js#L509

Can you provide some log traces?

Comment 23

[Carol Yang [:cyang]]

> Hi Carol,
> 
> I tested into my device and it's working well:
> 
> I/GeckoDump(  622): [system] STK sendStkResponse -- # response =
> {"resultCode":19}
> 
> The call is made here:
> https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/system/js/icc.js#L509
> 
> Can you provide some log traces?

You're right, looks like I was missing bug 1009188 in my build. The help info button now generates the correct TR. This patch looks good!

Comment 24

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

(In reply to Carol Yang [:cyang] from comment #23)
>
> You're right, looks like I was missing bug 1009188 in my build. The help
> info button now generates the correct TR. This patch looks good!

Ok, good :) - Thank you

Adding 1009188 as a dependency to avoid this error in the future ;)

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8424701 [details]
INPUT_5_BTN.png

I suspect the labels will not fit for some locales. Flagging UX for comment.

Comment 27

[Anshul]

Preeti, could you please get this patch reviewed; we need this fix soon.

Comment 28

[Preeti Raghunath(:Preeti)]

Anshul,

Trying to get UX review.

Comment 30

[Stephany Wilkes]

Comment on attachment 8424701 [details]
INPUT_5_BTN.png

Flagging Omega on ui-review?. Omega, I do not have Jenny's name or email; please flag her as needed here as well.

Comment 31

[Jack Liu]

Hi Vance,

As you know this is PTCRB blocker, would you please push landing patch to 1.3.

Thanks.

Comment 32

[Jenny Lee]

Comment on attachment 8424701 [details]
INPUT_5_BTN.png

Hello all,
Fitting 5 buttons on footer is not very ideal. It could cause truncation (for other languages) and it requires more effort for user to read through the options.

Comment 33

[Jenny Lee]

Attachment: STK layout v1.0.pdf

See attachment for proposed layout redesign. We moved "back" to upper left and "help" to upper right (as how we positioned "back" and "action" in our apps), this gives our users a more familiar layout that is easier to find what they want actions to take next. Header text and page content shown in the document varies as the context changes, the ones you see are only for demonstration purpose. Thanks =) !!

Comment 34

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

Thank you Vivien.

I'll upload a new commit with the new layout.

Should I add to this patch? or you consider that is better to open a new bug for the new layout ...

Comment 35

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

Attachment: ICC Alert new layout

Comment 36

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

Attachment: ICC Call message new layout

Comment 37

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

Attachment: ICC Confirm new layout

Comment 38

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

Attachment: ICC Input new layout

Comment 39

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

Attachment: ICC Input (Yes/No) new layout

Comment 40

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

Attachment: ICC New layout

Comment 41

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

Attachment: icc_newlayout.patch

Comment 42

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This patch solve the issue. The other patch is to make it looks better, which makes sense. 
But I would prefer to have a separate bug for the other, this way if we need to back out the new layout the underlying issue is still fixed.

Comment 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8429271 [details] [diff] [review]
icc_newlayout.patch

Let's discuss that into a separate bug.

Comment 44

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

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #43)
> Comment on attachment 8429271 [details] [diff] [review]
> icc_newlayout.patch
> 
> Let's discuss that into a separate bug.

Fully agree !
Thanks Vivien

Comment 45

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

New layout will be tracked here: https://bugzilla.mozilla.org/show_bug.cgi?id=1016368

Comment 46

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

Landed in master: https://github.com/mozilla-b2g/gaia/commit/eee24fa282be8e652b37369b30d55fea0a24c691
Landed in v1.4: https://github.com/mozilla-b2g/gaia/commit/6d415460490c8b8aca8f42460bb37b251ece2a47

Comment 47

[Yeojin Chung [:YeojinC]]

Unable to create a test case. This bug involves STK query.