768287 - keydown event's getModifierState() returns wrong value on GTK if the key has lockable modifier but the keyval doesn't correspond to a DOM modifier

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

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

bug 630817 code has been applied to lockable modifiers like CapsLock, NumLock and ScrollLock in bug 630811. Therefore, getModifierState() of keydown events of the keys always return true.

I found this bug at testing.

Comment 1

[Karl Tomlinson (:karlt)]

If you are referring to the situation where, for example, the CapsLock key is pressed to release the lock modifier, then I don't think there is a bug here because the lock modifier is not deactivated until the key is released.

For example,

1. Press and release CapsLock to enable the CapsLock modifier.

   Subsequent regular key presses have the lock modifier set.

2. Press and hold the CapsLock key down.

3. While the CapsLock key is still down, press and release a letter key.

   The events for the letter key press and release still have the lock modifier set.

4. Release the CapsLock key.

   Subsequent regular key presses have the lock modifier unset.

Comment 2

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

Sorry for the too simple my bug reporting, but I'm still investigating. I found following issues at least.

1. At unlocking the modifier, GDK_KEY_PRESS's state indicates still locking the modifier. This is different from other platforms.

2. Even if unshifted position of lockable modifier isn't a DOM modifier, we set the lockable modifier to the DOM keydown event for the unshifted GDK_KEY_PRESS event.

3. On Japanese keyboard layout, CapsLock key is in shifted position of the left key of 'A'. During pressing Shift+CapsLock, CapsLock isn't locked and GDK_KEY_RELEASE of CapsLock must be happen before Shift's GDK_KEY_RELEASE. Otherwise, the CapsLocking is canceled. But we're setting DOM modifier for it always. And during pressing Shift+CapsLock, letter keys always input lower cases.

4. Pressing NumLock key, always the keys in Numpad works as NumLocked.

I think that #1 is not a fixable issue. So, we just ignore the case.

#2 must be fixed, we shouldn't set wrong DOM modifier in this case.

According to #3 and #4, locking on Linux is performed at GDK_KEY_RELEASE event or during pressing the modifier key. So, it's not bad to set DOM modifier during pressing as karlt said.

However, it's not good for web application developers. So, anyway, we should document the fact on Linux in MDN.

I'll post a patch for #2.

Comment 3

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

Attachment: Patch

Just adding the keyval check if the keyval is actually mapped to DOM modifier key.

Comment 4

[Karl Tomlinson (:karlt)]

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

Sorry for the delay here.  The keyboard handling can be quite hard to understand.

The GetModifierKey(guint aHardwareKeycode) method is only used here, but the bug that this patch is fixing suggests that what is really wanted here is a GetModifierKey(guint aKeyval) method.
Or better GetModifierMaskForKeyval(guint AKeyval).
Would that work?

Comment 5

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

