Closed
Bug 1065961
Opened 10 years ago
Closed 10 years ago
Sometimes the last alternative key would be wrongly highlighted
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect)
Tracking
(blocking-b2g:2.2+, b2g-v2.2 verified)
Tracking | Status | |
---|---|---|
b2g-v2.2 | --- | verified |
People
(Reporter: rudyl, Assigned: rudyl)
References
Details
(Keywords: regression, Whiteboard: [p=2])
Attachments
(1 file)
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.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8487826 [details]
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
Attachment #8487826 -
Attachment description: WIP → Patch V1
Attachment #8487826 -
Flags: review?(timdream)
Updated•10 years ago
|
Blocks: 934209
Keywords: regression
Comment 4•10 years ago
|
||
Comment on attachment 8487826 [details]
Patch V1
Your math would be wrong if bug 934209 supports more than 2 lines. Did bug 934209 supports more than 2 lines?
Flags: needinfo?(rlu)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Flags: needinfo?(rlu)
Comment 6•10 years ago
|
||
Comment on attachment 8487826 [details]
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.
Attachment #8487826 -
Flags: review?(timdream)
Assignee | ||
Comment 7•10 years ago
|
||
** 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?
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8487826 [details]
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.
Attachment #8487826 -
Flags: review?(timdream)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8487826 [details]
Patch V1
Clear the review flag first as offline discussion, will try to refine the logic so that dummy key is not needed.
Attachment #8487826 -
Flags: review?(timdream)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8487826 [details]
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.
Attachment #8487826 -
Flags: review?(timdream)
Comment 12•10 years ago
|
||
Comment on attachment 8487826 [details]
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+
Comment 13•10 years ago
|
||
... let's *move* part of the getMenuTarget() ...
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8487826 [details]
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.
Attachment #8487826 -
Flags: review?(timdream)
Comment 15•10 years ago
|
||
Comment on attachment 8487826 [details]
Patch V1
Thanks!
Attachment #8487826 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 16•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Whiteboard: [p=1] → [p=2]
Target Milestone: --- → 2.1 S5 (26sep)
Updated•10 years ago
|
status-b2g-v2.2:
--- → fixed
Comment 17•10 years ago
|
||
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•