Closed Bug 748590 Opened 10 years ago Closed 10 years ago

Re-map KEYCODE_DPAD_CENTER to DOM_VK_ENTER

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox14 fixed, blocking-fennec1.0 soft)

RESOLVED WONTFIX
Firefox 15
Tracking Status
firefox14 --- fixed
blocking-fennec1.0 --- soft

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

(Keywords: regression)

Attachments

(2 files)

This was already fixed in bug 738741. But the digression appears in
http://hg.mozilla.org/mozilla-central/rev/a6dd3218bf6a
Attachment #618090 - Flags: review?(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.
OS: Linux → Android
Hardware: x86_64 → ARM
Comment on attachment 618090 [details] [diff] [review]
Map KEYCODE_DPAD_CENTER to DOM_VK_ENTER.

LGTM! Sorry for regressing you.
Attachment #618090 - Flags: review?(cpeterson) → review+
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
Attachment #618090 - Flags: approval-mozilla-aurora?
blocking-fennec1.0 for accessibility regression?
Blocks: 742036
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → soft
Attachment #618090 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
https://hg.mozilla.org/mozilla-central/rev/adea606b5694
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 14 → Firefox 15
(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.
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.
(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.
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.
(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.
Backout adea606b5694 to map KEYCODE_DPAD_CENTER to DOM_VK_RETURN.
Attachment #625224 - Flags: review?(eitan)
Attachment #625224 - Flags: review?(eitan) → review+
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
Attachment #625224 - Flags: approval-mozilla-aurora?
> I was not aware of that earlier.

I implemented the new attribute defined in D3E just 2 weeks ago ;-)
Backout landed on mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/e6cfe7a229f2

Should this be reopened, or changed to WONTFIX?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → WONTFIX
Comment on attachment 625224 [details] [diff] [review]
bug-748590-backout-dpad-enter.patch

[Triage Comment]
Soft blocker, mobile only, approved for Aurora 14.
Attachment #625224 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking fixed since it was resolved for Aurora (via a backout).
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.