842013 - keyup, keydown and keypress Event is not fired

Status: RESOLVED FIXED

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

Description

[Fabio Buda]

Attachment: A testcase consisting of 3 input elements each with inline event-driven callback.

#1 Install/Launch Firefox for Android 18 (or beta 19).

#2 Open the testcase - it is simply a group of three inputs with key(up|down|press) events attached inline.

#3 Give focus to one of the inputs.

#4 Type something.

What happens:
The attached callback is not executed (the event is not fired) until you press enter (or backspace).

What should happen:
The attached callback should be executed at any keyboard up|down|press.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Virtual keyboards may not generate key events in many cases. Oftentimes, they perform more complex operations than simple key events allow: for example, if you press space to auto-correct a word, should a space key event be generated? Or key events corresponding to the corrected word?

The best way to detect changing input is to use the input event. For example,

> <input oninput="alert('on input');">

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Send key events for character input (v1)

This patch switches back to using key events for entering text outside of a composition.

There will be at least two more patches -- one to address an issue with duplicate key up events when using a hardware keyboard, and another to address unusual key mappings on Gingerbread devices involving certain characters like '<' and '>'.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Suppress key up events during key down event (v1)

Editable can be used in many places, and we only want to suppress key up events during the key down handler.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 729765 [details] [diff] [review]
Send key events for character input (v1)

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

::: mobile/android/base/GeckoEditable.java
@@ +263,5 @@
> +            KeyEvent [] keyEvents = mKeyMap.getEvents(
> +                action.mSequence.toString().toCharArray());
> +            if (keyEvents == null || keyEvents.length == 0) {
> +                return false;
> +            }

1. Please extract all the messy KeyCharacterMap.load() and mKeyMap.getEvents() code into a separate private helper method. Call it something like `KeyEvent[] synthesizeKeyEvents(String str)`.

2. Do we need to worry all the special cases we used to handle for Froyo and GB? Here is the old synthesizeKeyEvents() method:

https://hg.mozilla.org/mozilla-central/annotate/fda4bfed1a1b/mobile/android/base/GeckoInputConnection.java#l635

3. If we want to handle these cases, we should consider encapsulating all the KeyCharacterMap code and special cases into our own ("KeyMap"?) class.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 729768 [details] [diff] [review]
Suppress key up events during key down event (v1)

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

ok

::: mobile/android/base/GeckoEditable.java
@@ +598,3 @@
>      public Handler getInputConnectionHandler() {
> +        // Can be called from either UI thread or IC thread;
> +        // take must be taken to avoid race conditions

s/take/care/ ?

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Send key events for character input (v2)

(In reply to Chris Peterson (:cpeterson) from comment #4)
> Comment on attachment 729765 [details] [diff] [review]
> Send key events for character input (v1)
> 
> Review of attachment 729765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoEditable.java
> @@ +263,5 @@
> > +            KeyEvent [] keyEvents = mKeyMap.getEvents(
> > +                action.mSequence.toString().toCharArray());
> > +            if (keyEvents == null || keyEvents.length == 0) {
> > +                return false;
> > +            }
> 
> 1. Please extract all the messy KeyCharacterMap.load() and
> mKeyMap.getEvents() code into a separate private helper method. Call it
> something like `KeyEvent[] synthesizeKeyEvents(String str)`.

Done

> 2. Do we need to worry all the special cases we used to handle for Froyo and
> GB? Here is the old synthesizeKeyEvents() method:
> 
> https://hg.mozilla.org/mozilla-central/annotate/fda4bfed1a1b/mobile/android/
> base/GeckoInputConnection.java#l635
> 
> 3. If we want to handle these cases, we should consider encapsulating all
> the KeyCharacterMap code and special cases into our own ("KeyMap"?) class.

I'm going to post another patch that makes the special cases unnecessary.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Suppress key up events during key down event (v1.1)

Addressed review comment

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't add extra modifiers to key press events (v1)

Basically some special characters didn't work on Gingerbread because they had extra modifiers like Ctrl added. This patch makes us not add modifiers to a key press if the character already has modifiers applied.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 730239 [details] [diff] [review]
Send key events for character input (v2)

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

LGTM!

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

I found a bug using the testcase in attachment 714774 [details]

Because we are sending key events one-by-one, if there's a window.alert() in the event handler, the events could end up being processed out-of-order. Seems to me like the best solution is to bunch up the key events in Gecko and dispatch them all at once.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Buffer IME key events and dispatch them at once (v1)

This patch creates a new Gecko event type - IME_KEY_EVENT, which is exactly the same as KEY_EVENT, except these new events are saved in an array until the next IME_REPLACE_TEXT event, which dispatches these events instead of the normal composition events. This ensures that the IME key events are processed in-order.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 730242 [details] [diff] [review]
Don't add extra modifiers to key press events (v1)

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

LGTM

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 730436 [details] [diff] [review]
Buffer IME key events and dispatch them at once (v1)

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

LGTM

::: mobile/android/base/GeckoEditable.java
@@ +284,2 @@
>              }
> +            return;

You don't need this return;

::: widget/android/nsWindow.cpp
@@ +877,5 @@
>              break;
>  
> +        case AndroidGeckoEvent::IME_KEY_EVENT:
> +            if (win->mFocus) {
> +                win->mFocus->mIMEKeyEvents.AppendElement(*ae);

Maybe add a short comment that these IME key events are batched up until IME_REPLACE_TEXT.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Addressed review comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/154de20f3148
https://hg.mozilla.org/integration/mozilla-inbound/rev/fccb21a4fada
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d5b906ca42
https://hg.mozilla.org/integration/mozilla-inbound/rev/95c1cfb26fda

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/154de20f3148
https://hg.mozilla.org/mozilla-central/rev/fccb21a4fada
https://hg.mozilla.org/mozilla-central/rev/d8d5b906ca42
https://hg.mozilla.org/mozilla-central/rev/95c1cfb26fda