Closed Bug 611764 Opened 9 years ago Closed 9 years ago

Hard keyboard events not being seen on page without input field

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: dre, Assigned: mwu)

References

()

Details

(Whiteboard: [hkb])

Attachments

(1 file, 3 obsolete files)

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b8pre) Gecko/20101112 Firefox/4.0b8pre Fennec/4.0b3pre

Samsung Galaxy S Pro (Sprint Epic 4G)

Tried visiting the JSGB gameboy emulator page.  Was happy to see that it renders even if at 6 to 8 FPS.  But I'm unable to start a game.  Pressing A button has no affect.  Pressing the arrow keys causes the screen to scroll.  Maybe the listeners they are trying to use don't work properly?

It was also tested by people in IRC on a G2 and Droid 2 with the same effect.  It was reported to work correctly on the Linux Desktop version of Fennec.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
It does not work on Nexus one too by forcing the display of the Virtual Keyboard but works on desktop.
Using the emulator, I get key events, but all the key codes are 0. Can you confirm that with your Epic (the result would likely be the same on your gameboy page)?
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attached patch patch (obsolete) — Splinter Review
I was able to test on dougt's droid 2. There were two issues. The first was that the ime suggestions eat the key events. The second is that the events don't have keycodes when they get to js. I don't know why we were zero'ing out event.keycode in the first place
Attachment #496071 - Attachment is obsolete: true
Attachment #496399 - Flags: review?(mwu)
Attached patch Fix (obsolete) — Splinter Review
Moves most of the key handling into surfaceview and uses onkeypreime to bypass the IME when we know we shouldn't be using the IME.
Assignee: blassey.bugs → mwu
Attachment #496399 - Attachment is obsolete: true
Attachment #496658 - Flags: review?(blassey.bugs)
Attachment #496399 - Flags: review?(mwu)
Comment on attachment 496658 [details] [diff] [review]
Fix


