Closed Bug 619519 Opened 9 years ago Closed 9 years ago

Fix android key down/press/up handling

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
tracking-fennec: --- → ?
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.
I meant that we shouldn't fix this bug by changing nsWindow::OnKeyEvent().
Attachment #498172 - Flags: review?(masayuki)
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.
Attachment #498172 - Attachment is obsolete: true
Attachment #498235 - Flags: review?(masayuki)
Attachment #498172 - Flags: review?(masayuki)
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.
Attachment #498235 - Attachment is obsolete: true
Attachment #498437 - Flags: review?(masayuki)
Attachment #498235 - Flags: review?(masayuki)
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 on attachment 498437 [details] [diff] [review]
Improve key event handling on android, v3

great! thanks!
Attachment #498437 - Flags: review?(masayuki) → review+
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.
Attachment #498437 - Flags: approval2.0?
Blocks: 618352
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.
Attachment #498437 - Flags: approval2.0?
tracking-fennec: ? → 2.0b4+
Attachment #498437 - Attachment is obsolete: true
No longer blocks: 618352
Pushed:

http://hg.mozilla.org/mozilla-central/rev/a2a3a6e8b0e0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 615308
You need to log in before you can comment on or make changes to this bug.