The default bug view has changed. See this FAQ.

Sony Ericsson Xperia Play's "circle" and "cross" D-pad buttons generate unexpected keycodes

RESOLVED FIXED in Firefox 24

Status

()

Firefox for Android
Keyboards and IME
P4
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: stully)

Tracking

Trunk
Firefox 24
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox18 affected, firefox23 wontfix, firefox24 fixed)

Details

(Whiteboard: [mentor=cpeterson][lang=java])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
The Sony Ericsson Xperia Play's D-pad button layout (unsurprisingly) matches the PlayStation game controller.

AFAIK, the expected mapping from PlayStation to Xbox controller buttons is:

  PS Cross    -> Xbox A
  PS Circle   -> Xbox B
  PS Square   -> Xbox X
  PS Triangle -> Xbox Y

The actual keycodes generated by the Xperia Play buttons is:

  Cross    -> Expected KEYCODE_BUTTON_A; got KEYCODE_DPAD_CENTER (keycode 23)
  Circle   -> Expected KEYCODE_BUTTON_B; got META_ALT_ON + KEYCODE_BACK (keycode 4)
  Square   -> Expected and got KEYCODE_BUTTON_X (keycode 99)
  Triangle -> Expected and got KEYCODE_BUTTON_Y (keycode 100)
(Reporter)

Updated

5 years ago
Summary: Son Ericsson Xperia Play's "circle" and "cross" D-pad buttons generate unexpected keycodes → Sony Ericsson Xperia Play's "circle" and "cross" D-pad buttons generate unexpected keycodes
(Reporter)

Updated

4 years ago
Whiteboard: [mentor=cpeterson][lang=java]
Version: unspecified → Trunk
(Reporter)

Comment 1

4 years ago
Chris, you may be interested in this bug. :)

Comment 2

4 years ago
If we think of X as 'select' and O as 'back' (which it is in Western games - interestingly, O is 'select' and I think triangle or X is back in Japan...), then the mappings make some sense... Still a bit annoying, but we should take it into account as many bluetooth gamepads emulate the Xperia Play key codes.
(Reporter)

Comment 3

4 years ago
If the Gamepad API defines gamepad button events, we should ensure that we map these Java keycodes to the correct gamepad codes.

https://wiki.mozilla.org/GamepadAPI#Gamepad_Button_Events
(Reporter)

Updated

4 years ago
Depends on: 604039
Priority: P3 → P4
(Reporter)

Comment 4

4 years ago
This is a good first bug.
Assignee: nobody → stully
(Assignee)

Comment 5

4 years ago
Created attachment 752466 [details] [diff] [review]
Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

The attached patch remaps the cross key to KEYCODE_BUTTON_A and the circle key to KEYCODE_BUTTON_B or vice verse if the keys are swapped (for devices in certain regions)
(Reporter)

Comment 6

4 years ago
Comment on attachment 752466 [details] [diff] [review]
Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

Review of attachment 752466 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Here are some suggestions:

::: mobile/android/base/GeckoInputConnection.java
@@ +24,5 @@
>  import android.util.Log;
>  import android.view.KeyEvent;
>  import android.view.View;
> +import android.view.InputDevice;
> +import android.view.KeyCharacterMap;

Please alphabetize these new android.view.* imports.

