1003649 - [Keyboard UX update] Change IME selector to use value selector style instead of action menu style

Status: VERIFIED FIXED

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

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

+++ This bug was initially created as a clone of Bug #985851 +++

Right now, the IME selector use action menu style, which does not support 2 buttons at the bottom, so we might need to change that to value selector for 2 reasons,
 1. Conform to the spec, after discussed with Omega.
 2. To fulfill the requirement of bug 985851.

Comment 1

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

To double confirm, I'd like to consult building block expert on how we can customize action menu.

Arnau,

Do you know if it is possible to show 2 buttons (within the same row) at the bottom of action menu?
This is what we need for the keyboard layout menu: p8 of attachment8408812 [details].
Thanks.

Comment 2

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Unfortunately Action menu cannot use 2 buttons the way we have structured the HTML.
Neither is Value selector:
http://buildingfirefoxos.com/building-blocks/value-selector.html

Value selector needs to steps to trigger an option:
1. Select option.
2. Tap "Select button".

So in case you want to implement that screen using a Value selector you would need 3 buttons: Select, cancel and settings.

As Action menu could be difficult to teak, I would suggest two options:
1- Use action menu with a setting icon to the right of the header.
2- Change the behavior of value selector, so the action is triggered on tap (without having to go to "select" afterwards)

Comment 3

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Arnau March  [:arnau] from comment #2)
> As Action menu could be difficult to teak, I would suggest two options:
> 1- Use action menu with a setting icon to the right of the header.
> 2- Change the behavior of value selector, so the action is triggered on tap
> (without having to go to "select" afterwards)

I prefer using Value Selector, since IME selection is for selecting a value.
And yeah, it will be great if Value Selector can change something:
1) If it's a single-selection <select> tag, tap an item to select and leave, and the bottom button is Cancel.
2) The bottom button can be customized, like multiple buttons and customizable button behaviors.

If the above changes cannot be done now, we use Action Menu with settings icon at top-right header.

Comment 4

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Arnau, Omega,

Thanks for your suggestion.
I'll try to go with option 1 in Comment 3, that is using a customized "value selector" style menu for our IME selection menu, which would have 2 buttons, [Cancel] and [Settings].

Note that this won't change the system value selector behavior.

Comment 5

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1 - pull request 19129

I think this is ready to review.
In this patch, I created a new class ImeMenu here since we need a value-selector style for our IME selection menu instead of using the original ActionMenu.

Tim, could you help review this?

--
Omega, please help on the UX review.

Comment 6

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8420565 [details] [review]
Patch V1 - pull request 19129

Hi Rudy,
Looks nice! However I found something worth to take look at:
1) Title of value selector: "Layout selection"
   It's better to use "Select"
2) Our Built-in IME strings are too long
   They cause 2 rows in value selector Maybe we can file another bug to discuss this.
3) When tapping "Settings" in value selector first time, it goes to "Settings" and then goes to "Keyboards"
   It should go to "Keyboards" directly.
4) In "Settings > Keyboards", the top-left icon is "X" (it's correct). Tap "Built-in Keyboard" -> Tap "<" to go back -> the "X" in "Settings > Keyboards" change to "<" -> KO
   It should keep "X".

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Omega,

Thanks for your feedback.

(In reply to Omega Feng [:Omega] from comment #6)
> Comment on attachment 8420565 [details] [review]
> Patch V1 - pull request 19129
> 
> Hi Rudy,
> Looks nice! However I found something worth to take look at:
> 1) Title of value selector: "Layout selection"
>    It's better to use "Select"

This should be easy to modify.

> 2) Our Built-in IME strings are too long
>    They cause 2 rows in value selector Maybe we can file another bug to
> discuss this.

Yeah, agree. The long name of "Built-in keyboard" did cause some problems before, and we should come out with a better name. Hope to handle this with an separate bug.

> 3) When tapping "Settings" in value selector first time, it goes to
> "Settings" and then goes to "Keyboards"
>    It should go to "Keyboards" directly.

I guess this is by the current design of settings, and we will tackle this issue after settings app refactoring.

> 4) In "Settings > Keyboards", the top-left icon is "X" (it's correct). Tap
> "Built-in Keyboard" -> Tap "<" to go back -> the "X" in "Settings >
> Keyboards" change to "<" -> KO
>    It should keep "X".

And this one should be related to bug 1005827.
Will track this bug as a dependency of the user story bug.

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Arthur,

For comment 6 point 3,
  3) When tapping "Settings" in value selector first time, it goes to "Settings" and then goes to "Keyboards"

Do we have a bug to depend on?

Thanks.

Comment 9

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

Yes, we have https://bugzilla.mozilla.org/show_bug.cgi?id=1007600 tracking this issue.

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Arthur, thanks.

The above 2 issues have been set at dependencies as follows,
https://bugzilla.mozilla.org/show_bug.cgi?id=985851#c4

Comment 11

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

Comment on attachment 8420565 [details] [review]
Patch V1 - pull request 19129

Please fix feedback from Omega before landing.

Comment 12

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Omega, do you think we could land this without the settings button, with 1) addressed?
If yes, I would send an updated pull request for this.

Thank you.

Comment 13

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Rudy Lu [:rudyl] from comment #12)
> Omega, do you think we could land this without the settings button, with 1)
> addressed?
> If yes, I would send an updated pull request for this.

Yeah, we can land it for now, and add the Settings button back when it's ready.

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1 - without the settings button

This is the version that does not contain the settings button.

Omega,

Could you please confirm this is what we want for now?
Thanks.

Comment 15

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8423710 [details] [review]
Patch V1 - without the settings button

ui-review+, but it seems to crash easily on my Helix. Don't know why... You can check my phone if you'd like to.

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8423710 [details] [review]
Patch V1 - without the settings button

Carry forward the r+.
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/3afc852b8579d22072919408f6c2e39b167590c8

--
Could not repro the phone crash issue at my side, so seems irrelevant to this change.

Comment 17

[William Hsu [:whsu]]

I have verified this patch.
But I think we need to adjust the value selector UI.
Attach the screenshot. (2014-05-21-15-57-11.png)

* Build information:
 - Gaia      c462d9183d294a2d8ecc472f593ea8cfa15bc9de
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/9d8d16695f6a
 - BuildID   20140520160203
 - Version   32.0a1

Comment 18

[William Hsu [:whsu]]

Remaining work -> Bug 1013837