>     public boolean onKeyMultiple(int keyCode, int repeatCount, KeyEvent event) {
>         GeckoAppShell.sendEventToGecko(new GeckoEvent(event));
>         return true;

should onKeyMulitple be moved to the surface view as well?
>+        if (mIMEState == GeckoSurfaceView.IME_STATE_DISABLED ||
shouldn't need GeckoSurfaceView here

>+            !mKeyListener.onKeyDown(GeckoApp.surfaceView, GeckoApp.surfaceView.mEditable, keyCode, event))
I assume that GeckoApp.surfaceView is this, so just use this and mEditable

>+        if (mIMEState == GeckoSurfaceView.IME_STATE_DISABLED ||
>+            keyCode == KeyEvent.KEYCODE_ENTER ||
>+            !mKeyListener.onKeyUp(GeckoApp.surfaceView, GeckoApp.surfaceView.mEditable, keyCode, event))
same as above

>+        switch (event.getAction()) {
>+            case KeyEvent.ACTION_DOWN:
>+                return onKeyDown(keyCode, event);
>+            case KeyEvent.ACTION_UP:
>+                return onKeyUp(keyCode, event);
>+        }
why not ACTION_MULTIPLE?
>+        return super.onKeyPreIme(keyCode, event);
if we have a case for multiple above, we should never hit this right? Please just Log an error message in that case

>     InitKeyEvent(event, *ae);
>-    if (event.charCode)
>-        event.keyCode = 0;
did you determine if this is the right thing to do, or if we should clear the keycode if a meta flag is set like some other platforms do?

r+ with nits addressed
Attachment #496658 - Flags: review?(blassey.bugs) → review+
(In reply to comment #6)
> Comment on attachment 496658 [details] [diff] [review]
> Fix
> 
> 
> >     public boolean onKeyMultiple(int keyCode, int repeatCount, KeyEvent event) {
> >         GeckoAppShell.sendEventToGecko(new GeckoEvent(event));
> >         return true;
> 
> should onKeyMulitple be moved to the surface view as well?

Maybe, but I don't remember how to test onKeyMultiple so I don't want to risk it. I'll file a follow up.

> >+        switch (event.getAction()) {
> >+            case KeyEvent.ACTION_DOWN:
> >+                return onKeyDown(keyCode, event);
> >+            case KeyEvent.ACTION_UP:
> >+                return onKeyUp(keyCode, event);
> >+        }
> why not ACTION_MULTIPLE?

ACTION_MULTIPLE is a software (IME generated), not hardware event AFAIK, so it wouldn't hit this path.

> >     InitKeyEvent(event, *ae);
> >-    if (event.charCode)
> >-        event.keyCode = 0;
> did you determine if this is the right thing to do, or if we should clear the
> keycode if a meta flag is set like some other platforms do?
> 

According to http://www.quirksmode.org/js/keys.html , keyCode is always set on down and up events. The oddity is mostly in the handling of charCode.
Now that I look at our handling of onKeyMultiple, I'm pretty sure its at least partially wrong. According to the android docs (http://developer.android.com/reference/android/view/KeyEvent.html#getCharacters%28%29) getCharecters() is only non-null for the special case of KEYCODE_UNKNOWN, which is the IME generated case I think your thinking of (and/or perhaps the ".com" button).

I read the docs for ACTION_MULIPLE (http://developer.android.com/reference/android/view/KeyEvent.html#ACTION_MULTIPLE) to mean that the case when keycode is not KEYCODE_UNKNOWN is a single key hit repeatedly (or perhaps held down). 

It seems like the right thing to do in nsWindow.cpp (https://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsWindow.cpp#1468) is to test for keycode != KEYCODE_UNKNOWN and send multiple nsKeyEvents. We'll have to add mRepeatCount to GeckoEvent to support that. Please file a follow up bug to address this.

Moving onKeyMultpile into GeckoSurfaceView seems pretty save and keeping all the key handling code together seems like the right thing to do. Please also add the ACTION_MULTIPLE case to that switch.
Requesting approval for b3 since this is a nice improvement for users of hard keyboards. I've tested this a bit and there are no regressions afaict.
Attachment #496658 - Attachment is obsolete: true
Attachment #497377 - Flags: approval2.0?
Attachment #497377 - Flags: approval2.0? → approval2.0+
pushed http://hg.mozilla.org/mozilla-central/rev/cf153d816745
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to comment #7)
> (In reply to comment #6)
> > >     InitKeyEvent(event, *ae);
> > >-    if (event.charCode)
> > >-        event.keyCode = 0;
> > did you determine if this is the right thing to do, or if we should clear the
> > keycode if a meta flag is set like some other platforms do?
> > 
> 
> According to http://www.quirksmode.org/js/keys.html , keyCode is always set on
> down and up events. The oddity is mostly in the handling of charCode.

Why did you change this? I don't understand the reason. Gecko shouldn't set keyCode if charCode isn't 0. Gecko on the other platforms is doing so. And the website is working well on latest trunk on Win7. Did you confirm the cause isn't the website's script?
Oh, I see. The old code is wrong.


1472 void
1473 nsWindow::OnKeyEvent(AndroidGeckoEvent *ae)
1474 {
1475     PRUint32 msg;
1476     switch (ae->Action()) {
1477     case AndroidKeyEvent::ACTION_DOWN:
1478         msg = NS_KEY_DOWN;
1479         break;
1480     case AndroidKeyEvent::ACTION_UP:
1481         msg = NS_KEY_UP;
1482         break;

1518     nsKeyEvent event(PR_TRUE, msg, this);
1519     InitKeyEvent(event, *ae);
1520     if (event.charCode)
1521         event.keyCode = 0;
1522     DispatchEvent(&event);
1523 
1524     if (isDown) {
1525         nsKeyEvent pressEvent(PR_TRUE, NS_KEY_PRESS, this);
1526         InitKeyEvent(pressEvent, *ae);
1527 #ifdef ANDROID_DEBUG_WIDGET
1528         ALOG("Dispatching key event with keyCode %d charCode %d shift %d alt %d sym/ctrl %d metamask %d", event.keyCode, event.charCode, event.isShift, event.isAlt, event.isControl, ae->MetaState());
1529 #endif
1530         DispatchEvent(&pressEvent);
1531     }

The clearing code clears the keyCode of NS_KEY_DOWN. This is wrong. When NS_KEY_DOWN or NS_KEY_UP event is dispatched, their keyCode value shouldn't be cleared but their charCode must be 0.

Furthermore, in |if (isDown)| block, the keyCode of NS_KEY_PRESS should be cleared if the charCode isn't 0. Please backout the patch and fix by the right way.

-> REOPEN
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #11)
> > According to http://www.quirksmode.org/js/keys.html , keyCode is always set on
> > down and up events. The oddity is mostly in the handling of charCode.
I general, don't trust quirksmode.org. Based on my experience it has had
plenty of wrong information. Though, could be that the situation has changed.

> Why did you change this? I don't understand the reason. Gecko shouldn't set
> keyCode if charCode isn't 0. 
Yeah. keyCode in keypress event is 0 when there is charCode
http://mozilla.pettay.fi/moztests/events/keycode-charcode-tester.html
(In reply to comment #12)
> The clearing code clears the keyCode of NS_KEY_DOWN. This is wrong. When
> NS_KEY_DOWN or NS_KEY_UP event is dispatched, their keyCode value shouldn't be
> cleared but their charCode must be 0.
> 
> Furthermore, in |if (isDown)| block, the keyCode of NS_KEY_PRESS should be
> cleared if the charCode isn't 0. Please backout the patch and fix by the right
> way.
> 
Closing again. This doesn't appear to be a regression nor does it make the behavior any more wrong.. I'll file a new bug to fix all the bugs in the android key event impl as this isn't the only one and I'd like to fix them all at once.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
On my Droid device, I keep on crashing on the url, I filed bug 641795 for that.

I can verify that it works on Maemo, though. (but it's very slow).
This was tested on:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre
naoki, can you see if this is fixed on the droid 2?  or kevin, can u check the G2?
VERIFIED FIXED on:
Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110315
Firefox/4.0b13pre Fennec /4.0b6pre
Device: Motorola Droid 2
Status: RESOLVED → VERIFIED
Fixed on Droid 2 as per above.
You need to log in before you can comment on or make changes to this bug.