(In reply to Karl Tomlinson (:karlt) from comment #4)
> The GetModifierKey(guint aHardwareKeycode) method is only used here, but the
> bug that this patch is fixing suggests that what is really wanted here is a
> GetModifierKey(guint aKeyval) method.

GetModifierKey(guint aHardwareKeycode) is also used here:
http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsGtkKeyUtils.cpp#401

The ModifierKey struct which is returned by GetModifierKey() stores the relation of hardware keycode and modifier flag activated or deactivated by the physical key. So, using GDK keyval for the param doesn't make sense.

> Or better GetModifierMaskForKeyval(guint AKeyval).

I don't understand what you meant here... GetModifierForGDKKeyval() returns one of modifiers which are supported by KeymapWrapper.

In the example in comment #2, GDK_EisuToggle key event is fired by CapsLock key without Shift. Even in this case, aGdkKeyEvent->is_modifier is true. Then, we activate CapsLock state of the DOM key event. This is what I'm trying to fix in this bug. The keyval doesn't indicate one of modifier keys, we shouldn't set the DOM modifier key flag since GTK doesn't change the actual modifier state too.

Comment 6

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/f7b3889f16cc was really the origin
of this bug when it changed from using keyvals(= keysyms) to keycodes when
trying to guess which modifier might be activated by the keypress.
However, the issue wasn't visible to the DOM until bug 630811.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)

This bug, which occurs with Eisu_Toggle and CapsLock at different levels on
the same key, is an example demonstrating that a keycode does not necessarily
always map to a single modifier (nor single modifier bitmask even).

The XModifierKeymap returned from XGetModifierMapping() gives a mapping from
hardware KeyCode to modifier bitmask.  XModifierKeymap is an old structure
from the time when X11 modifiers were dependent solely on the keycode.

Since around the year 1996, the "X Keyboard Extension" has meant that the
effects of key events on modifiers now more closely follow the keysym (which
is equivalent to GDK's keyval).  i.e. the effect on modifiers depends on the
keycode, level, and group.
http://www.x.org/releases/current/doc/kbproto/xkbproto.html#Key_Actions

So a mapping from KeySym to modifier (or to modifier bitmask) is what is
wanted here but that is not readily available.  XModifierKeymap does not
provide it.  XkbModAction
http://www.x.org/releases/current/doc/libX11/XKB/xkblib.html#Actions_for_Changing_Modifiers_State
would provide the right info much of the time, but
mirroring the X server mapping would be complicated, and would still not give
us enough info some of the time:

  Release effects depend on whether "no keys were operated simultaneously"

  "Actions which cause an event from another key or from a button on another
   device immediately generate the specified event. These actions do not
   consider the behavior or actions (if any) that are bound to the key or
   button to which the event is redirected."
  So reverse lookup of the actions for the key event could find actions that
  are not applied because the key event doesn't identify the original key.

  If there are multiple keyboards attached to the computer, I'm not sure, but
  it looks like the actions could depend on the particular keyboard on which
  the key was pressed, and we don't know which keyboard that was."

> GetModifierKey(guint aHardwareKeycode) is also used here:
> http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsGtkKeyUtils.
> cpp#401

GetModifierKey is used there just to initialize the mapping from hardware
keycode to modifier bitmask, but that mapping is not what we want/need.

Comment 7

[Karl Tomlinson (:karlt)]

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

I'm not happy with this fix because it uses a heuristic that happens to work
in this situation, but it doesn't directly address the problem and there could
just as easily be different DOM modifiers on different levels, in which case
the heuristic would not help us.  NumLock, CapsLock, ScrollLock, Mode_switch,
and ISO_Level3_Shift may all be modifiers where two of them could be the same
key.  There are also layout options where Shift+Shift is CapsLock, so Shift_L
and CapsLock are on different levels of the same key.

I don't want this code to turn into a cluster of special cases to handle each
situation that we find in practice.

In bug 630813 comment 34, I said I feared that trying to interpret/guess the
effect of keys on modifier state is just going to be too difficult.
But I think there are up to 3 options here:

1. Don't try to guess.  Just leave the state reflecting the state at the time
   the key was pressed, and get the spec to recognize that this differs on
   different platforms.

  This behavior could even be useful for detecting whether one Shift key was
  already down when the other Shift was pressed, but other platforms would
  need to match the behavior for this to be truly useful.

2. Perhaps there is a simple implementation that can get things right most of
   the time without needing special cases, and we are happy enough with that.

  ComputeDOMKeyCode already has logic to handle this Eisu_Toggle key.  If this
  code that tweaks the modifier bits were moved to after the InitKeyEvent()
  call, then perhaps modifiers of the nsKeyEvent can be tweaked based on
  aKeyEvent.keyCode?  This would still fail if someone had, for example,
  separate Alt_L and Meta_L keys that activate the same modifier bit, but I
  don't think that is an important case to get right.

3. Use Xkb events to get this correct in all situations.

Comment 8

[Karl Tomlinson (:karlt)]

For option 3, it is possible to listen for Xkb events that notify when
modifier state changes.

These arrive after the key event of course.  But on receipt of the key event
it is possible to look through the event queue to see if there is subsequent
XkbStateNotify event.  Experiments here suggest that it will be the
immediately following event (if any), which makes it easy to check for with
XEventsQueued(display, QueuedAfterReading) and XPeekEvent().

These events are sent even when the application doesn't have focus, which
perhaps could cause unnecessary wakeups, but this is probably of minor concern
compared with other activity in a javascript app.  At least it would only add
activity to the computer while it is being actively used.

These are the relevant parts of Xkb library interface:

XkbQueryExtension()
XkbSelectEventDetails()
XkbModifierStateMask
XkbStateNotifyEvent::lookup_mods
XkbStateNotifyEvent::event_type
XkbStateNotifyEvent::keycode

Comment 9

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

Thank you for a lot of information! Sounds like option 3 is interesting. I don't think we should do big change only for this bug since this is very minor issue (can mark as WONTFIX), however, it seems that the implementation may help bug 749553, isn't it?

Comment 10

[Karl Tomlinson (:karlt)]

Let's leave this bug open as it tracks something that can be improved.

There are probably other ways to get bug 749553 right in the majority of situations, but option 3 should be able to provide the right info all situations.

On its own, getting bug 749553 absolutely correct may not be so necessary as to require listening for Xkb events.  However, listening for Xkb events could help in that bug, as well as this bug, and for doing better than DispatchMissedButtonReleases
http://hg.mozilla.org/mozilla-central/annotate/253009438c5b/widget/gtk2/nsWindow.cpp#l2558
All those use cases are making it look like it could be worthwhile.

Comment 11

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

Attachment: Patch

This works perfectly to me.

I failed to use XkbSelectEventDetails(), therefore, this patch uses XkbSelectEvents() instead. If you have idea to use it and think it's better, let me know.

Comment 12

[Karl Tomlinson (:karlt)]

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

>+    int xkbMajorVer = XkbMajorVersion;
>+    int xkbMinorVer = XkbMinorVersion;
>+    if (!XkbLibraryVersion(&xkbMajorVer, &xkbMinorVer)) {
>+        PR_LOG(gKeymapWrapperLog, PR_LOG_ALWAYS,
>+               ("KeymapWrapper(%p): InitXKBExtension failed due to failure of "
>+                "XkbLibraryVersion()", this));
>+        return;
>+    }
>+
>+    Display* display =
>+        gdk_x11_display_get_xdisplay(gdk_display_get_default());
>+
>+    int opcode, baseErrorCode;
>+    if (!XkbQueryExtension(display, &opcode, &mXKBBaseEventCode, &baseErrorCode,
>+                           &xkbMajorVer, &xkbMinorVer)) {

XkbLibraryVersion will set xkbMajorVer and xkbMinorVer to that of the library,
which may be newer than what is required of the server in XkbQueryExtension,
so these variables should be reset to XkbMajorVersion and XkbMinorVersion
before the XkbQueryExtension call.

>+    if (!XkbSelectEvents(display, XkbUseCoreKbd,
>+                         XkbStateNotifyMask | XkbIndicatorStateNotifyMask,
>+                         XkbStateNotifyMask | XkbIndicatorStateNotifyMask)) {

We should be able to only listen for what we need here.
Did you try this?:

  XkbSelectEventDetails(display, XkbUseCoreKbd, XkbStateNotify,
                        XkbModifierStateMask, XkbModifierStateMask);

If so, what happened?

I'm not very clear what the indicator map is, but I don't think you need
XkbIndicatorStateNotifyMask here.

The rest looks good.

Comment 13

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

Attachment: Patch

> We should be able to only listen for what we need here.
> Did you try this?:
> 
>   XkbSelectEventDetails(display, XkbUseCoreKbd, XkbStateNotify,
>                         XkbModifierStateMask, XkbModifierStateMask);
> 
> If so, what happened?

You're right. My old patch didn't work with like your code, but perhaps, I just passed wrong values to the arguments.

This patch works fine both on Fedora and Ubuntu.

Comment 14

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

Oops, please ignore the |#ifdef 0|. I'll remove it.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/35c594c4201a

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35c594c4201a