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

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.

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

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

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.

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.

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.

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 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 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)

(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 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 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.

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.

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

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 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 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 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 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?

(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>.

(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 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!"

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

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

Is there anything Firefox Desktop QA can help with here?

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.

(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.

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

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.

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