Closed
Bug 1015309
Opened 11 years ago
Closed 10 years ago
Symbol UI Keyboard should follow the recommendation
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect, P1)
Firefox OS Graveyard
Gaia::Keyboard
Tracking
(b2g-v2.1 verified)
VERIFIED
FIXED
2.1 S2 (15aug)
Tracking | Status | |
---|---|---|
b2g-v2.1 | --- | verified |
People
(Reporter: raniere, Assigned: rudyl)
References
Details
(Whiteboard: [p=1])
Attachments
(2 files, 1 obsolete file)
(deleted),
text/x-github-pull-request
|
timdream
:
review+
Omega
:
ui-review+
|
Details |
(deleted),
video/mp4
|
Details |
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: '@', '#', '$', '&', '*', ...
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #8427860 -
Flags: ui-review?(ofeng)
Attachment #8427860 -
Flags: review?(rlu)
Comment 2•11 years ago
|
||
Comment on attachment 8427860 [details]
default-and-en-patch
Good job! Thanks!
Attachment #8427860 -
Flags: ui-review?(ofeng) → ui-review+
Assignee | ||
Comment 3•11 years ago
|
||
Comment on attachment 8427860 [details]
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.
Attachment #8427860 -
Flags: review?(rlu)
Flags: needinfo?(ofeng)
Comment 4•11 years ago
|
||
(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.
Flags: needinfo?(ofeng) → needinfo?(rlu)
Assignee | ||
Comment 5•11 years ago
|
||
(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.
Flags: needinfo?(rlu)
Flags: needinfo?(ra092767)
Flags: needinfo?(ofeng)
Reporter | ||
Comment 6•11 years ago
|
||
Rudy, patch update (rebase + avoid redundant symbols).
Waiting Omega reply about '{', '}', '`', '«', '»'.
Flags: needinfo?(ra092767) → needinfo?(rlu)
Comment 7•11 years ago
|
||
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.
Flags: needinfo?(ofeng)
Reporter | ||
Comment 8•11 years ago
|
||
Patch updated based on v2.1. Need review and ui-review.
Flags: needinfo?(ofeng)
Comment 9•11 years ago
|
||
(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)
Flags: needinfo?(ofeng)
Reporter | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 11•11 years ago
|
||
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.
Flags: needinfo?(rlu)
Assignee | ||
Comment 12•11 years ago
|
||
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.)
Assignee | ||
Comment 13•11 years ago
|
||
Assign to Raniere first, since he got a patch for this.
Assignee: nobody → ra092767
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Updated•11 years ago
|
Blocks: keyboard-ux-update
Assignee | ||
Comment 14•11 years ago
|
||
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.
Flags: needinfo?(ofeng)
Assignee | ||
Comment 15•11 years ago
|
||
(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.
Depends on: 1022609
Comment 16•10 years ago
|
||
(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.
Flags: needinfo?(ofeng)
Comment 17•10 years ago
|
||
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.
blocking-b2g: --- → 2.0?
Comment 18•10 years ago
|
||
Bruce - I need a product call if this is needed for 2.0 or not.
Flags: needinfo?(bhuang)
Comment 20•10 years ago
|
||
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!
blocking-b2g: 2.0? → ---
feature-b2g: --- → 2.0
Comment 21•10 years ago
|
||
This is not committed in 2.0, removing featrue-b2g tag.
feature-b2g: 2.0 → ---
Reporter | ||
Comment 22•10 years ago
|
||
I rebase and update my patch to be compatible with changes from Bug 1023729. I believe that it will need another round of review.
Flags: needinfo?(rlu)
Flags: needinfo?(ofeng)
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8427860 [details]
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.
Flags: needinfo?(rlu)
Assignee | ||
Updated•10 years ago
|
Priority: -- → P1
Comment 25•10 years ago
|
||
(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.
Assignee | ||
Comment 26•10 years ago
|
||
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.
Flags: needinfo?(ra092767)
Reporter | ||
Comment 27•10 years ago
|
||
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.
Flags: needinfo?(ra092767)
Assignee | ||
Comment 28•10 years ago
|
||
Take this since this is one of the features for v2.1 and already talked to Raniere about this.
Assignee: ra092767 → rlu
Status: ASSIGNED → NEW
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S2 (15aug)
Assignee | ||
Comment 29•10 years ago
|
||
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.
Attachment #8468405 -
Flags: ui-review?(ofeng)
Attachment #8468405 -
Flags: review?(timdream)
Assignee | ||
Updated•10 years ago
|
Attachment #8427860 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 30•10 years ago
|
||
Comment on attachment 8468405 [details]
Patch V1
Good job, thanks!
Attachment #8468405 -
Flags: ui-review?(ofeng) → ui-review+
Comment 31•10 years ago
|
||
Comment on attachment 8468405 [details]
Patch V1
Looks good except one small possible error, thanks!
Attachment #8468405 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/d06ea1bf769fce63b30e89a24d8a6fbd27b4cca3
--
Thanks for the review.
Comment 33•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•