Sometimes the last alternative key would be wrongly highlighted

VERIFIED FIXED in 2.1 S5 (26sep)

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: rudyl, Assigned: rudyl)

Tracking

({regression})

unspecified
2.1 S5 (26sep)
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified)

Details

(Whiteboard: [p=2])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
timdream
: review+
timdream
: feedback+
Details | Review
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
Status: NEW → ASSIGNED
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
Attachment #8487826 - Attachment description: WIP → Patch V1
Attachment #8487826 - Flags: review?(timdream)
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?
Flags: needinfo?(rlu)
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 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.
Attachment #8487826 - Flags: review?(timdream)
** 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.
Attachment #8487826 - Flags: review?(timdream)
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.
Attachment #8487826 - Flags: review?(timdream)
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.
Attachment #8487826 - Flags: review?(timdream)

Comment 11

5 years ago
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.
Attachment #8487826 - Flags: review?(timdream)
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: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=1] → [p=2]
Target Milestone: --- → 2.1 S5 (26sep)
Depends on: 1074095

Comment 17

5 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)
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.