1458845 - Unity/WebGL keyboard input doesn't support ALT modifier

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P2)

Description

[vigo]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180502220059

Steps to reproduce:

1) load https://bloodyapp.sealmedia.de/webgl/seal.html (WebGL version of a Unity game)
2) if this is your first time playing, or after an explicit logout from inside the game, there's a "register / login" mask displayed, asking for an email
3) try to enter an email (e.g. [e-mail])

This is on macOS 10.13.4, with an Apple keyboard, and a current Nightly, with all addons disabled. It works with regular FF and Chrome.


Actual results:

Step 3) doesn't work, since the @ cannot be successfully keyed in - the resulting text is "userdomain.de"


Expected results:

Step 3) should work, resulting in "[e-mail]"

Comment 1

[vigo]

This problems affects all textual input inside the game, for any character requiring the ALT modifier on that keyboard. Important: It's a QWERTZ-layout (Germany), so that's e.g. ALT + L = "@", ALT + E = "€", ALT + SHIFT + 7 = "\".

With "US" keyboard layout (SHIFT + 2 = "@"), entering the email works. Sorry for the possible confusion.

Comment 2

[Cristian Craciun]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180507222648

I have managed to reproduce this issue on the latest Nightly build 62.0a1/2018-05-07 and on the Nightly build 61.0a1/2018-05-06, using macOS 10.13.2 (64 bit). 

I've also tested this on the latest release build (59.0.2) and the beta build (60.0) using the same macOS and didn't managed to reproduce the issue.

Please have in mind that the issue occurs only on the QWERTZ-layout (Germany). 

Based on the pushlog: 
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1e6febc9f5af217779860536398cd1c60122e2e1&tochange=c19f2cea4987db5855c65b7128f10643a4eef99b
the issue may be caused by the bug 1440189. I've added Masayuki Nakano for more info. 

Based on the Description + Comment 1 and the tests executed, I will mark this bug as NEW.

Comment 3

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

TextInputHandler dispatches keypress event from 2 paths now. One is from TextInputHandler::InsertText() when IME is enabled or in password fields. The other is from TextInputHandler::HandleKeyDownEvent() otherwise. In the former case, we unset altKey since our editor cannot handle keypress events as inputting character if one or more of ctrlKey, altKey and metaKey is true. However, in the latter case, we don't do this hack since editor won't handle the keypress event. So, this means that we dispatched different keypress event only on macOS when a printable key is pressed with Alt key.

I think that we should avoid this inconsistent keypress event state in widget level rather than just dispatching keypress event even when altKey is true on macOS.

Comment 4

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

Ah, but if we change the behavior in widget level, web contents cannot receive keypress events whose altKey is true on macOS. For compatibility with Chrome, we should ignore altKey only on macOS.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc04b47cabddad705baeb5bcd0081ac3c685fed

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef3e84e392ef842112dcba7bad17665b0f4f2a25

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=585f5ecd3a883ab2f346dc5ece3b3c1d29d42bc3

Comment 8

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

Attachment: Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

On macOS, option key is mapped to alt key, and it works like AltGr key on
the other platforms.  Since our editor doesn't accept keypress events as
typing printable character if one of altKey, ctrlKey and metaKey of the
events is true, widget for macOS sets those attributes to false.  However,
if no editor has focus, our widget does not do that since it's same
behavior as the other browsers.  Therefore, we need to keep setting altKey
of keypress events to true if no editor has focus but we need to dispatch
keypress events even on content unless the charCode is not 0.

So, only on macOS, WidgetKeyboardEvent::IsInputtingText() does not need
to check altKey state.

Review commit: https://reviewboard.mozilla.org/r/246310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246310/

Comment 9

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

https://reviewboard.mozilla.org/r/246310/#review252506

::: commit-message-21f09:1
(Diff revision 1)
> +Bug 1458845 - Make dispatch keypress event even if altKey is true on macOS r?smaug

Oops, I meant Make TextEventDispatcher dispatch keypress event...

I'll modify it before landing.

Comment 10

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

