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

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(4 keywords)

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(7 attachments, 4 obsolete attachments)

3.74 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.96 KB, patch
smaug
: review+
jimm
: review+
Details | Diff | Splinter Review
4.79 KB, patch
smaug
: review+
karlt
: review+
Details | Diff | Splinter Review
5.54 KB, patch
smichaud
: review+
smaug
: review+
Details | Diff | Splinter Review
6.12 KB, patch
mwu
: review+
Details | Diff | Splinter Review
5.58 KB, patch
mwu
: review+
smaug
: review+
Details | Diff | Splinter Review
13.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Summary: Implement KeyboardEvent.key for printable keys → Implement KeyboardEvent.key for printable keys (except dead key handling)
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.
Attachment #8340321 - Flags: review?(bugs)
On Windows, NativeKey::mCommittedCharsAndModifiers stores a key value which we want for KeyboardEvent.key.
Attachment #8340322 - Flags: review?(jmathies)
Attachment #8340322 - Flags: review?(bugs)
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.
Attachment #8340327 - Flags: review?(nchen)
Attachment #8340327 - Flags: review?(bugs)
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.
Attachment #8340329 - Flags: review?(mwu)
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.
Attachment #8340330 - Flags: review?(mwu)
Attachment #8340330 - Flags: review?(bugs)
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".
Depends on: 938987
Attachment #8340322 - Flags: review?(jmathies) → review+
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.
Attachment #8340329 - Flags: review?(mwu) → review+
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)
Attachment #8340327 - Flags: review?(nchen) → review+
(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.
Attachment #8340326 - Flags: review?(karlt) → review+
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.
Attachment #8340330 - Flags: review?(mwu)
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.
Attachment #8340325 - Attachment is obsolete: true
Attachment #8343056 - Flags: review?(smichaud)
Attachment #8343056 - Flags: review?(bugs)
Okay, then, all attributes of KeyboardEvent should be computed in KeyEventDispatcher.
Attachment #8340329 - Attachment is obsolete: true
Attachment #8343098 - Flags: review?(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.
Attachment #8343098 - Flags: review?(mwu) → review+
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?
Attachment #8343056 - Flags: review?(smichaud) → review+
Attachment #8340321 - Flags: review?(bugs) → review+
Attachment #8340322 - Flags: review?(bugs) → review+
Attachment #8340326 - Flags: review?(bugs) → review+
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.
Attachment #8340327 - Flags: review?(bugs) → review-
Attachment #8343056 - Flags: review?(bugs) → review+
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.
Attachment #8343101 - Flags: review?(mwu) → review+
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?
Attachment #8343101 - Flags: review?(bugs)
(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>.
Attachment #8343101 - Flags: review?(bugs) → review+
(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!"
Attachment #8344324 - Flags: review?(bugs) → review+
Is there anything Firefox Desktop QA can help with here?
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
(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.
Whiteboard: [qa-]
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
You need to log in before you can comment on or make changes to this bug.