Unity/WebGL keyboard input doesn't support ALT modifier

VERIFIED FIXED in Firefox 62

Status

()

defect
P2
normal
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: vigo.von.harrach, Assigned: masayuki)

Tracking

({regression})

61 Branch
mozilla62
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 disabled, firefox62 verified)

Details

Attachments

(1 attachment)

Reporter

Description

a year ago
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. user@domain.de)

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 "user@domain.de"
Reporter

Comment 1

a year ago
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.
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Events
Ever confirmed: true
Flags: needinfo?(masayuki)
Keywords: regression
OS: Unspecified → Mac OS X
Product: Firefox → Core
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.
Assignee: nobody → masayuki
Blocks: 1440189
Status: NEW → ASSIGNED
Component: DOM: Events → Widget: Cocoa
Flags: needinfo?(masayuki)
Hardware: Unspecified → All
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.
Component: Widget: Cocoa → DOM: Events
Priority: -- → P2
Comment hidden (mozreview-request)
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 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?
Attachment #8980239 - Flags: review?(bugs) → review-
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 hidden (mozreview-request)
(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 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?
Attachment #8980239 - Flags: review?(bugs) → review-
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 hidden (mozreview-request)
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
Attachment #8980239 - Flags: review?(bugs) → review+

Comment 18

a year ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8128097dd7a7
Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS r=smaug

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8128097dd7a7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.