Bugfix: compare all resolved modifier masks

Before this commit, i3 only compared the user-specified modifiers and
incorrectly ignored the resolved modifiers (such as the numlock
fallback).

While at it, also fix the testcase which treated numlock as a momentary
modifier, whereas it really is a latched modifier.

fixes #2418

src/bindings.c

@@ -188,6 +188,17 @@ void regrab_all_buttons(xcb_connection_t *conn) {
     xcb_ungrab_server(conn);
 }
 
+static bool modifiers_match(const uint32_t modifiers_mask, const uint32_t modifiers_state) {
+    /* modifiers_mask is a special case: a value of 0 does not mean “match
+     * all”, but rather “match exactly when no modifiers are present”. */
+    if (modifiers_mask == 0) {
+        /* Verify no modifiers are pressed. A bitwise AND would lead to
+         * false positives, see issue #2002. */
+        return (modifiers_state == 0);
+    }
+    return ((modifiers_state & modifiers_mask) == modifiers_mask);
+}
+
 /*
  * Returns a pointer to the Binding with the specified modifiers and
  * keycode or NULL if no such binding exists.
@@ -210,34 +221,15 @@ static Binding *get_binding(i3_event_state_mask_t state_filtered, bool is_releas
     const uint32_t xkb_group_state = (state_filtered & 0xFFFF0000);
     const uint32_t modifiers_state = (state_filtered & 0x0000FFFF);
     TAILQ_FOREACH(bind, bindings, bindings) {
-        const uint32_t xkb_group_mask = (bind->event_state_mask & 0xFFFF0000);
-        /* modifiers_mask is a special case: a value of 0 does not mean “match all”,
-         * but rather “match exactly when no modifiers are present”. */
-        const uint32_t modifiers_mask = (bind->event_state_mask & 0x0000FFFF);
-        const bool groups_match = ((xkb_group_state & xkb_group_mask) == xkb_group_mask);
-        bool mods_match;
-        if (modifiers_mask == 0) {
-            /* Verify no modifiers are pressed. A bitwise AND would lead to
-             * false positives, see issue #2002. */
-            mods_match = (modifiers_state == 0);
-        } else {
-            mods_match = ((modifiers_state & modifiers_mask) == modifiers_mask);
-        }
-        const bool state_matches = (groups_match && mods_match);
-
-        DLOG("binding groups_match = %s, mods_match = %s, state_matches = %s\n",
-             (groups_match ? "yes" : "no"),
-             (mods_match ? "yes" : "no"),
-             (state_matches ? "yes" : "no"));
-        /* First compare the state_filtered (unless this is a
-         * B_UPON_KEYRELEASE_IGNORE_MODS binding and this is a KeyRelease
-         * event) */
         if (bind->input_type != input_type)
             continue;
-        if (!state_matches &&
+
-            (bind->release != B_UPON_KEYRELEASE_IGNORE_MODS ||
+        const uint32_t xkb_group_mask = (bind->event_state_mask & 0xFFFF0000);
-             !is_release))
+        const bool groups_match = ((xkb_group_state & xkb_group_mask) == xkb_group_mask);
+        if (!groups_match) {
+            DLOG("skipping binding %p because XKB groups do not match\n", bind);
             continue;
+        }
 
         /* For keyboard bindings where a symbol was specified by the user, we
          * need to look in the array of translated keycodes for the event’s
@@ -247,7 +239,11 @@ static Binding *get_binding(i3_event_state_mask_t state_filtered, bool is_releas
             bool found_keycode = false;
             struct Binding_Keycode *binding_keycode;
             TAILQ_FOREACH(binding_keycode, &(bind->keycodes_head), keycodes) {
-                if (binding_keycode->keycode == input_keycode) {
+                const uint32_t modifiers_mask = (binding_keycode->modifiers & 0x0000FFFF);
+                const bool mods_match = modifiers_match(modifiers_mask, modifiers_state);
+                DLOG("binding_keycode->modifiers = %d, modifiers_mask = %d, modifiers_state = %d, mods_match = %s\n",
+                     binding_keycode->modifiers, modifiers_mask, modifiers_state, (mods_match ? "yes" : "no"));
+                if (binding_keycode->keycode == input_keycode && mods_match) {
                     found_keycode = true;
                     break;
                 }
@@ -255,6 +251,17 @@ static Binding *get_binding(i3_event_state_mask_t state_filtered, bool is_releas
             if (!found_keycode)
                 continue;
         } else {
+            const uint32_t modifiers_mask = (bind->event_state_mask & 0x0000FFFF);
+            const bool mods_match = modifiers_match(modifiers_mask, modifiers_state);
+            DLOG("binding mods_match = %s\n", (mods_match ? "yes" : "no"));
+            /* First compare the state_filtered (unless this is a
+             * B_UPON_KEYRELEASE_IGNORE_MODS binding and this is a KeyRelease
+             * event) */
+            if (!mods_match &&
+                (bind->release != B_UPON_KEYRELEASE_IGNORE_MODS ||
+                 !is_release))
+                continue;
+
             /* This case is easier: The user specified a keycode */
             if (bind->keycode != input_code)
                 continue;

