748590 - Re-map KEYCODE_DPAD_CENTER to DOM_VK_ENTER

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Eitan Isaacson [:eeejay]]

This was already fixed in bug 738741. But the digression appears in
http://hg.mozilla.org/mozilla-central/rev/a6dd3218bf6a

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Map KEYCODE_DPAD_CENTER to DOM_VK_ENTER.

Comment 2

[Chris Peterson [:cpeterson]]

Eitan, what device are you testing on? Something with hardware DPAD or arrow keys?

> android does not have a keycode for the numpad enter key, so this does not introduce new ambiguities.

btw, in bug 738741, you commented that Android does not have a keycode for numpad enter key, but it does: AndroidKeyEvent::KEYCODE_NUMPAD_ENTER. Gecko IME developer Masayuki recommended I map Android's KEYCODE_NUMPAD_ENTER to NS_VK_RETURN, so there is still no key conflict for DOM_VK_ENTER.

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 618090 [details] [diff] [review]
Map KEYCODE_DPAD_CENTER to DOM_VK_ENTER.

LGTM! Sorry for regressing you.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 618090 [details] [diff] [review]
Map KEYCODE_DPAD_CENTER to DOM_VK_ENTER.

[Approval Request Comment]
Regression caused by (bug #): bug 742036
User impact if declined: Android accessibility regression because DPAD "select" button broke.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low risk because the patch just changes a keycode mapping back to the old (correct) value.
String changes made by this patch: None

Comment 5

[Chris Peterson [:cpeterson]]

blocking-fennec1.0 for accessibility regression?

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Chris Peterson (:cpeterson) from comment #2)
> Eitan, what device are you testing on? Something with hardware DPAD or arrow
> keys?
> 
> > android does not have a keycode for the numpad enter key, so this does not introduce new ambiguities.
> 
> btw, in bug 738741, you commented that Android does not have a keycode for
> numpad enter key, but it does: AndroidKeyEvent::KEYCODE_NUMPAD_ENTER. Gecko
> IME developer Masayuki recommended I map Android's KEYCODE_NUMPAD_ENTER to
> NS_VK_RETURN, so there is still no key conflict for DOM_VK_ENTER.

Huh. Good catch. I was wondering why desktop firefox maps numpad enter to DOM_VK_RETURN... Anyway, it means an extra key code for us. So we win.

Comment 7

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/adea606b5694
http://hg.mozilla.org/releases/mozilla-aurora/rev/776a2c422717

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/adea606b5694

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to Chris Peterson (:cpeterson) from comment #2)
> > Eitan, what device are you testing on? Something with hardware DPAD or arrow
> > keys?
> > 
> > > android does not have a keycode for the numpad enter key, so this does not introduce new ambiguities.
> > 
> > btw, in bug 738741, you commented that Android does not have a keycode for
> > numpad enter key, but it does: AndroidKeyEvent::KEYCODE_NUMPAD_ENTER. Gecko
> > IME developer Masayuki recommended I map Android's KEYCODE_NUMPAD_ENTER to
> > NS_VK_RETURN, so there is still no key conflict for DOM_VK_ENTER.
> 
> Huh. Good catch. I was wondering why desktop firefox maps numpad enter to
> DOM_VK_RETURN... Anyway, it means an extra key code for us. So we win.

Because it should be distinguished by KeyboardEvent.location. It might be better that the patch of this bug is backed out and DPAD keys location should be JOYSTICK? I'm not sure what's like the actual interface of DPAD keys, though.

Comment 10

[Chris Peterson [:cpeterson]]

Some Android devices (like Sony Xperia) have a traditional game controller buttons like DPAD and A,B,C,L1,L2,R1,R2,X,Y,Z. The game buttons should probably use DOM_KEY_LOCATION_JOYSTICK. I can opened bug 756504 to track that work.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> (In reply to Eitan Isaacson [:eeejay] from comment #6)
> > (In reply to Chris Peterson (:cpeterson) from comment #2)
> > > Eitan, what device are you testing on? Something with hardware DPAD or arrow
> > > keys?
> > > 
> > > > android does not have a keycode for the numpad enter key, so this does not introduce new ambiguities.
> > > 
> > > btw, in bug 738741, you commented that Android does not have a keycode for
> > > numpad enter key, but it does: AndroidKeyEvent::KEYCODE_NUMPAD_ENTER. Gecko
> > > IME developer Masayuki recommended I map Android's KEYCODE_NUMPAD_ENTER to
> > > NS_VK_RETURN, so there is still no key conflict for DOM_VK_ENTER.
> > 
> > Huh. Good catch. I was wondering why desktop firefox maps numpad enter to
> > DOM_VK_RETURN... Anyway, it means an extra key code for us. So we win.
> 
> Because it should be distinguished by KeyboardEvent.location. It might be
> better that the patch of this bug is backed out and DPAD keys location
> should be JOYSTICK? I'm not sure what's like the actual interface of DPAD
> keys, though.

I am fine with this being backed out and KeyboardEvent.location being used. I was not aware of that earlier.

Comment 12

[Chris Peterson [:cpeterson]]

Eitan, will returning NS_VK_RETURN (instead of NS_VK_ENTER) cause accessibility regressions for content? XUL Fennec returned NS_VK_ENTER so this will change existing behavior.

Comment 13

[Eitan Isaacson [:eeejay]]

(In reply to Chris Peterson (:cpeterson) from comment #12)
> Eitan, will returning NS_VK_RETURN (instead of NS_VK_ENTER) cause
> accessibility regressions for content? XUL Fennec returned NS_VK_ENTER so
> this will change existing behavior.

No, it should be fine. Originally we thought all devices with a hw keyboard have both dbad center and return, but it ends up that sony devices don't have dpad center/ok/select, so we needed to map things to return anyway.

The long term solution seems to be adding the location nuance, but backing this out should be fine for now.

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: bug-748590-backout-dpad-enter.patch

Backout adea606b5694 to map KEYCODE_DPAD_CENTER to DOM_VK_RETURN.

Comment 15

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6cfe7a229f2

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 625224 [details] [diff] [review]
bug-748590-backout-dpad-enter.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 748590 (this bug :)
User impact if declined: Android D-pad control will return an incorrect key code to Gecko.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Android Java only. This bug is a fennec soft blocker. This patch just backs out an earlier patch from this bug.
String or UUID changes made by this patch: N/A

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I was not aware of that earlier.

I implemented the new attribute defined in D3E just 2 weeks ago ;-)

Comment 18

[Matt Brubeck (:mbrubeck)]

Backout landed on mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/e6cfe7a229f2

Should this be reopened, or changed to WONTFIX?

Comment 19

[Chris Peterson [:cpeterson]]

Good point, mbrubeck. Since we have decided to back out the bug's original patch, I will close this bug as WONTFIX.

We still want the backout merged to m-a.

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 625224 [details] [diff] [review]
bug-748590-backout-dpad-enter.patch

[Triage Comment]
Soft blocker, mobile only, approved for Aurora 14.

Comment 21

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cda5d09d37bb

Comment 22

[JP Rosevear [:jpr]]

Marking fixed since it was resolved for Aurora (via a backout).