Omega Feng [:Omega] [:馮於懋] Reporter
	Description • 10 years ago

Device: Flame
Gaia: 2014-06-05 23:34:17
BuildID: 20140602160205 
Version: 2.0.0.0-prerelease

STR:
1) Tap any text input to show keyboard
2) Switch to PinYin

Actual:
Looks very different from ZhuYin in the UX spec

Expected:
Looks similar to ZhuYin in the UX spec

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 1 • 10 years ago

See 983043 for latest UX spec.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 2 • 10 years ago

See 983043 for latest UX spec.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 3 • 10 years ago

Thanks Omega. I'll ask Cedric to work on this bug.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 4 • 10 years ago

It's a major issue in new feature. Nominating as 2.0 blocking.
A consistent layout is really important for a large number of users.

	Stephany Wilkes
	Comment 5 • 10 years ago

I discussed this bug (and others like it) with Jason in IRC today. This looks like a 2.0 feature that was not correctly or fully implemented to spec, and should not ship as-is as a result (per Omega's comment #4). To that end, I am removing the blocking flag but adding the feature-b2g flag for 2.0, which it looks like this bug should have had as it was on the work list for 2.0.

If this is not correct, let me know and we can discuss making it a blocking bug if it was not indeed a feature. Thanks!

	howie [:howie]
	Comment 6 • 10 years ago

This is not committed in 2.0, removing feature-b2g tag.

	wdeng@mozilla.com Assignee
	Comment 8 • 10 years ago

Hi rudy, do you have time to review it for me?

Thanks.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 9 • 10 years ago

Let's link to the pull request for easier review and checking the CI status.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 10 • 10 years ago

Comment on attachment 8452844 [details]
Patch V1 - pull request 21425

General comment:
  1. Please help update the commit message to the following format,
     Bug 1021505 - bug description or solution.

  2. Please help add tests to apps/keyboard/test/unit/keyboard/layout_manager_test.js for needsCommaKey.

For other minor comments/questions, please help refer to github pull request.
Thanks.

--
Also need Omega to do ui review.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 11 • 10 years ago

Comment on attachment 8452844 [details]
Patch V1 - pull request 21425

Some comments:
1) Alternatives of 。 in the default panel should be 。，？！；：
2) a) switch to symbol panel
   b) switch to another IME
   c) switch back to Pinyin, and it stays in symbol panel
   -> it should go back to default panel (the qwerty one)

Also ask Carol for visual review.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 13 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Cancel review request as travis failed.

	Carol Huang [:Carol]
	Comment 14 • 10 years ago

Please adjust the position of “，"＆“。” to the right a bit, in order to make it look Visually align in the center of the key width.(see attachment PinYin_revise_visual spec_0714.jpg)
thanks!!

	wdeng@mozilla.com Assignee
	Comment 15 • 10 years ago

(In reply to Omega Feng [:Omega] [:馮於懋] from comment #11)
> Comment on attachment 8452844 [details]
> Patch V1 - pull request 21425
> 
> Some comments:
> 1) Alternatives of 。 in the default panel should be 。，？！；：
  It has been fixed.
> 2) a) switch to symbol panel
>    b) switch to another IME
>    c) switch back to Pinyin, and it stays in symbol panel
>    -> it should go back to default panel (the qwerty one)
  I will open a new bug for this one. 
> Also ask Carol for visual review.

	wdeng@mozilla.com Assignee
	Comment 16 • 10 years ago

Hi Carol,
  I will open a new bug for this problem, thanks a lot.

