Closed Bug 1065961 Opened 6 years ago Closed 6 years ago
Sometimes the last alternative key would be wrongly highlighted
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.
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.
Assignee: nobody → rlu
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 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?
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 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.
** For Triage ** Nominate this as a 2.2 blocker since this patch is needed to improve the new menu UI's usability.
blocking-b2g: --- → 2.2?
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 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 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.
Triage: regression, blocking.
blocking-b2g: 2.2? → 2.2+
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.
Attachment #8487826 - Flags: review?(timdream) → feedback+
... let's *move* part of the getMenuTarget() ...
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 on attachment 8487826 [details] [review] Patch V1 Thanks!
Attachment #8487826 - Flags: review?(timdream) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [p=1] → [p=2]
Target Milestone: --- → 2.1 S5 (26sep)
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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
You need to log in before you can comment on or make changes to this bug.