Closed Bug 934209 Opened 8 years ago Closed 7 years ago

Alternate keys list overflows

Categories

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

defect

Tracking

(b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S4 (12sep)
Tracking Status
b2g-v2.2 --- fixed

People

(Reporter: ndtrung4419, Assigned: rudyl)

References

Details

(Whiteboard: [p=3])

Attachments

(6 files, 3 obsolete files)

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

Steps to reproduce:

Try out the attached patch on comment #5 at bug #934198. Compile gaia with:

`make DEVICE_DEBUG=1 GAIA_KEYBOARD_LAYOUTS=en,vi`

Enable Vietnamese keyboard in Settings. Open any text field, switch to Vietnamese keyboard, press and hold the "u" key.

https://bugzilla.mozilla.org/show_bug.cgi?id=934198#c5


Actual results:

A list of candidates for "u" shows up but it's too long and overflows.


Expected results:

The list should adapt. Either by scrolling when the touch point reaches the left/right edges or by opening a two-dimensional bubble like in Android.
Do this happen only on Vietnamese keyboard or other existing keyboards as well?
I looks like we are the only language with enough combinations to trigger the bug. But who knows, what if someone decided to create a button for emoticons, listing lots of emoticons as alternative key?
Blocks: 934198
Hey @jcarpenter What's your take on this ?
Perhaps if we reduce the size of tonal characters (Hint: ratio:x.x) then they might fit on one line. Possible work around?
This is a UX issue, so let's ask our UX designer, Carrie, to help on this topic.

Carrie,

Do you think we can have multiple-line alternative char menu?
Or what is the suggested way to handle this case?
Thanks.
Flags: needinfo?(cawang)
Hi guys,

We are currently working on keyboard design and coming out some suggestions on this issue.
We'll soon release the spec after internal review. Stay tuned!
Thanks.
Flags: needinfo?(cawang)
Hey, I just want to note that this issue can still be found in the latest gaia.
Omega, could you help take this bug for design work as this would block bug 985334?
Thanks.
Flags: needinfo?(ofeng)
Hi, we have 2 row design for this. Please see bug 983043 for the UX spec update. (p.14)
Flags: needinfo?(ofeng)
Hi Omega,

Yes, however per our previous offline meeting, we still need detailed design, especially the visual part, right?

Could you please give an estimate on when it will be ready?
Thanks.
Flags: needinfo?(ofeng)
(In reply to Rudy Lu [:rudyl] from comment #10)
> Yes, however per our previous offline meeting, we still need detailed
> design, especially the visual part, right?

Right, Carol is doing this. ni? Carol for more details.
Flags: needinfo?(ofeng) → needinfo?(chuang)
Hey Rudy,
The attachment is the Alternate keys menu visual spec for two lines.
Let me know if you have any question! thanks!!!
Flags: needinfo?(chuang) → needinfo?(rlu)
Carol,

Thanks for the updates.
The visual spec looks pretty good to me.
Flags: needinfo?(rlu)
Thanks Carol, That looks very slick.
Hi Omega,

the spec hasn't pointed out how to handle below cases,
1. how about 8 keys?

1 2 3 4 5
6 7 8 

or

1 2 3 4
5 6 7 8 

2. How about 16 keys?

1 2 3 4 5 6 7 8
9 10 11 12 13 14 15 16 

or 

1 2 3 4 5 6
7 8 9 10 11 12
13 14 15 16

Please identify when to split to multiple lines and maximum num of lines.
Thanks.
Flags: needinfo?(ofeng)
Hi George,
Please see bug 983043 for UX spec update.
Flags: needinfo?(ofeng) → needinfo?(gduan)
(In reply to Omega Feng [:Omega] from comment #16)
> Hi George,
> Please see bug 983043 for UX spec update.

It's in P.14.
Hi Rudy,
There's some little changes on the Alternate Keys menu.
1. Make the menu corner to 1px.(both 1 line & 2 lines situation)
2. Change the size on "single overflow menu width" to 31px.

I've updated the Visual spec. Please let me know if you have any question! thanks!
Attachment #8420750 - Attachment is obsolete: true
Flags: needinfo?(rlu)
I am fine with this visual update.
Carol, thanks for the work.

George, please be informed of this visual update if you're still working on this.
Thanks.
Flags: needinfo?(rlu)
Hi Rudy.

Update Alternate keys list menu Radius size 
2 lines menu:2px
1 key & 1 line:  will keep it rounded

thank you!
Attachment #8430130 - Attachment is obsolete: true
Flags: needinfo?(rlu)
Got it.
Thanks for the visual update.
Flags: needinfo?(rlu)
Rudy, do you know someone that can help me with the CSS?
Flags: needinfo?(rlu)
Sorry but I am not sure who is going to help on this, maybe George?
Flags: needinfo?(rlu)
Attached image patch-screenshot.png (obsolete) —
This is a screenshot of https://github.com/r-gaia-cs/gaia/commit/1be73ad92a12ec5ce53c47f371bfa30bd5fb320e.

Omega, about "Condition when two row candidates come in odd", the empty space should be at the right even when the key is at the right side of the keyboard?
Flags: needinfo?(ofeng)
Rudy, could you take a look at my patch in progress?
Flags: needinfo?(rlu)
(In reply to Raniere Silva from comment #24)
> Omega, about "Condition when two row candidates come in odd", the empty
> space should be at the right even when the key is at the right side of the
> keyboard?

Raniere, about "Condition when two row candidates come in odd", the empty space should be at the left when the key is at the right side of the keyboard.
Flags: needinfo?(ofeng)
I think it's too early to give any feedback on this patch, but please be informed,
 1. This is not only a visual refresh patch, it also involves interaction part, since now the alternatives menu contains 2 rows.
 2. We should avoid put inline CSS styles in JS code, for the complexity of this requirement, maybe we would define a set of rules for each combinations.
Flags: needinfo?(rlu)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Duplicate of this bug: 1032247
Assignee: nobody → rlu
Target Milestone: --- → 2.1 S1 (1aug)
Status: NEW → ASSIGNED
Flags: needinfo?(gduan)
Whiteboard: [p=2]
Rudy,

I make some changes at my patch. I still thinking the best way to replace CSS rule for first-child and last-child.

> 1. This is not only a visual refresh patch, it also involves interaction part, since now the alternatives menu contains 2 rows.

Could you be more clear about the changes involving interaction?
Flags: needinfo?(rlu)
Omega, do you know some one that can help me with the CSS?
Attachment #8448041 - Attachment is obsolete: true
Flags: needinfo?(ofeng)
> Omega, do you know some one that can help me with the CSS?

The only guy who can help as I know is Rudy. :D
Flags: needinfo?(ofeng)
(In reply to Raniere Silva from comment #29)
> Rudy,
> 
> I make some changes at my patch. I still thinking the best way to replace
> CSS rule for first-child and last-child.
> 
> > 1. This is not only a visual refresh patch, it also involves interaction part, since now the alternatives menu contains 2 rows.
> 
> Could you be more clear about the changes involving interaction?

I mean the first section of page 13 of the spec,
https://bug983043.bugzilla.mozilla.org/attachment.cgi?id=8454327#page=13

But as our offline discussion in the email, I'll look into this issue, so don't worry about it.
:)
Flags: needinfo?(rlu)
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Attached file WIP
WIP for this requirement.
There are some items to be addressed, I think it is ready to get some feedback from code and UI side.

Please help take a look.

ToDo:
UI part:
 1. Might need some tweaks for landscape mode.
 2. Need to check with VD about the top positioning of the menu, right now I just keep "1-row" menu the same as before, "2-row" would the same as the spec.
 3. The horizontal position might need some tweaks as well, especially for the first/last key.

Code level:
 1. Unit test are not addressed yet, and this might break some integration test, I suppose.
Attachment #8477271 - Flags: ui-review?(ofeng)
Attachment #8477271 - Flags: ui-review?(chuang)
Attachment #8477271 - Flags: feedback?(timdream)
Comment on attachment 8477271 [details] [review]
WIP

Two comments:

1) There is no buffer for touch area in this patch
   It's better to have some buffer like before:

 #6  #7  #8  #9
[#1] #2  #3  #4  #5
╭──╮
│  │

 #6  #7  #8  #9
 #1  #2  #3  #4 [#5]
                ╭──╮
                │  │

 #6  #7  #8  #9
 #1  #2  #3  #4 [#5]
                    ╭──╮
                    │  │

2) Similar to 1), the second row should have buffer as well.
   
 #6  #7  #8  #9
[#1] #2  #3  #4  #5
╭──╮
│  │

 #6  #7  #8  #9
 #1  #2  #3  #4 [#5]
                ╭──╮
                │  │

 #6  #7  #8 [#9]
 #1  #2  #3  #4 ╭──╮
                │  │
Attachment #8477271 - Flags: ui-review?(ofeng) → ui-review-
Comment on attachment 8477271 [details] [review]
WIP

The patch has been updated: 
 1. To address the comments around the interaction details.
 2. The positioning in landscape mode.

Omega, mind to look at it again?
Thanks.
Attachment #8477271 - Flags: ui-review- → ui-review?(ofeng)
Comment on attachment 8477271 [details] [review]
WIP

Good job! Thanks! :)
Attachment #8477271 - Flags: ui-review?(ofeng) → ui-review+
Hi Rudy,
For the position alternate key menu, please leave 0.4rem space if it's on the sides (see attachment). Thanks!
Flags: needinfo?(rlu)
Comment on attachment 8477271 [details] [review]
WIP

This looks good and thank you for spending time to give some thought on the architectures. So as we have discussed offline, I still some concerns between the separations between the new module and the current AlternativesCharMenuManager. We can definitely discuss more in the next iteration.
Attachment #8477271 - Flags: feedback?(timdream) → feedback+
Comment on attachment 8477271 [details] [review]
WIP

looks good!
But there's few minor improvement we need to do.

Let's file bugs for that later on.
Thank you, Rudy!
Attachment #8477271 - Flags: ui-review?(chuang) → ui-review+
Comment on attachment 8477271 [details] [review]
WIP

Patched updated to address both code and UI review comments,
   Code
     - Update the naming of menu view.
     - Refine the menu related instances that manager holds.
     - Fix integration test and unit tests.

    UI:
    To address UI review comment for edge case.
    Other tweaks according to offline discussion.

Tim, could you help review this?
Thank you.
Attachment #8477271 - Flags: review?(timdream)
Flags: needinfo?(rlu)
Comment on attachment 8477271 [details] [review]
WIP

r=me after offline discussion.

So eventually I think we should still absorb all coordinate calculations (i.e. isMenuTarget() and getMenuTarget()) to view classes, but I agree we not to do it in this patch because since that means we need more code in render.js and it's harder to harden the code there.
Attachment #8477271 - Flags: review?(timdream) → review+
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/7592d02b61490fe09f5cf4279b9441f7fb2a139a

--
Thank you all for the review.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [p=2] → [p=3]
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
Depends on: 1062571
feature-b2g: --- → 2.2+
Remove the feature b2g tag to match the new v2.2 scope.
feature-b2g: 2.2+ → ---
You need to log in before you can comment on or make changes to this bug.