https://reviewboard.mozilla.org/r/246310/#review252672

I think I'd like to see some clarifications to the comments, and actually understand what is the behavior on other browsers on MacOS.

::: widget/TextEvents.h:236
(Diff revision 1)
>    bool IsInputtingText() const
>    {
>      // NOTE: On some keyboard layout, some characters are inputted with Control
>      //       key or Alt key, but at that time, widget unset the modifier flag
> -    //       from eKeyPress event.
> +    //       from eKeyPress event on Windows and Linux.  On the other hand,
> +    //       on macOS, Alt key is unset only when an editor has focus.  This

I'm having trouble to parse this
"  This
    //       inconsistent behavior makes fortunately our behavior same as
    //       the other browsers on macOS only when no editor has focus.  So,
    //       for compatiblity with both older Gecko and the other browsers,
"

So other browsers don't care about editor having focus or not either? Or what?
What events are dispatched when?
If editor has focus and users types something with alt set, which events are dispatched? what if editor doesn't have focus?
Do we do the same thing as other browsers?

::: widget/tests/test_keypress_event_with_alt_on_mac.html:73
(Diff revision 1)
> +       kDescription + "'a' key press with shift key should cause firing keypress event");
> +    ok(keypress.shiftKey,
> +       kDescription + "shiftKey of 'a' key press with shift key should be true");
> +
> +    // When a key inputs a character with option key, we need to unset altKey for our editor.
> +    // Otherwise, altKey should be true for compatibility with the other browsers.

Also here. Are we compatible with other browsers only when we aren't inputting to editor?

Comment 11

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

https://reviewboard.mozilla.org/r/246310/#review252672

Sure. I rewrite all comments around here to explain current standard behavior, and what our widget do and its reason.

On the other browsers, modifier attributes represent actual key input as-is. I.e., any modifiers won't be cleared.  Note that clearing the modifiers only when editor has focus is bug of us. See bug 1346832.

Anyway, I investigated more, printable keys with altKey works as expected. altKey won't set to false and keypress event is fired anytime except when key combination doesn't input printable character.

Note that I found odd behavior with control key, I filed bugs for them:
https://bugs.webkit.org/show_bug.cgi?id=185971
https://bugs.chromium.org/p/chromium/issues/detail?id=846556

> I'm having trouble to parse this
> "  This
>     //       inconsistent behavior makes fortunately our behavior same as
>     //       the other browsers on macOS only when no editor has focus.  So,
>     //       for compatiblity with both older Gecko and the other browsers,
> "
> 
> So other browsers don't care about editor having focus or not either? Or what?
> What events are dispatched when?
> If editor has focus and users types something with alt set, which events are dispatched? what if editor doesn't have focus?
> Do we do the same thing as other browsers?

> So other browsers don't care about editor having focus or not either? Or what?

Looks like that both Chrome and Safari tries to keep consistency between those situations, and they succeeded to do that at least simply only when altKey is pressed. However, according to my bug reports mentioned above, they also use different path to dispatch keypress event. Perhaps, this is caused by design of cocoa API.  Browsers need to send received native keydown events to IME manually.  Then, insertText: may be called.  However, if IME is disabled, shouldn't send keydown events to IME.  Therefore, insertText: won't be called.

> What events are dispatched when?

