619519 - Fix android key down/press/up handling

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Comment 1

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

I think that nsWindow::InitKeyEvent() shouldn't set charCode if the event is NS_KEY_DOWN or NS_KEY_UP. And if the event is NS_KEY_PRESS and it has charCode, should clear the keyCode.

Comment 2

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

I meant that we shouldn't fix this bug by changing nsWindow::OnKeyEvent().

Comment 3

[Michael Wu [:mwu]]

Attachment: Improve key event handling on android

Comment 4

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

Comment on attachment 498172 [details] [diff] [review]
Improve key event handling on android

+    nsEventStatus status;
     nsKeyEvent event(PR_TRUE, msg, this);
     InitKeyEvent(event, *ae);
-    DispatchEvent(&event);
+    DispatchEvent(&event, status);
 
-    if (isDown) {
+    if (isDown && status != nsEventStatus_eConsumeNoDefault) {
         nsKeyEvent pressEvent(PR_TRUE, NS_KEY_PRESS, this);

This is wrong. Even if the keydown event was consumed and shouldn't do default, we must dispatch the event. But you need to set NS_EVENT_FLAG_NO_DEFAULT flag to the keypress event's flags. See gtk2 implementation:
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#3067

However, there is another rule. You must not dispatch keypress event if the down key is a modifier key such as Ctrl, Alt, Meta or Shift key. I think that InitKeyEvent() should return PRBool if the event shouldn't be dispatched. Then, the OnKeyEvent() will not be complicated.

Comment 5

[Michael Wu [:mwu]]

Attachment: Improve key event handling on android, v2

Comment 6

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

Comment on attachment 498235 [details] [diff] [review]
Improve key event handling on android, v2

Can we use MotionEvent.getMetaState()?
http://developer.android.com/reference/android/view/MotionEvent.html#getMetaState%28%29

Looks like we don't copy it for MotionEvent.
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/AndroidJavaWrappers.cpp#297

Comment 7

[Michael Wu [:mwu]]

Attachment: Improve key event handling on android, v3

Looks like android does provide proper alt/shift key state reporting after all. I missed that somehow - thanks for pointing that out.

This patch also filters out ascii control characters that android might send. Only setting \n in charCode without setting keyCode breaks our fennec frontend. \n might be the only control code that android tries to send, but a bunch of them are filtered out just in case.

Comment 8

[Michael Wu [:mwu]]

Also note that the simple gestures don't get shift and alt set correctly, but that's fine as we're currently planning to remove those events completely.

Comment 9

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

Comment on attachment 498437 [details] [diff] [review]
Improve key event handling on android, v3

great! thanks!

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 498437 [details] [diff] [review]
Improve key event handling on android, v3

Requesting approval for a patch which will give us key event handling parity with other widget backends.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 498437 [details] [diff] [review]
Improve key event handling on android, v3

Can't give approval without some kind of tests. Once our Android testing system gets off the ground we can start that process.

But since this could fix a blocker, we can make it a blocker.

Comment 12

[Michael Wu [:mwu]]

Attachment: hg patch ready patch

Comment 13

[Alon Zakai (:azakai)]

Pushed:

http://hg.mozilla.org/mozilla-central/rev/a2a3a6e8b0e0