912858 - Implement KeyboardEvent.key for printable keys (except dead key handling)

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

KeyboardEvent.key spec for printable keys will be clearly defined in the draft with examples.
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#keys-Modifiers

Comment 1

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

Attachment: part.1 WidgetKeyboardEvent should be able to store .key value as string if the key name isn't one of pre-defined key names

I think that mKey is too general and not easy to grep from the tree. Therefore, I names the new member which stores printable key's |.key| value as mKeyValue.

Comment 2

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

Attachment: part.2 Implement KeyboardEvent.key for printable keys on Windows

On Windows, NativeKey::mCommittedCharsAndModifiers stores a key value which we want for KeyboardEvent.key.

Comment 3

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

Attachment: part.3 Implement KeyboardEvent.key for printable keys on Mac

Unfortunately, current D3E spec doesn't define a .key value while Command key is pressed. Mac OS switches printable key's layout to ASCII capable when Command key is pressed but it causes difference value from Control key pressed case.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23908

Comment 4

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

Attachment: part.4 Implement KeyboardEvent.key for printable keys on GTK

Comment 5

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

Attachment: part.5 Implement KeyboardEvent.key for printable keys on Android

On Android, there is a problem. While NumLock is unlocked, Android sends KEYCODE_NUMPAD_* to us for keys in NumPad. So, we cannot know how the keys should work as non-printable key.

With this patch, |.key| value becomes empty string in such case.

Comment 6

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

Attachment: part.6-1 Create KeyEventDispatcher in gonk/nsAppShell.cpp for easier maintenance

Gonk's key event dispatcher has a couple of methods which have similar and a lot of arguments. This makes the code messy.

This patch creates KeyEventDispatcher class. The class members are initialized by the constructor and the methods use the members for initializing events to dispatch.

# This doesn't change the behavior.

Comment 7

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

Attachment: part.6-2 Implement KeyboardEvent.key for printable keys on Gonk

I don't have environment to test this patch. However, this patch is too simple. This must work well.

I realized that Gonk's key event dispatching code has a lot of problems which are solved on the other platforms for this several years. Additionally, it doesn't initialize modifier state! I.e., shiftKey, altKey, ctrlKey and metaKey must be always false. And getModifierState() must not work too. We should fix them later.

Comment 8

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

FYI:

The examples of the KeyboardEvent.key values are here:
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#keys-modifiers

I'll work on dead key cases in bug 938987. But it's not so important.

I don't create patches for Qt and OS/2 because I cannot build and test them. On them, printable keys keep causing "MozPrintableKey".

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 8340329 [details] [diff] [review]
part.6-1 Create KeyEventDispatcher in gonk/nsAppShell.cpp for easier maintenance

Looks good. I'll also test this and the next patch on my dev board later today, but this looks straightforward enough.

Comment 10

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

Comment on attachment 8340327 [details] [diff] [review]
part.5 Implement KeyboardEvent.key for printable keys on Android

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

LGTM.

::: mobile/android/base/GeckoEvent.java
@@ +251,5 @@
>          mCharacters = k.getCharacters();
> +        if (mUnicodeChar >= ' ') {
> +            mDOMPrintableKeyValue = mUnicodeChar;
> +        } else {
> +            int unModifiedMetaState =

unmodifiedMetaState (lowercase m in unmodified)

Comment 11

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

(In reply to Michael Wu [:mwu] from comment #9)
> Comment on attachment 8340329 [details] [diff] [review]
> part.6-1 Create KeyEventDispatcher in gonk/nsAppShell.cpp for easier
> maintenance
> 
> Looks good. I'll also test this and the next patch on my dev board later
> today, but this looks straightforward enough.

Thank you. Before you apply the patches of this, please apply a patch for bug 936318.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8340326 [details] [diff] [review]
part.4 Implement KeyboardEvent.key for printable keys on GTK

https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#key-algorithm
doesn't say what should happen if "the primary current function of the key" is
not "to generate a character", and there does not exist "an appropriate key
value in the key values set".

>+        uint32_t charCode = GetCharCodeFor(aGdkKeyEvent);
>+        if (!charCode) {
>+            charCode = keymapWrapper->GetUnmodifiedCharCodeFor(aGdkKeyEvent);
>+        }

Given no direction from the spec here, this approach looks reasonable.

Comment 13

[Michael Wu [:mwu]]

Comment on attachment 8340330 [details] [diff] [review]
part.6-2 Implement KeyboardEvent.key for printable keys on Gonk

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

This and the previous patch seems to test ok on my dev board. I wasn't able to test the numpad keys though, since I'm using a keyboard without a numpad. However, it seems reasonable to me.

::: widget/gonk/nsAppShell.cpp
@@ +245,5 @@
>      uint32_t mDOMKeyCode;
>      uint32_t mDOMCharCode;
>      uint64_t mDOMTimestamp;
>      KeyNameIndex mDOMKeyNameIndex;
> +    nsString mDOMPrintableKeyValue;

I think we should just store int16_t here and convert when we need to assign event.mKeyValue. KeyCharacterMap doesn't support anything better.

@@ +607,3 @@
>          sp<KeyCharacterMap> kcm = mEventHub->getKeyCharacterMap(data.deviceId);
> +        if (kcm.get()) {
> +            // XXX Needs more work for deciding charCode against for key hell.

I don't know what this comment means.

@@ +633,5 @@
> +                        }
> +                    }
> +                }
> +            }
> +        }

Too much indentation. I think this will end up cleaner if you move the charCode and DOMPrintableKeyValue logic into its own function.

Comment 14

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

