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).

WFM with the latest Nightly on Win 7

Seems to work for me in latest nightly on Win 10, too. Maybe a Linux-specific thing? smaug?

masayuki, could you take look?

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.

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.

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?

(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.

(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.

Hmm, that's too bad, sigh. karlt: do you have any idea to hack this issue?

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.

This occurs ibus too. uim is no problem. Nakano-san, could you use XkbGetState() if queue is empty()?

(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.

(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...

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.

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...

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.

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.

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.

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

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 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 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 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.