1013837 - [Keyboard UX][V2.0] Need to refine UI of IME selection menu

Status: VERIFIED FIXED

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

Description

[William Hsu [:whsu]]

Attachment: 2014-05-21-15-57-11.png

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

-----------------------------------------------------------------
* Description:
We changed the UI of IME selection menu.
But, I think we still need refine it because the style is inconsistent.
Attach the UI (2014-05-21-15-57-11.png)

* Reproduction step:
 1. Enable 6 keyboard layouts on keyboard settings page
 2. Launch the browser app, and tap URL bar
 3. Long press the Globe key to show IME selection menu after keyboard pops up

* Expected result:
  (TBC)
  
* Actual result:
  (TBC)

Needinfo: Omega

Comment 1

[William Hsu [:whsu]]

Hi, Omega,

Could I have your help on the bug?
Thanks!

Comment 2

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

I think we can replace all "Built-in Keyboard" by "Mozilla" or "Firefox".

Comment 3

[William Hsu [:whsu]]

One more question, if the name of keyboard layout is longer than 27 characters, how should we handle the case? Separate into two lines?

Comment 4

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

Flagging Omega on comment 3. Also, could you attach a updated spec on comment 2, or be clear on what you want? Is it "Firefox OS: English" for example?

Preferably we should not be using any brand names since that require us localize app name differently by response to OFFICIAL flag. Pike, I can make keyboard build script amend it's own name on manifest, but I am not sure if your l10n tool for manifest strings will collect these strings. Could you suggest a plan forward?

Comment 5

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #4)
> Flagging Omega on comment 3. Also, could you attach a updated spec on
> comment 2, or be clear on what you want? Is it "Firefox OS: English" for
> example?

I mean something like:

Mozilla English
Mozilla Español
Mozilla Deutsch
...

or

Firefox English
Firefox Español
Firefox Deutsch

However, you mentioned that we shouldn't use brand names, so maybe we can use just:

English
Español
Deutsch
...

I can attach an updated spec after we have some conclusion here.

Comment 6

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

Well, it's possible we have multiple layouts labeled "English" in the list from different apps, so removing the label altogether might not be the best option.

Comment 7

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

Attachment: IME Selector Proposal.png

Here is 2 proposals for IME selector.
Also ni? Harly, we need to extend value selector to make it happen. Could you have some comments on it? Thanks!

Comment 8

[Axel Hecht [:Pike]]

I like the proposals from comment 7, personally the second better.

The proposals in comment 5 are tricky because some languages might have real grammar a construct "Firefox OS English", like "The English of Firefox OS", which could require different grammatical forms for different brandings.

Comment 9

[Harly Hsu[:harly]]

The framework team decided not to support value selector with sub-header; therefore, I would say that proposal 2 maybe the best option.

Comment 10

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

Either way, let me file another bug and see if it's possible to support make the app name "{{brandShortName}} Keyboard" in the source and compile to the right string.

Comment 11

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> Either way, let me file another bug and see if it's possible to support make
> the app name "{{brandShortName}} Keyboard" in the source and compile to the
> right string.

This is now bug 1020066 and blocks this bug.

Comment 12

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

@Rudy, Let's go for proposal 2 and follow Carol's design, thanks!

Comment 13

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

Sure, put this into my queue.

Omega, Carol, thanks.

Comment 14

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

Attachment: Patch V1.2

Since this is mostly style change, Omega, could you please help do the ui review first?
Thank you.

Comment 15

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

I modified the value selector in building block a little to move the [Check] icon out of <span> element, and put it in <label> element instead.
This will allow us to put multiple <span>s (as required by this bug) in each item of the list.

Arnau, could you please help review this part?
https://github.com/mozilla-b2g/gaia/pull/20520/files#diff-5

Thank you.

Comment 16

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

Looks good to me!
Still needs Carol's ui-review.

Comment 17

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

The BB part looks good ;)

Comment 18

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

Patch updated for,
 1. Slightly adjust the spacing between layout Name and app name, now it has 0.6rem padding.
 2. Change the background size from 2rem to 2.1rem for the checked icon, since its original size is 21 x 18.
    However, I am curious why the size is not 20 x 18, since it might have some "off-by-one-pixel" issue for @1.5x case.

Comment 19

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

Carol,

Please help to do ui-review again, sorry for not noticing the icon issue.

Comment 20

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

For building block issue, Przemek, could you help us on this question? 
========
    2. Change the background size from 2rem to 2.1rem for the checked icon, since its original size is 21 x 18.
    However, I am curious why the size is not 20 x 18, since it might have some "off-by-one-pixel" issue for @1.5x case.

Comment 21

[Carol Huang [:Carol]]

Hi Rudy,
Please help me revise the text distance (between rows) to 0.3 rem.
Thanks!

Comment 22

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

Done, please refer to the last commit of the pull request.

Comment 23

[Carol Huang [:Carol]]

Comment on attachment 8440495 [details] [review]
Patch V1.2

Hi Rudy,
The UI looks pretty good to me! thank you! :-)

Comment 24

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

Tim, please help review the system app related changes.
Thanks.

Comment 25

[Przemek Abratowski [:przemek] UX]

 Carol already responded to the question.

Comment 26

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #11)
> This is now bug 1020066 and blocks this bug.

Unblock from UI update here.

Comment 27

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

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/495bc911fc93bd6307869793d231c11d96169778

--
Tim, Carol, Omega, thanks for the review.

Comment 28

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

Comment on attachment 8440495 [details] [review]
Patch V1.2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): This is a layout/style change to accomplish feature bug 1003649.
[User impact] if declined: The keyboard layout name (including 3rd-party keyboard) might be too long, and not easy to read.
[Testing completed]: Yes, manual and with unit/integration test coverage.
[Risk to taking this patch] (and alternatives if risky): Low, minor changes to shared styles which would be applied to value selector, and some changes to IME menu layout/styles.
[String changes made]: N/A.

Comment 29

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

Uplifted to Gaia v2.0,
https://github.com/mozilla-b2g/gaia/commit/84ca0fe0a86d039f6d99cb562f52ef55045dee1d

--
Travis is green: https://travis-ci.org/mozilla-b2g/gaia/builds/28193909
TBPL result: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=ac428721154bc1b85f0f581850df1fb3e577db57
(The Gi failure is irrelevant.)

Comment 30

[William Hsu [:whsu]]

WoW! Cool. Thanks everyone!
I have verified this patch! I saw the new UI. It is better than previous version.
Attach the screenshot (2014-07-04-20-09-22.png)

* Build information:
 - Gaia      8193ad514e27440154684f112a5796e65f13fd93
 - Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/552da2802e91
 - BuildID   20140704000201
 - Version   32.0a2

Comment 31

[William Hsu [:whsu]]

Attachment: 2014-07-04-20-09-22.png

Comment 32

[Sue]

Attachment: video of issue verify

This issue has been verified successfully on Flame 2.1
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.0 versions:
Gaia-Rev        99e4594c66aa3738d58b0cb44bd885a87a063b6e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f91abc6127d9
Build-ID        20141125000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.035023
FW-Date         Tue Nov 25 03:50:34 EST 2014
Bootloader      L1TC00011880

Flame 2.1 versions:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.040617
FW-Date         Tue Nov 25 04:06:28 EST 2014
Bootloader      L1TC00011880