1015309 - Symbol UI Keyboard should follow the recommendation

Status: VERIFIED FIXED

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

Description

[Raniere Silva]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140518030203

Steps to reproduce:

1. Start any app that has input (type: text)
2. Select the input field
3. Go to Symbol UI


Actual results:

It shows the old layout. Second line has: '@', "#', '$', '%', '*', ...


Expected results:

It shows the new layout (v1.9). Second line has: '@', '#', '$', '&', '*', ...

Comment 1

[Raniere Silva]

Attachment: default-and-en-patch

Comment 2

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

Comment on attachment 8427860 [details] [review]
default-and-en-patch

Good job! Thanks!

Comment 3

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

Comment on attachment 8427860 [details] [review]
default-and-en-patch

Sorry for the delay to review this patch, since I was thinking about if it is ok that we modified only the default symbol layout and the symbol ayout for English, while causing this to be inconsistent with those in Spanish and Portuguese.
It seems Omega agree that we could do this change without considering Portuguese and Spanish case.

However, I have to clear the review flag because:
1. The following keys would be unavailable per the new design. 
'{', '}', '`', '«', '»'

   Omega, could you please double confirm this is acceptable?


2. '_' would appear twice for input with type="email".
   This would also need our UX to provide an appropriate value for this case.

Comment 4

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