(In reply to Carol Huang [:Carol] from comment #14)
> Created attachment 8455134 [details]
> PinYin_revise_visual spec_0714.jpg
> 
> Please adjust the position of “，"＆“。” to the right a bit, in order to make
> it look Visually align in the center of the key width.(see attachment
> PinYin_revise_visual spec_0714.jpg)
> thanks!!

	wdeng@mozilla.com Assignee
	Comment 17 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Hi rudy,
   Pull request has been updated, do you have time review it for me? Thanks.

Hi Omega,
   I will fix the switching problem(2 in comment 11) in bug 1038516, thanks.

Hi Carol,
   I will fix the problem described in comment 14 in bug 1038518, thanks.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 18 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Got it. Please ui-review? me when your patch for bug 1038516 is ready, thanks!

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 19 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Sorry for bothering. The patch needs some polish before review and wdeng will add comments to explain the changes of the new pull request to make review easier.

	wdeng@mozilla.com Assignee
	Comment 20 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Hi rudy,
  Patch V2 is a new pull request, the former one is closed.
  What did in the new patch includes:
  a) Modify Pinyin layout according to the new UX spec.
  b) Add test to apps/keyboard/test/unit/keyboard/layout_manager_test.js for needsCommaKey.

  Thanks a lot for your patience.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 21 • 10 years ago

(In reply to wdeng@mozilla.com from comment #20)
> Comment on attachment 8453603 [details]
> Patch V2 - pull request 21569
> 
> Hi rudy,
>   Patch V2 is a new pull request, the former one is closed.
>   What did in the new patch includes:
>   a) Modify Pinyin layout according to the new UX spec.
>   b) Add test to apps/keyboard/test/unit/keyboard/layout_manager_test.js for
> needsCommaKey.
> 
>   Thanks a lot for your patience.

@Rudy, wdeng is new to gaia dev and deleted the previous pull request accidentally, so we create a new one.

The new pull request fixed all issues addressed in the previous pull request[1]:
 1. add back deleted comments as well as comment for the purpose of `needsCommaKey`.
 2. fix js code format.
 3. Update the commit message to match mozilla format:
    Bug 1021505 - Modify Pinyin keybaord to new UX
 4. Add 'needsCommaKey' tests to layout_manager_test.js.

Besides, 
 5. Alternatives of 。 in the default panel is set to 。，？！；： according to comment 11 from Omega.
 6. it changes the numeric keys(0-9) in "全" mode to full width according to design. Specifically, the numeric keys(0-9) for "zh-Hans-Pinyin-Symbol-En-2" and "zh-Hans-Pinyin-Symbol-Ch-1" keyboards are all changed to full width.

There are two issues not fixed, which are moved to follow up bugs:
 1. Bug 1038516 - Switch back to default page instead of symbol panel of Comment 11
    This is a bug not related to keyboard layout appearance and inherits from the original pinyin. We need to fix a few related tests as well, so I ask wdeng to reslove it by firing new bug.

 2. Bug 1038518 - Adjust Chinese comma and period positions visually center in the key width of  Comment 14
    The two characters of "。" and "," are center aligned in the DOM tree, but not center aligned visually due to the asymmetry of their fonts. We need some hacks to fix their position.

[1] https://github.com/mozilla-b2g/gaia/pull/21425

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 22 • 10 years ago