@@ +759,5 @@
>          }
>          return false;
>      }
>  
> +    private KeyEvent translateSonyXperiaGamepad(int keyCode, KeyEvent event) {

I would rename this method to something like `translateSonyXperiaGamepadKey()`.

@@ +762,5 @@
>  
> +    private KeyEvent translateSonyXperiaGamepad(int keyCode, KeyEvent event) {
> +        // The cross and circle button mappings may be swapped in the different
> +        // regions so determine if they are swapped so the proper key codes
> +        // can be mapped to the keys

Mozilla's C++ coding style requires 80 column lines, but we allow Java files to use 100 columns (because Java code is so verbose). So you can use wider comments, if you like.

@@ +764,5 @@
> +        // The cross and circle button mappings may be swapped in the different
> +        // regions so determine if they are swapped so the proper key codes
> +        // can be mapped to the keys
> +        boolean areKeysSwapped = areSonyXperiaGamepadKeysSwapped();
> +        

Please remove trailing whitespace.

@@ +768,5 @@
> +        
> +        // If a Sony Xperia, remap the cross and circle buttons to buttons
> +        // A and B for the gamepad API
> +        int newKeyCode = keyCode;
> +        switch(keyCode) {

Please insert a space between before the first paren.

@@ +778,5 @@
> +                newKeyCode = (areKeysSwapped ? KeyEvent.KEYCODE_BUTTON_B : KeyEvent.KEYCODE_BUTTON_A);
> +                break;
> +        }
> +
> +        return new KeyEvent(event.getAction(), newKeyCode);

Instead of allocating a new KeyEvent every time, let's add a default case to the switch statement that just returns the original `event` parameter. You will probably be able to skip the initialization of `newKeyCode = keyCode` (because the compiler should be able tell that newKeyCode will always be initialized in every code path).

@@ +790,5 @@
> +
> +        boolean swapped = false;
> +        int[] ids = InputDevice.getDeviceIds();
> +
> +        for (int i= 0; ids != null && i<ids.length; i++) {

I know this code was written by someone else, but please add spaces before and after the '<' operator.

@@ +815,5 @@
>      }
>  
>      private boolean processKey(int keyCode, KeyEvent event, boolean down) {
> +        // If the key event came from a Sony Xperia keypad, remap the keys
> +        // if necessary

We can probably remove this comment because the translateSonyXperiaGamePad() method name is pretty self-explanatory. :)

@@ +816,5 @@
>  
>      private boolean processKey(int keyCode, KeyEvent event, boolean down) {
> +        // If the key event came from a Sony Xperia keypad, remap the keys
> +        // if necessary
> +        if (event.getDeviceId() == SONY_XPERIA_GAMEPAD_DEVICE_ID) {

I'm worried that some other (Sony) device might return the same device ID. I think we should double-check the android.os.Build.MANUFACTURER and MODEL, too. Check device ID first because it is cheap integer comparison.

Now that the Xperia device check is getting more complicated, I think we should extract that logic into a static helper method. Something like `private static boolean isSonyXperiaKeyEvent(KeyEvent event)`.
Attachment #752466 - Flags: feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 752849 [details] [diff] [review]
02: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

Revised patch with changes requested by :cpeterson
Drive-by comment: If some of these new functions are meant to be reusable it might be better to put them in the util/GamepadUtils.java file. I'm not sure if that's more appropriate or not.
(Reporter)

Updated

4 years ago
Attachment #752466 - Attachment is obsolete: true
(Reporter)

Comment 9

4 years ago
Comment on attachment 752849 [details] [diff] [review]
02: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

Review of attachment 752849 [details] [diff] [review]:
-----------------------------------------------------------------

kats makes a good point. Even though these new methods are specific to the Sony Xperia devices, I think we should move them to util/GamepadUtils.java. We can just expose isSonyXperiaGamepadKeyEvent() and translateSonyXperiaGamepadKeys() from GamepadUtils.java and encapsulate all the implementation details in private methods.

Plus, GeckoInputConnection.java is already a huge file, so moving code to other files is a good idea in general. :)

::: mobile/android/base/GeckoInputConnection.java
@@ +769,5 @@
> +        // A and B for the gamepad API
> +        switch (keyCode) {
> +            case KeyEvent.KEYCODE_BACK:
> +                keyCode = (areKeysSwapped ? KeyEvent.KEYCODE_BUTTON_A : 
> +                                            KeyEvent.KEYCODE_BUTTON_B);

Please remove the trailing whitespace at the end of the line and move the colon to the next line, indented to line up with the question mark. Mozilla coding style is to put && and || at the end of a continued line, but put other operators like + or - at the beginning of the continuing line.

@@ +774,5 @@
> +                break;
> +
> +            case KeyEvent.KEYCODE_DPAD_CENTER:
> +                keyCode = (areKeysSwapped ? KeyEvent.KEYCODE_BUTTON_B : 
> +                                            KeyEvent.KEYCODE_BUTTON_A);

Same as above.

@@ +805,5 @@
> +        return swapped;
> +    }
> +
> +    private static boolean isSonyXperiaGamepadKeyEvent(KeyEvent event) {
> +        return (event.getDeviceId() == SONY_XPERIA_GAMEPAD_DEVICE_ID && 

Please remove trailing whitespace.

@@ +807,5 @@
> +
> +    private static boolean isSonyXperiaGamepadKeyEvent(KeyEvent event) {
> +        return (event.getDeviceId() == SONY_XPERIA_GAMEPAD_DEVICE_ID && 
> +                android.os.Build.MANUFACTURER.equals("Sony Ericsson") &&
> +                (android.os.Build.MODEL.equals("R800") || android.os.Build.MODEL.equals("R800i")));

* You can just specify `Build.MODEL` instead of `android.os.Build.MODEL` because this file already imported android.os.Build above.

* Our Java coding style for string comparisons is to put the string constant first (like `"Sony Ericsson".equals(Build.MANUFACTURER)`. This style is a little ugly but it protects against NullPointerExceptions (because the equal() method checks for null) and may, in theory, allow Java to make some micro-optimization for the method call because the object, the string constant, is known statically at compile-time.
Attachment #752849 - Flags: feedback+
(Assignee)

Comment 10

4 years ago
Created attachment 752890 [details] [diff] [review]
03: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region


Revised patch with changes requested by :cpeterson and :kats
Attachment #752890 - Flags: review?(cpeterson)
(Assignee)

Updated

4 years ago
Attachment #752849 - Attachment is obsolete: true
(Reporter)

Comment 11

4 years ago
Comment on attachment 752890 [details] [diff] [review]
03: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

Review of attachment 752890 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! I can land your patch on mozilla-inbound today.
Attachment #752890 - Flags: review?(cpeterson) → review+
(Reporter)

Comment 12

4 years ago
I landed stully's patch on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0d9ac28abb
status-firefox23: --- → wontfix
status-firefox24: --- → affected
https://hg.mozilla.org/mozilla-central/rev/cd0d9ac28abb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

4 years ago
status-firefox24: affected → fixed
You need to log in before you can comment on or make changes to this bug.