(In reply to Rudy Lu [:rudyl] from comment #3)
> Sorry for the delay to review this patch, since I was thinking about if it
> is ok that we modified only the default symbol layout and the symbol ayout
> for English, while causing this to be inconsistent with those in Spanish and
> Portuguese.
> It seems Omega agree that we could do this change without considering
> Portuguese and Spanish case.
Yes, let's do it on English for now.

> However, I have to clear the review flag because:
> 1. The following keys would be unavailable per the new design. 
> '{', '}', '`', '«', '»'
> 
>    Omega, could you please double confirm this is acceptable?
Yes, it's acceptable.

> 2. '_' would appear twice for input with type="email".
>    This would also need our UX to provide an appropriate value for this case.
I think you mean the bottom row's "@" for "email" and "/" for "url".
After internal discussion, we decide use "@" for "email" bottom row only in the default page, not in symbol pages. (similar to url) So there are no redundant symbols.

Comment 5

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

(In reply to Omega Feng [:Omega] from comment #4)
> (In reply to Rudy Lu [:rudyl] from comment #3)
> > Sorry for the delay to review this patch, since I was thinking about if it
> > is ok that we modified only the default symbol layout and the symbol ayout
> > for English, while causing this to be inconsistent with those in Spanish and
> > Portuguese.
> > It seems Omega agree that we could do this change without considering
> > Portuguese and Spanish case.
> Yes, let's do it on English for now.
> 
> > However, I have to clear the review flag because:
> > 1. The following keys would be unavailable per the new design. 
> > '{', '}', '`', '«', '»'
> > 
> >    Omega, could you please double confirm this is acceptable?
> Yes, it's acceptable.

Hi Omega,
I don't agree we should take this approach.
How could a user input '{' if he/she really needs it?
These symbols would appear on a standard keyboard, so we should cover this, or I am afraid we're going to have regression bugs filed for this change.

> 
> > 2. '_' would appear twice for input with type="email".
> >    This would also need our UX to provide an appropriate value for this case.
> I think you mean the bottom row's "@" for "email" and "/" for "url".
> After internal discussion, we decide use "@" for "email" bottom row only in
> the default page, not in symbol pages. (similar to url) So there are no
> redundant symbols.

I see, Raniere, could you address this in this patch?
Thanks.

Comment 6

[Raniere Silva]

Rudy, patch update (rebase + avoid redundant symbols).

Waiting Omega reply about '{', '}', '`', '«', '»'.

Comment 7

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

See bug 983043 for the latest UX spec.
I've added '{', '}' and '`' back.

Since ² and ³ are removed, maybe we can add ⁰ - ⁹ as alternatives of number 0 - 9 keys.

Comment 8

[Raniere Silva]

Patch updated based on v2.1. Need review and ui-review.

Comment 9

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

(In reply to Raniere Silva from comment #8)
> Patch updated based on v2.1. Need review and ui-review.

UI seems OK.
Here are some comments:
1) The alternatives of '$' should be like p.12 of spec.
2) It still needs two different layouts for single or multiple IMEs. (maybe we should file another bug for this)

Comment 10

[Raniere Silva]

Patch updated one more time.

> 1) The alternatives of '$' should be like p.12 of spec.

Fixed.

> 2) It still needs two different layouts for single or multiple IMEs. (maybe we should file another bug for this)

+1 to file another bug. The dev team should talk the best way to implement this.

Comment 11

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

I am not sure with comment 4, if we need to also remove the following way we took for not showing '@' at the symbol panel.
https://github.com/mozilla-b2g/gaia/blob/24fe85863d7828e9414ee1ed72454d92a301dbe1/apps/keyboard/js/layouts/es.js#L69

Will talk about this with Omega tomorrow.

Comment 12

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

I also left some comments, please help address those comments and help take care of the Travis CI, and then flag me to review again.
Thank you.

--
Sometimes, you may need to rerun one of the Travis job if it failed due to intermittent failure or other unexpected failure. (Need to log on to Travis first with your github account.)

Comment 13

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

Assign to Raniere first, since he got a patch for this.

Comment 14

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

I didn't have a chance to talk about this with Omega yet.
--

Omega, could you please help take a look at comment 11?
We modify the symbol order of Spanish and Portuguese layout to avoid showing redundant symbols like '@' for email?
Do you think we should remove that behavior because of this new design?
We can talk about this if you need more details. Thanks.

Comment 15

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

(In reply to Raniere Silva from comment #10)
> Patch updated one more time.
> 
> > 1) The alternatives of '$' should be like p.12 of spec.
> 
> Fixed.
> 
> > 2) It still needs two different layouts for single or multiple IMEs. (maybe we should file another bug for this)
> 
> +1 to file another bug. The dev team should talk the best way to implement
> this.

Bug 1022609 filed to track this issue.

Comment 16

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

(In reply to Rudy Lu [:rudyl] from comment #14)
> Omega, could you please help take a look at comment 11?
> We modify the symbol order of Spanish and Portuguese layout to avoid showing
> redundant symbols like '@' for email?
> Do you think we should remove that behavior because of this new design?
> We can talk about this if you need more details. Thanks.

We can use the same rationale as English, in email input:
1) In alphabet panel, we have '@' at the bottom row.
2) In symbol panels, we don't have '@' at the bottom row, and we use the same layout as text input.

Comment 17

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

It's a major issue in new feature. Nominating as 2.0 blocking.
Symbol layout redesign are really important for a large number of users.

Comment 18

[Jason Smith [:jsmith]]

Bruce - I need a product call if this is needed for 2.0 or not.

Comment 19

[Jason Smith [:jsmith]]

Going to followup offline on this.

Comment 20

[Stephany Wilkes]

I discussed with Jason in IRC today. This looks like a 2.0 feature that was not correctly or fully implemented, and should not ship as-is as a result (per Omega's comment #17). 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!

Comment 21

[howie [:howie]]

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

Comment 22

[Raniere Silva]

I rebase and update my patch to be compatible with changes from Bug 1023729. I believe that it will need another round of review.

Comment 23

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

Looks good from UX side.

Comment 24

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

Comment on attachment 8427860 [details] [review]
default-and-en-patch

I don't see comment 16 addressed,
https://bugzilla.mozilla.org/show_bug.cgi?id=1015309#c16

so, there will 2 '@' symbols on symbol panel, which will be a regression.

Could you please help take a look?

--
BTW, I think we already have several rounds of reviews back and forth, which made me think maybe we should consider adding integration test as well, instead of manually checking all the symbols are there and without duplicates.

Comment 25

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

(In reply to Rudy Lu [:rudyl] from comment #24)
> BTW, I think we already have several rounds of reviews back and forth, which
> made me think maybe we should consider adding integration test as well,
> instead of manually checking all the symbols are there and without
> duplicates.

It's non-trivial to add integration tests to check "duplicate" symbols... we should not block feature work on it.

Comment 26

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

Raniere, may I know if you're still working on this?

Because this bug and bug 1022609 are in our feature plan for v2.1, do you mind I take these 2 bugs if you're not going to have time to work on them in a short time?
Thanks.

Comment 27

[Raniere Silva]

Rudy, I was trying (without success) to implement the * tests for this and bug 1022609.

Today I will rebase my patch for this and for bug 1022609 and if you need you can try call me at IRC a one or two hours before your work hours tomorrow since I'm based on Brazil.

Comment 28

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

Take this since this is one of the features for v2.1 and already talked to Raniere about this.

Comment 29

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

Attachment: Patch V1

The patch includes the following 2 changes,
1. Update the layout definition per the spec.
2. Don't add special chars at symbol panels for type=email, url.

Tim, Omega, could you please help review this?
Thank you.

Comment 30

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

Comment on attachment 8468405 [details] [review]
Patch V1

Good job, thanks!

Comment 31

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

Comment on attachment 8468405 [details] [review]
Patch V1

Looks good except one small possible error, thanks!

Comment 32

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

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

--
Thanks for the review.

Comment 33

[Sue]

Attachment: video of issue verify

This issue has been verified successfully on Flame v2.1
STR:
1. Start Mail app
2. Select the input field "Your name"
3. Go to Symbol UI
**The second line has: '@', '#', '$', '&', '*', ...
See attachment: verify_video.MP4
Reproducing rate: 0/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