(In reply to Yuan Xulei [:yxl] from comment #21)
...
>  6. it changes the numeric keys(0-9) in "全" mode to full width according to
> design. Specifically, the numeric keys(0-9) for "zh-Hans-Pinyin-Symbol-En-2"
                                                   ~ It should be "zh-Hans-Pinyin-Symbol-Ch-2". Sorry for my mistake.
> and "zh-Hans-Pinyin-Symbol-Ch-1" keyboards are all changed to full width.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 23 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

The code itself looks good to me.
However, I don't understand why we change the default to show "full-width" numbers when your switch to symbol panel.
I cannot find this behavior when using Android phone.
Although I am not a pinyin user, I would think this is wrong because we use the normal numbers instead full-width ones in general, right?

I'll give a f+ here first, but let's get the above question clear before landing.
Thanks.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 24 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Carol,

Please help review this again or is comment 14 the only concern?
If that is the case, please help comment on if we could land this change first and address comment 14 in a separate bug, bug 1038518.

Thank you.

	wdeng@mozilla.com Assignee
	Comment 25 • 10 years ago

(In reply to Rudy Lu [:rudyl] from comment #23)
> Comment on attachment 8453603 [details]
> Patch V2 - pull request 21569
> 
> The code itself looks good to me.
> However, I don't understand why we change the default to show "full-width"
> numbers when your switch to symbol panel.
> I cannot find this behavior when using Android phone.
> Although I am not a pinyin user, I would think this is wrong because we use
> the normal numbers instead full-width ones in general, right?
Let Jing answer your question about this design.
> 
> I'll give a f+ here first, but let's get the above question clear before
> landing.
> Thanks.

	Carol Huang [:Carol]
	Comment 26 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

The overall layout looks good! I'll give a + here.

Please Adjust Chinese comma and period positions(全形) visually center in the key width on Bug 1038518. Thank you!!

	Jing Zhang
	Comment 27 • 10 years ago

(In reply to wdeng@mozilla.com from comment #25)
> (In reply to Rudy Lu [:rudyl] from comment #23)
> > Comment on attachment 8453603 [details]
> > Patch V2 - pull request 21569
> > 
> > The code itself looks good to me.
> > However, I don't understand why we change the default to show "full-width"
> > numbers when your switch to symbol panel.
> > I cannot find this behavior when using Android phone.
> > Although I am not a pinyin user, I would think this is wrong because we use
> > the normal numbers instead full-width ones in general, right?
> Let Jing answer your question about this design.
> > 
> > I'll give a f+ here first, but let's get the above question clear before
> > landing.
> > Thanks.

Yes, I've researched other popular PinYin IMEs (including Sogou-IME, Baidu-IME and Samsung built-in IME) , and they all use normal numbers in PinYin keyboard. So, I think it's better to use normal numbers in both '全' and '半' symbol keyboard.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 28 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Agreed with Rudy and Jing. Let's use half-width numbers in both full-width and half-width panels.
Sorry Wei, I guess the current UX spec might mislead you. I'll revise it to avoid that problem.

	wdeng@mozilla.com Assignee
	Comment 29 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

The pull request has been updated, numerics in all symbol pages are half-width now.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 30 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Looks good! +

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 31 • 10 years ago

Comment on attachment 8453603 [details]
Patch V2 - pull request 21569

Yeah, this looks pretty good to me, so r+.
Wei Deng, thanks for your work!

	wdeng@mozilla.com Assignee
	Comment 32 • 10 years ago

@Omega, @Rudy, thanks for your review.

	Carsten Book [:Tomcat]
	Comment 33 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/22adb72967204d1a25e8b480af9f43aaf2d1fc9e

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 34 • 10 years ago

\^o^/

	Sue
	Comment 35 • 10 years ago

This issue has been verified unsuccessfully on Flame v2.1
STR:
1. Launch Messages app -> new a message.
2. Tap message text box and wait Keyboard appear.
3. Tap "12&" at bottom left of keyboard.
4. Switch to another IME.
5. Switch back to Pinyin.
**It will back to default panel (the qwerty one).
6. Check the "，" and "。" of Pinyin keyboard.
**The "，" and "。" are not center aligned in the DOM tree.
Found time:15:00
See attachment: verify_video.MP4 and logcat_1500.txt
Reproducing rate: 5/5
Flame 2.1 versions:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880

	Hubert Lu[:hlu] <hlu@mozilla.com>
	Comment 37 • 10 years ago

Hi Mike,
    Per talk, please help handle v2.1 bug verified by Marigold. Thank you.

	Mike Lien[:mlien]
	Comment 38 • 10 years ago

Verified again with v2.1
Refer to comment 0 and comment 1, keyboard style is the same with UX spec
The alignment problem is already nominated by bug 1038518

Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141126161202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.193631
FW-Date         Wed Nov 26 19:36:42 EST 2014
Bootloader      L1TC10011880