1065961 - Sometimes the last alternative key would be wrongly highlighted

Status: VERIFIED FIXED

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

Description

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

STR
===
1. Open the alternative char menu by long pressing on "e" or "i" key.
2. Move around to make the transferred touch (moved upwards about 60px) hit the divider between each key (including vertical or hirizontal).
   => The last alternative key would be highlighted.

Comment 1

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

I think this is because I used document.elementFromPoint() to convert the touch event to the corresponding element in the menu.
However, for some reason, for some edge case, it would hit the menu container instead of the exact alternative key element, though I just made sure that there is no border/margin/padding between these alternative keys.

Comment 2

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

Attachment: Patch V1

Comment 3

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

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

[Solution]
Insert a dummy key into the menu where there will be empty space in it (i.e. the number of the key is odd and it is a 2-row menu), and transfer the touch event to the last alternative key when the user is touching the empty space.
--

Tim, could you please help review this change?
It would be easier to review with this url, since there is some indent change,
https://github.com/mozilla-b2g/gaia/pull/23946/files?w=1

Comment 4

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

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

Your math would be wrong if bug 934209 supports more than 2 lines. Did bug 934209 supports more than 2 lines?

Comment 5

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

Right now, we don't have a use case that would have more than 2 lines, so I did not test it.
But, I suppose it should work for more than 2 lines, if there is anything worth checking to make it correct, please let me know, thanks.

Comment 6

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

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

Let's try to calculate the target element from menu container box without talking to DOM API. Adopt/workaround behavior of DOM is very problematic.

Comment 7

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

** For Triage **
Nominate this as a 2.2 blocker since this patch is needed to improve the new menu UI's usability.

Comment 8

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

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

The patch has been updated to calculate the menu target by pure math.

Tim, could you please help review it again?
Thank you.

Comment 9

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

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

Clear the review flag first as offline discussion, will try to refine the logic so that dummy key is not needed.

Comment 10

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

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

The patch has been updated to use math to determine the touched menu target instead of document.elementFromPoint().

Could you please help look at it again?
Thank you.

Comment 11

[howie [:howie]]

Triage: regression, blocking.

Comment 12

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

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

Here is a radical proposal and sorry for not figuring out this for the first time: let's part of the getMenuTarget(), starting from Line 148, to AlternativesCharMenuView.

I think we need to do this because your patch expose two properties (direction and keyCountInSingleRow) from AlternativesCharMenuView to AlternativesCharMenuManager just to make the math work. Plus menuRect it's three. If we move the math to AlternativesCharMenuView, these can all be kept there and you only need to send absolute x-y from manager to get the target from view. I think this is the better task division.

Please correct me if this is wrong.

Comment 13

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

... let's *move* part of the getMenuTarget() ...

Comment 14

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

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

Patch updated to accommodate the above suggestion that moving the main calculation of menu target to the menu view.
Also add unit tests to cover this part in the menu view.

Tim, please help review it again and thanks for the suggestions.

Comment 15

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

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

Thanks!

Comment 16

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

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/495a388785355a48e45dd870e75b396ea8c414a8
(CI passed: https://tbpl.mozilla.org/?rev=0a856706a6d3c0fa98e5d8ed4523ab3fc9015808&tree=Gaia-Try)

--
Thanks for the review.

Comment 17

[[:RJ]]

issue is veryified fixed on 2.2 flame 

When in the alternate char menu is prompted the last alternative key is highlighted, and selection is held between the dividers

Flame 2.2
Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141121040204
Gaia: 25388c6bce932657ebf93adedf31881bfaf88c15
Gecko: 3366c0fcf9c2
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0