1245428 - KeyboardEvent.shiftKey, .altKey, .metaKey are wrong when the focus is on an input field

Status: NEW

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Francesco]

Attachment: test.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160120164649

Steps to reproduce:

I listen to onkeydown, onkeyup events.
Whenever an event is triggered, I look at the value of event.shiftKey, event.altKey, ...

I try to press and release the key "shift"


Actual results:

When an input field is focused, the "keydown" event reports that event.shiftKey is false, and the "keyup" event reports that event.shiftKey is true.

The problem can be replicated on FF45 and in the current Nightly.


Expected results:

event.shiftKey should be 'true' in the keydown event and 'false' in the keyup event.

(this is the correct behaviour, that is properly triggered when no input field is focused).

Comment 1

[Loic]

WFM with the latest Nightly on Win 7

Comment 2

[Andrew Overholt [:overholt]]

Seems to work for me in latest nightly on Win 10, too.

Maybe a Linux-specific thing?

smaug?

Comment 3

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

masayuki, could you take look?

Comment 4

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

Odd, I've already fixed the bug with karlt:
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsGtkKeyUtils.cpp?rev=4a2797061277&mark=892-904#883

> 883     // NOTE: The state of given key event indicates adjacent state of
> 884     // modifier keys.  E.g., even if the event is Shift key press event,
> 885     // the bit for Shift is still false.  By the same token, even if the
> 886     // event is Shift key release event, the bit for Shift is still true.
> 887     // Unfortunately, gdk_keyboard_get_modifiers() returns current modifier
> 888     // state.  It means if there're some pending modifier key press or
> 889     // key release events, the result isn't what we want.
> 890     guint modifierState = aGdkKeyEvent->state;
> 891     GdkDisplay* gdkDisplay = gdk_display_get_default();
> 892     if (aGdkKeyEvent->is_modifier && GDK_IS_X11_DISPLAY(gdkDisplay)) {
> 893         Display* display =
> 894             gdk_x11_display_get_xdisplay(gdkDisplay);
> 895         if (XEventsQueued(display, QueuedAfterReading)) {
> 896             XEvent nextEvent;
> 897             XPeekEvent(display, &nextEvent);
> 898             if (nextEvent.type == keymapWrapper->mXKBBaseEventCode) {
> 899                 XkbEvent* XKBEvent = (XkbEvent*)&nextEvent;
> 900                 if (XKBEvent->any.xkb_type == XkbStateNotify) {
> 901                     XkbStateNotifyEvent* stateNotifyEvent =
> 902                         (XkbStateNotifyEvent*)XKBEvent;
> 903                     modifierState &= ~0xFF;
> 904                     modifierState |= stateNotifyEvent->lookup_mods;
> 905                 }
> 906             }
> 907         }
> 908     }

It seems that some condition are not expected in some envrionments... Unfortunately, I don't have much time to work on this, but I'll try to check this when I have time... E.g., waiting reviews of patches.

Comment 5

[Francesco]

Just one comment: with debian unstable (gtk 3.18.7) everything is working well.

The problem was triggered on Ubuntu 15.10, with all the extensions/themes removed.
I tested two independent installations of Ubuntu.

Comment 6

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

I guess that there are a couple of possible causes:

1. The trigger is the behavior change of GTK/GDK or X11 but they are reverted in the newer version.
2. The trigger is the default IME of Ubuntu.
3. The trigger is customize of something by Ubuntu.

Does somebody test these suspicion?

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> I guess that there are a couple of possible causes:
> 
> 1. The trigger is the behavior change of GTK/GDK or X11 but they are
> reverted in the newer version.

This issue may be x11 because this occurs on GTK2 too.  It seems that XEventsQueued returns 0.

Comment 8

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)

> 2. The trigger is the default IME of Ubuntu.