Unless option + a printable key does not input any character (e.g., a lot of keys with option key on Arabic - PC keyboard layout won't input any character), keypress event should be fired even when altKey is true.

> If editor has focus and users types something with alt set, which events are dispatched? what if editor doesn't have focus?

Only on Gecko, altKey of keypress event is set to false (by widget) only when IME is enabled (i.e., editor has focus and it's not password field and does not have |ime-mode: disabled;|).  This is done by insertText: callback of cocoa.  Therefore, when IME is disabled, this won't be called and we won't clear altKey state.

> Do we do the same thing as other browsers?

We should do except the reporting bugs (with control key cases).

> Also here. Are we compatible with other browsers only when we aren't inputting to editor?

Even after landing this patch, yes it is. We need to fix bug 1346832. Do you think that I should give the fix higher priority? If so, I'll work on it soon. Perhaps, we can do that simply (but needs to hack all widgets).

Comment 12

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246310/diff/1-2/

Comment 13

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #11)
> > Also here. Are we compatible with other browsers only when we aren't inputting to editor?
> 
> Even after landing this patch, yes it is. We need to fix bug 1346832. Do you
> think that I should give the fix higher priority? If so, I'll work on it
> soon. Perhaps, we can do that simply (but needs to hack all widgets).

Ah, but I think that we shouldn't do that until fixing bug 968056 completely because if we set altKey and ctrlKey to keypress event, web apps cannot distinguish whether each keypress event is redundant or not. Distinguishing it may be important for making web apps aware of both current Fireofox and Nightly builds without version number. So, I strongly recommend that we should not fix it now.

Comment 14

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

https://reviewboard.mozilla.org/r/246310/#review252850

Sorry, this still isn't clear to me.

::: widget/TextEvents.h:250
(Diff revision 2)
> +    // NOTE: There are some complicated issues of our traditional behavior.
> +    //       On Windows, KeyboardLayout::WillDispatchKeyboardEvent() clears
> +    //       MODIFIER_ALT and MODIFIER_CONTROL of eKeyPress event if it
> +    //       should be treated as inputting a character because AltGr is
> +    //       represented with both Alt key and Ctrl key are pressed, and
> +    //       some keyboard layout may produces a character with Ctrl key.

s/produces/produce/

::: widget/tests/test_keypress_event_with_alt_on_mac.html:51
(Diff revision 2)
> +  await SpecialPowers.pushPrefEnv({"set": [
> +          ["dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content", true],
> +        ]});
> +  const kTests =
> +    [ { target: "input", isEditable: true },
> +      { target: "password", isEditable: true },

So per the documentation in the code type="password" is handled specially, but based on the test it is like type="text".
I'm lost here now.

::: widget/tests/test_keypress_event_with_alt_on_mac.html:83
(Diff revision 2)
> +       kDescription + "altKey of 'a' key press with option key should be " + !kTest.isEditable);
> +
> +    keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {altKey: true, shiftKey: true}, "\u00C5", "A");
> +    ok(keypress,
> +       kDescription + "'a' key press with option key  and shift key should cause firing keypress event");
> +    is(keypress.altKey, !kTest.isEditable,

... so per documentation this should fail when type=password.

::: widget/tests/test_keypress_event_with_alt_on_mac.html:92
(Diff revision 2)
> +
> +    keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {ctrlKey: true}, "\u0001", "a");
> +    ok(!keypress,
> +       kDescription + "'a' key press with control key should not cause firing keypress event");
> +
> +    keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {altKey: true, ctrlKey: true}, "\u0001", "a");

Or maybe the type=password case is about this?

Comment 15

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

https://reviewboard.mozilla.org/r/246310/#review252850

> So per the documentation in the code type="password" is handled specially, but based on the test it is like type="text".
> I'm lost here now.

Oh, good point. I misremembered that.

https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/widget/cocoa/TextInputHandler.mm#1756,1760
IsIMEEnabled() here means IME is enabled in normal editor as its name explains.
Additionally, we send given native keydown to Cocoa if IsASCIICapableOnly() is true. This means that IME is disabled for password field or |ime-mode: disabled;|. So, we send key events to cocoa if an editor has focus. And if IME is actually enabled, Cocoa sends the key events to IME, but anyway, insertText: will be called by Cocoa.

So, my comment is wrong. I'll update it.

Comment 16

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246310/diff/2-3/

Comment 17

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

Comment on attachment 8980239 [details]
Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS

https://reviewboard.mozilla.org/r/246310/#review253262

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8128097dd7a7
Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS r=smaug

Comment 19

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8128097dd7a7

Comment 20

[Ciprian Georgiu, Desktop QA]

Reproduced this bug on an affected Nightly build (2018-05-03), using the STR from comment 0 and the additional info from comment 1.

Verified fixed on Beta 62.0b10 (20180719140244) running macOS 10.13.