Attachment: part.3 Implement KeyboardEvent.key for printable keys on Mac

While command key is pressed, the modified character should be set to .key because OS provides the character intentionally.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23908

See the comments in the patch for the other rules.

Comment 15

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

Attachment: part.6-1 Create KeyEventDispatcher in gonk/nsAppShell.cpp for easier maintenance

Okay, then, all attributes of KeyboardEvent should be computed in KeyEventDispatcher.

Comment 16

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

Attachment: part.6-2 Implement KeyboardEvent.key for printable keys on Gonk

Comment 17

[Michael Wu [:mwu]]

Comment on attachment 8343098 [details] [diff] [review]
part.6-1 Create KeyEventDispatcher in gonk/nsAppShell.cpp for easier maintenance

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

Looks good. I just have a small nit about where the Dispatch() function is placed.

::: widget/gonk/nsAppShell.cpp
@@ +223,5 @@
>  {
> +public:
> +    KeyEventDispatcher(const UserInputData& aData,
> +                       KeyCharacterMap* aKeyCharMap);
> +    void Dispatch()

I think this function deserves to be moved outside of the class declaration and with the rest of the other functions.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8343056 [details] [diff] [review]
part.3 Implement KeyboardEvent.key for printable keys on Mac

This looks fine to me.  But will it break current dead key functionality?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8340327 [details] [diff] [review]
part.5 Implement KeyboardEvent.key for printable keys on Android

"This isn't assmued this is called for ACTION_MULTIPLE" ?
assmued?

How can this compile? event.mKeyValue is nsString and
keyValue is int.

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 8343101 [details] [diff] [review]
part.6-2 Implement KeyboardEvent.key for printable keys on Gonk

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

Looks pretty good.

::: widget/gonk/nsAppShell.cpp
@@ +304,5 @@
> +    }
> +    int32_t unmodifiedMetaState = mData.metaState &
> +            ~(AMETA_ALT_ON | AMETA_ALT_LEFT_ON | AMETA_ALT_RIGHT_ON |
> +              AMETA_CTRL_ON | AMETA_CTRL_LEFT_ON | AMETA_CTRL_RIGHT_ON |
> +              AMETA_META_ON | AMETA_META_LEFT_ON | AMETA_META_RIGHT_ON);

nit: Indentation seems slightly off here.

Comment 21

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

Comment on attachment 8343101 [details] [diff] [review]
part.6-2 Implement KeyboardEvent.key for printable keys on Gonk

I'm thinking these patches should be landed after merge in next week, how about you?

Comment 22

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

(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 8340327 [details] [diff] [review]
> part.5 Implement KeyboardEvent.key for printable keys on Android
> 
> "This isn't assmued this is called for ACTION_MULTIPLE" ?
> assmued?

Oops,

> How can this compile? event.mKeyValue is nsString and
> keyValue is int.

Oh, I'll add static_cast<PRUnichar>.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> I'm thinking these patches should be landed after merge in next week, how
> about you?
Sounds good. I was about to suggest that :)

Comment 24

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

Attachment: part.5 Implement KeyboardEvent.key for printable keys on Android

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8344324 [details] [diff] [review]
part.5 Implement KeyboardEvent.key for printable keys on Android


>+            MOZ_ASSERT(
>+                aAndroidGeckoEvent.Action() != AKEY_EVENT_ACTION_MULTIPLE,
>+                "This isn't assumed that this is called for ACTION_MULTIPLE");
Perhaps just "Don't call this when action is AKEY_EVENT_ACTION_MULTIPLE!"

Comment 26

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/63a364842347
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/24417ae99937
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f32dd0f132
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/31e994f78a9e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55fbb1c09486
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5decddfef63d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65ef767aa205

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/63a364842347
https://hg.mozilla.org/mozilla-central/rev/24417ae99937
https://hg.mozilla.org/mozilla-central/rev/b0f32dd0f132
https://hg.mozilla.org/mozilla-central/rev/31e994f78a9e
https://hg.mozilla.org/mozilla-central/rev/55fbb1c09486
https://hg.mozilla.org/mozilla-central/rev/5decddfef63d
https://hg.mozilla.org/mozilla-central/rev/65ef767aa205

Comment 28

[Ioana (away)]

Is there anything Firefox Desktop QA can help with here?

Comment 29

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

If somebody in QA team is interested in DOM Level 3 Events, please test it. However, some part of the spec is still unstable. Therefore, it's difficult to decide what is buggy.

This is an experimental implementation for looking for bugs of current D3E draft. So, you don't need to worry about the quality of this implementation for now.

Thank you.

Comment 30

[Ioana (away)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #29)
> If somebody in QA team is interested in DOM Level 3 Events, please test it.
> However, some part of the spec is still unstable. Therefore, it's difficult
> to decide what is buggy.
> 
> This is an experimental implementation for looking for bugs of current D3E
> draft. So, you don't need to worry about the quality of this implementation
> for now.
> 
> Thank you.

Thanks :) We'll stay out of this for now then.

Comment 31

[Kohei Yoshino]

This was missing in the site compat doc. Just added.
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility
https://twitter.com/FxSiteCompat/status/464760601508474881

Comment 32

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

Thank you, Kohei-san.

However, KeyboardEvent.key is still unstable. I don't recommend any developers to use it in this stage.

We need to change some key values of non-printable keys, see bug 900372.

And also we need to support/change dead key implementation.

Comment 33

[Kohei Yoshino]

Okay, I have just modified the copy. I believe the compat info is still needed because this is a public API, not behind a pref.
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#KeyboardEvent.key_values_have_been_changed