Even if Debian/sid, this occurs with fcitx.  If GTK_IM_MODULE is empty, this doesn't occur.
This issue is depends on im config.  (Ubuntu's default is ibus. And I can reproduce on fcitx)

> 3. The trigger is customize of something by Ubuntu.

This occurs on Debain/sid too.

Comment 9

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

Hmm, that's too bad, sigh.

karlt: do you have any idea to hack this issue?

Comment 10

[Karl Tomlinson (:karlt)]

It depends on how fcitx is changing the events in the event queue.

If fcitx is popping the XKBEvent off the queue before Gecko looks at it, then perhaps Gecko could peek at the event earlier.

If however fcitx is reordering events by processing asynchronously and then putting some of the events back in the queue in a way that information is lost, then it's probably not practical to change the conventions to those expected here.

Comment 11

[Makoto Kato [:m_kato]]

This occurs ibus too.  uim is no problem.

Nakano-san, could you use XkbGetState() if queue is empty()?

Comment 12

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

(In reply to Makoto Kato [:m_kato] from comment #11)
> This occurs ibus too.  uim is no problem.
> 
> Nakano-san, could you use XkbGetState() if queue is empty()?

Yeah, I'll try to do it later. Perhaps, I cannot work on this today, though.

Comment 13

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

(In reply to Makoto Kato [:m_kato] from comment #11)
> This occurs ibus too.  uim is no problem.
> 
> Nakano-san, could you use XkbGetState() if queue is empty()?

Almost works, but not perfectly. Sometimes, modifier state isn't updated at keydown. I keep investigating...

Comment 14

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

I see what happens.

First, we receive modifier key event normally. At this time, XkbStateNotify event is in the queue. Then, we send the key event to IME. IME sends duplicated key event synchronously. I.e., keypress event is nested. In this time, XkbStateNotify event has gone from the queue. Then, we dispatch DOM keydown event with the nested event and the first key event is consumed by IME.

Comment 15

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

Oops, I'm wrong. The events are not nested, unfortunately, posted to the queue by IME. Therefore, I found following 2 cases :-(

1. normal Shift keypress event is consumed by IME
2. normal Shift keyreleae event is consumed by IME
3. posted Shift keypress event comes without XkbStateNotify event
4. posted Shift keyrelease event comes without XkbStateNotify event

1. normal Shift keypress event is consumed by IME
2. posted Shift keypress event comes without XkbStateNotify event
3. normal Shift keyreleae event is consumed by IME
4. posted Shift keyrelease event comes without XkbStateNotify event

Fixing the former case is very difficult...

Comment 16

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

I'm still thinking what approach is the best but I'm still not sure.

1. I think that if we fix this bug ideally, we should store consumed shortcut key events and modified modifier state until coming the posted event. However, it's difficult to discards the cache of key events which are completely consumed by IME.

2. Update GdkEventKey::state before sending IME. However, this *might* cause odd behavior of IME.

3. Dispatch modifier keydown/keyup events even if it's consumed by IME but IME does nothing actually. After that, we should ignore posted events. At least tested with iBus on Ubuntu, GdkEventKey::time isn't modified at posting. So, we can ignore older key events simply.

It seems that we should fix this bug by the #2 approach temporarily because the patch is the smallest. But we should add a pref to disable the new behavior. Then, even if somebody would meet regression in release builds, they can disable the fix by themselves.

However, in the future, we need to use the approach #3 because we probably need to dispatch keydown and keyup events even during composition. Then, we need to dispatch keyboard events with normal keyboard events. But posted events will cause redundant (duplicated) keyboard events.

Comment 17

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

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

Comment 18

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

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

Comment 19

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

Attachment: part.1 Should log GdkEventKey with more information

Currently, we use unified cpp file. So, we should define/implement logging utilities in a file. So, I think that GtkLoggingUtils.(h|cpp) is better place.

This patch logs whole members of GdkEventKey struct for checking what ibus and fcitx do.

Comment 20

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

Attachment: part.2 Update GdkEventKey::state to the new modifier state which includes the result of the event when nsWindow receives a key press or key release event is received

Let's take the approach #2 of comment 16 for now.

Of course, modified modifier state *might* cause IME confused. However, I think that it's very rare case because I have no idea the reason why IME needs to check modifier state of modifier key event, especially, the state hasn't been updated.

So, I believe that this is enough safe.

Comment 21

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

Attachment: part.3 Add a pref to disable updating modifier state before sending key events to IME

However, we should provide last resort of users if the hack would cause some trouble with some IME.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8721310 [details] [diff] [review]
part.3 Add a pref to disable updating modifier state before sending key events to IME

>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* vim: set ts=4 et sw=4 tw=80: */

The style guide requests 2 space indentation.
Please use that for new files.

>+ToChar(GdkEventType aType)
>+{
>+    switch (aType) {

There is actually already a method to do this.  I think it would be better to
use it to implement this function, as it already has all the static strings
and will return the appropriate string even if Gecko is compiled against an
older library.  I think something like this should work:

  auto enumClass =
    G_ENUM_CLASS(g_type_class_peek_static(GDK_TYPE_EVENT_TYPE));
  GEnumValue* enumValue = g_enum_get_value(enumClass, aType);

  return enumValue->value_name;

>+
>+private:
>+    // Hide unnecessary constructor.
>+    GdkEventKeyToString() {}

Please remove these lines.
The implicitly-declared default constructor is not provided because there are
user-declared constructors.  Base class constructors are not inherited.

>+const char* ToChar(bool aBool) { return aBool ? "true" : "false"; }

This is sometimes called with an integer parameter and conversion to bool can
happen for other types also, so it would be nice to make it clear from the
function name that the returned string represents a bool.

I would suggest "CStringForBool".

That also makes it clear that it is not a single char ('Y'/'N'/'T'/'F')
returned, but a pointer to a char array.

Or perhaps you may prefer "char CharForBool(bool aBool)", for more
concise output,
or even just "%d", (aInputEvent.modifiers & MODIFIER_SHIFT) != 0.

>+const char* ToChar(GdkEventType aType);

Perhaps "ToCString", so as not to imply returning a single char.

>+
>+class GdkEventKeyToString final : public nsAutoCString

Perhaps "nsCStringForGdkEventKey", to clarify how the type behaves,
distinguish from char*, and clarify that it is not using wide chars.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8721309 [details] [diff] [review]
part.2 Update GdkEventKey::state to the new modifier state which includes the result of the event when nsWindow receives a key press or key release event is received

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #20)
> Let's take the approach #2 of comment 16 for now.
> 
> Of course, modified modifier state *might* cause IME confused. However, I
> think that it's very rare case because I have no idea the reason why IME
> needs to check modifier state of modifier key event, especially, the state
> hasn't been updated.
> 
> So, I believe that this is enough safe.

The risk is that an IME might use the modifier state to detect when two
shift keys are pressed together.  With this change, pressing one shift key
would look like two shift keys.

Isn't this bug safe enough not to fix?
Is it causing a real problem?

My opinion is that these IME bugs are ones that are not worth working around.
The fact that the fix causes a similar bug for IMEs that don't have this
problem reinforces that opinion.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8721305 [details] [diff] [review]
part.1 Should log GdkEventKey with more information

Comment 22 was intended as a review for this patch, sorry.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8721309 [details] [diff] [review]
part.2 Update GdkEventKey::state to the new modifier state which includes the result of the event when nsWindow receives a key press or key release event is received

My opinion is that the risk taking this workaround for an IME bug is at least as great as the risk of living with the IME bug, and so I don't think we should do this without a real problem to solve.

Comment 26

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

Moving all open keyboard/IME handling bugs to DOM: UI Events & Focus Handling component.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 28

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

Not a regression.