testcases/t/264-keypress-numlock.t

@@ -35,6 +35,9 @@ bindsym KP_End nop KP_End
 
 # Binding which should work with numlock and without.
 bindsym Mod4+a nop a
+
+# Binding which should work with numlock and without, see issue #2418.
+bindsym Escape nop Escape
 EOT
 
 my $pid = launch_with_config($config);
@@ -52,10 +55,12 @@ is(listen_for_binding(
 
 is(listen_for_binding(
     sub {
-        xtest_key_press(77); # Num_Lock
+        xtest_key_press(77); # enable Num_Lock
+        xtest_key_release(77); # enable Num_Lock
         xtest_key_press(87); # KP_1
         xtest_key_release(87); # KP_1
-        xtest_key_release(77); # Num_Lock
+        xtest_key_press(77); # disable Num_Lock
+        xtest_key_release(77); # disable Num_Lock
     },
     ),
    'KP_1',
@@ -74,20 +79,43 @@ is(listen_for_binding(
 
 is(listen_for_binding(
     sub {
-        xtest_key_press(77); # Num_Lock
+        xtest_key_press(77); # enable Num_Lock
+        xtest_key_release(77); # enable Num_Lock
         xtest_key_press(133); # Super_L
         xtest_key_press(38); # a
         xtest_key_release(38); # a
         xtest_key_release(133); # Super_L
-        xtest_key_release(77); # Num_Lock
+        xtest_key_press(77); # disable Num_Lock
+        xtest_key_release(77); # disable Num_Lock
     },
     ),
    'a',
    'triggered the "a" keybinding');
 
+is(listen_for_binding(
+    sub {
+        xtest_key_press(9); # Escape
+        xtest_key_release(9); # Escape
+    },
+    ),
+   'Escape',
+   'triggered the "Escape" keybinding');
+
+is(listen_for_binding(
+    sub {
+        xtest_key_press(77); # enable Num_Lock
+        xtest_key_release(77); # enable Num_Lock
+        xtest_key_press(9); # Escape
+        xtest_key_release(9); # Escape
+        xtest_key_press(77); # disable Num_Lock
+        xtest_key_release(77); # disable Num_Lock
+    },
+    ),
+   'Escape',
+   'triggered the "Escape" keybinding');
 
 sync_with_i3;
-is(scalar @i3test::XTEST::binding_events, 4, 'Received exactly 4 binding events');
+is(scalar @i3test::XTEST::binding_events, 6, 'Received exactly 4 binding events');
 
 exit_gracefully($pid);
 
@@ -117,10 +145,12 @@ is(listen_for_binding(
 
 is(listen_for_binding(
     sub {
-        xtest_key_press(77); # Num_Lock
+        xtest_key_press(77); # enable Num_Lock
+        xtest_key_release(77); # enable Num_Lock
         xtest_key_press(87); # KP_1
         xtest_key_release(87); # KP_1
-        xtest_key_release(77); # Num_Lock
+        xtest_key_press(77); # disable Num_Lock
+        xtest_key_release(77); # disable Num_Lock
     },
     ),
    'timeout',
@@ -129,7 +159,7 @@ is(listen_for_binding(
 # TODO: This test does not verify that i3 does _NOT_ grab keycode 87 with Mod2.
 
 sync_with_i3;
-is(scalar @i3test::XTEST::binding_events, 5, 'Received exactly 5 binding events');
+is(scalar @i3test::XTEST::binding_events, 7, 'Received exactly 5 binding events');
 
 exit_gracefully($pid);