951023 - Gonk widget should set modifier state at dispatching events derived from WidgetInputEvent

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

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

Similar to bug 951021, Gonk widget should set modifier state as far as possible.

Comment 1

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

Attachment: Patch

I don't test this because I don't have devices which run FxOS.

This patch is similar to bug 951021's patch, therefore, I guess this work well.

tryserver build:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7258700fd945

Comment 2

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

test pages:
https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html
https://bugzilla.mozilla.org/attachment.cgi?id=640488

Comment 3

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

Comment on attachment 8348508 [details] [diff] [review]
Patch

>+    event.modifiers = mData.DOMModifiers();
>+    // When dispatching keypress event should cause text input, Alt, Ctrl and
>+    // Meta states must not be set.
>+    // XXX According to Android 4.4's behavior, we might not need to do this.
>+    if (aEventMessage == NS_KEY_PRESS &&
>+        event.charCode && mChar != mUnmodifiedChar) {
>+        event.modifiers &= ~(MODIFIER_ALT | MODIFIER_CONTROL | MODIFIER_META);
>+    }

According to nchen's comment at bug 951021 comment 3, this hack isn't necessary on Gonk.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 8348508 [details] [diff] [review]
Patch

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

Mostly makes sense, though we should probably test this before landing.

::: widget/gonk/nsAppShell.cpp
@@ +288,3 @@
>      PRUnichar PrintableKeyValue() const;
>  
> +    int32_t UnModifiedMetaState() const

nit: UnmodifiedMetaState() looks better to me.

@@ +313,2 @@
>  
> +    if (mKeyCharMap.get()) {

if (!mKeyCharMap.get()) return;

Just have to make sure mChar and mUnmodifiedChar are initialized to some value.

@@ +314,5 @@
> +    if (mKeyCharMap.get()) {
> +        mChar = mKeyCharMap->getCharacter(mData.key.keyCode, mData.metaState);
> +        if (mChar < ' ') {
> +            mChar = 0;
> +         }

Odd indentation

@@ +338,3 @@
>          return 0;
>      }
> +    if (mChar >= ' ') {

Due to the logic in the KeyEventDispatcher constructor, I think we can just do return mChar ? mChar : mUnmodifiedChar;

Comment 5

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

Attachment: Patch

Sorry, I forgot to post this new patch.

Changes from the previous patch:

* Removing the code of adjusting modifiers for printable keys because I guess it is not necessary on Android 4.x. See comment 3.
* Adding IsControlChar() instead of comparing with ASCII whitespace.
* Removing CharCode() because mChar now stores it.

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 8356060 [details] [diff] [review]
Patch

Looks good. If you haven't tested this on a device though, I'll try to test it on a device some time this week.

Comment 7

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

(In reply to Michael Wu [:mwu] from comment #6)
> Comment on attachment 8356060 [details] [diff] [review]
> Patch
> 
> Looks good. If you haven't tested this on a device though, I'll try to test
> it on a device some time this week.

Yes, I've not tested the patch. I'd like you to test it. Do you want to test it before landing? Or you will test it on landed build and file followup bugs?

Comment 8

[Michael Wu [:mwu]]

I'll test it before landing, and then mark check-in needed if things look ok.

Comment 9

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

Okay, thank you for your testing!

Comment 10

[Michael Wu [:mwu]]

WFM

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9ff749eca0c0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ff749eca0c0