Properly interpret modifiers on GLFW key events (#44844)

GLFW key events set modifier flags based on what the modifier state was before the event, unlike every other platform. This modifies the GLFW key support to take that into account.

As a small cleanup, I fixed a documentation macro reference for the modifier flags that was duplicate

packages/flutter/lib/src/services/raw_keyboard.dart

@@ -276,8 +276,7 @@ abstract class RawKeyEvent extends Diagnosticable {
       case 'macos':
         data = RawKeyEventDataMacOs(
             characters: message['characters'] ?? '',
-            charactersIgnoringModifiers:
-                message['charactersIgnoringModifiers'] ?? '',
+            charactersIgnoringModifiers: message['charactersIgnoringModifiers'] ?? '',
             keyCode: message['keyCode'] ?? 0,
             modifiers: message['modifiers'] ?? 0);
         break;
@@ -287,7 +286,8 @@ abstract class RawKeyEvent extends Diagnosticable {
             unicodeScalarValues: message['unicodeScalarValues'] ?? 0,
             keyCode: message['keyCode'] ?? 0,
             scanCode: message['scanCode'] ?? 0,
-            modifiers: message['modifiers'] ?? 0);
+            modifiers: message['modifiers'] ?? 0,
+            isDown: message['type'] == 'keydown');
         break;
       case 'web':
         data = RawKeyEventDataWeb(

packages/flutter/lib/src/services/raw_keyboard_linux.dart

@@ -27,6 +27,7 @@ class RawKeyEventDataLinux extends RawKeyEventData {
     this.scanCode = 0,
     this.keyCode = 0,
     this.modifiers = 0,
+    @required this.isDown,
   }) : assert(scanCode != null),
        assert(unicodeScalarValues != null),
        assert((unicodeScalarValues & ~LogicalKeyboardKey.valueMask) == 0),
@@ -64,6 +65,9 @@ class RawKeyEventDataLinux extends RawKeyEventData {
   /// This value may be different depending on the window toolkit used. See [KeyHelper].
   final int modifiers;
 
+  /// Whether or not this key event is a key down (true) or key up (false).
+  final bool isDown;
+
   @override
   String get keyLabel => unicodeScalarValues == 0 ? null : String.fromCharCode(unicodeScalarValues);
 
@@ -113,7 +117,7 @@ class RawKeyEventDataLinux extends RawKeyEventData {
 
   @override
   bool isModifierPressed(ModifierKey key, {KeyboardSide side = KeyboardSide.any}) {
-    return keyHelper.isModifierPressed(key, modifiers, side: side);
+    return keyHelper.isModifierPressed(key, modifiers, side: side, keyCode: keyCode, isDown: isDown);
   }
 
   @override
@@ -150,7 +154,7 @@ abstract class KeyHelper {
 
   /// Returns true if the given [ModifierKey] was pressed at the time of this
   /// event.
-  bool isModifierPressed(ModifierKey key, int modifiers, {KeyboardSide side = KeyboardSide.any});
+  bool isModifierPressed(ModifierKey key, int modifiers, {KeyboardSide side = KeyboardSide.any, int keyCode, bool isDown});
 
   /// The numpad key from the specific key code mapping.
   LogicalKeyboardKey numpadKey(int keyCode);
@@ -164,46 +168,97 @@ class GLFWKeyHelper with KeyHelper {
   /// This mask is used to check the [modifiers] field to test whether the CAPS
   /// LOCK modifier key is on.
   ///
-  /// {@template flutter.services.logicalKeyboardKey.modifiers}
+  /// {@template flutter.services.glfwKeyHelper.modifiers}
   /// Use this value if you need to decode the [modifiers] field yourself, but
-  /// it's much easier to use [isModifierPressed] if you just want to know if
-  /// a modifier is pressed.
+  /// it's much easier to use [isModifierPressed] if you just want to know if a
+  /// modifier is pressed. This is especially true on GLFW, since its modifiers
+  /// don't include the effects of the current key event.
   /// {@endtemplate}
   static const int modifierCapsLock = 0x0010;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// SHIFT modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.glfwKeyHelper.modifiers}
   static const int modifierShift = 0x0001;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// CTRL modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.glfwKeyHelper.modifiers}
   static const int modifierControl = 0x0002;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// ALT modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.glfwKeyHelper.modifiers}
   static const int modifierAlt = 0x0004;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// Meta(SUPER) modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.glfwKeyHelper.modifiers}
   static const int modifierMeta = 0x0008;
 
 
   /// This mask is used to check the [modifiers] field to test whether any key in
   /// the numeric keypad is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.glfwKeyHelper.modifiers}
   static const int modifierNumericPad = 0x0020;
 
+  int _mergeModifiers({int modifiers, int keyCode, bool isDown}) {
+    // GLFW Key codes for modifier keys.
+    const int shiftLeftKeyCode = 340;
+    const int shiftRightKeyCode = 344;
+    const int controlLeftKeyCode = 341;
+    const int controlRightKeyCode = 345;
+    const int altLeftKeyCode = 342;
+    const int altRightKeyCode = 346;
+    const int metaLeftKeyCode = 343;
+    const int metaRightKeyCode = 347;
+    const int capsLockKeyCode = 280;
+    const int numLockKeyCode = 282;
+
+    // On GLFW, the "modifiers" bitfield is the state as it is BEFORE this event
+    // happened, not AFTER, like every other platform. Consequently, if this is
+    // a key down, then we need to add the correct modifier bits, and if it's a
+    // key up, we need to remove them.
+
+    int modifierChange = 0;
+    switch (keyCode) {
+      case shiftLeftKeyCode:
+      case shiftRightKeyCode:
+        modifierChange = modifierShift;
+        break;
+      case controlLeftKeyCode:
+      case controlRightKeyCode:
+        modifierChange = modifierControl;
+        break;
+      case altLeftKeyCode:
+      case altRightKeyCode:
+        modifierChange = modifierAlt;
+        break;
+      case metaLeftKeyCode:
+      case metaRightKeyCode:
+        modifierChange = modifierMeta;
+        break;
+      case capsLockKeyCode:
+        modifierChange = modifierCapsLock;
+        break;
+      case numLockKeyCode:
+        modifierChange = modifierNumericPad;
+        break;
+      default:
+        break;
+    }
+
+    return isDown ? modifiers | modifierChange : modifiers & ~modifierChange;
+  }
+
   @override
-  bool isModifierPressed(ModifierKey key, int modifiers, {KeyboardSide side = KeyboardSide.any}) {
+  bool isModifierPressed(ModifierKey key, int modifiers, {KeyboardSide side = KeyboardSide.any, int keyCode, bool isDown}) {
+    modifiers = _mergeModifiers(modifiers: modifiers, keyCode: keyCode, isDown: isDown);
     switch (key) {
       case ModifierKey.controlModifier:
         return modifiers & modifierControl != 0;

packages/flutter/lib/src/services/raw_keyboard_macos.dart

@@ -228,7 +228,7 @@ class RawKeyEventDataMacOs extends RawKeyEventData {
   /// This mask is used to check the [modifiers] field to test whether the CAPS
   /// LOCK modifier key is on.
   ///
-  /// {@template flutter.services.logicalKeyboardKey.modifiers}
+  /// {@template flutter.services.rawKeyEventDataMacOs.modifiers}
   /// Use this value if you need to decode the [modifiers] field yourself, but
   /// it's much easier to use [isModifierPressed] if you just want to know if
   /// a modifier is pressed.
@@ -238,91 +238,91 @@ class RawKeyEventDataMacOs extends RawKeyEventData {
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// SHIFT modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierShift = 0x20000;
 
   /// This mask is used to check the [modifiers] field to test whether the left
   /// SHIFT modifier key is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierLeftShift = 0x02;
 
   /// This mask is used to check the [modifiers] field to test whether the right
   /// SHIFT modifier key is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierRightShift = 0x04;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// CTRL modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierControl = 0x40000;
 
   /// This mask is used to check the [modifiers] field to test whether the left
   /// CTRL modifier key is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierLeftControl = 0x01;
 
   /// This mask is used to check the [modifiers] field to test whether the right
   /// CTRL modifier key is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierRightControl = 0x2000;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// ALT modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierOption = 0x80000;
 
   /// This mask is used to check the [modifiers] field to test whether the left
   /// ALT modifier key is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierLeftOption = 0x20;
 
   /// This mask is used to check the [modifiers] field to test whether the right
   /// ALT modifier key is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierRightOption = 0x40;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// CMD modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierCommand = 0x100000;
 
   /// This mask is used to check the [modifiers] field to test whether the left
   /// CMD modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierLeftCommand = 0x08;
 
   /// This mask is used to check the [modifiers] field to test whether the right
   /// CMD modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierRightCommand = 0x10;
 
   /// This mask is used to check the [modifiers] field to test whether any key in
   /// the numeric keypad is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierNumericPad = 0x200000;
 
   /// This mask is used to check the [modifiers] field to test whether the
   /// HELP modifier key is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierHelp = 0x400000;
 
   /// This mask is used to check the [modifiers] field to test whether one of the
   /// FUNCTION modifier keys is pressed.
   ///
-  /// {@macro flutter.services.logicalKeyboardKey.modifiers}
+  /// {@macro flutter.services.rawKeyEventDataMacOs.modifiers}
   static const int modifierFunction = 0x800000;
 
   /// Used to retrieve only the device-independent modifier flags, allowing

packages/flutter/test/services/raw_keyboard_test.dart

@@ -706,32 +706,57 @@ void main() {
       GLFWKeyHelper.modifierCapsLock: _ModifierCheck(ModifierKey.capsLockModifier, KeyboardSide.all),
     };
 
+    // How modifiers are interpreted depends upon the keyCode for GLFW.
+    int keyCodeForModifier(int modifier, {bool isLeft}) {
+      switch (modifier) {
+        case GLFWKeyHelper.modifierAlt:
+          return isLeft ? 342 : 346;
+        case GLFWKeyHelper.modifierShift:
+          return isLeft ? 340 : 344;
+        case GLFWKeyHelper.modifierControl:
+          return isLeft ? 341 : 345;
+        case GLFWKeyHelper.modifierMeta:
+          return isLeft ? 343 : 347;
+        case GLFWKeyHelper.modifierNumericPad:
+          return 282;
+        case GLFWKeyHelper.modifierCapsLock:
+          return 280;
+        default:
+          return 65; // keyA
+      }
+    }
+
     test('modifier keys are recognized individually', () {
       for (int modifier in modifierTests.keys) {
-        final RawKeyEvent event = RawKeyEvent.fromMessage(<String, dynamic>{
-          'type': 'keydown',
-          'keymap': 'linux',
-          'toolkit': 'glfw',
-          'keyCode': 65,
-          'scanCode': 0x00000026,
-          'unicodeScalarValues': 97,
-          'modifiers': modifier,
-        });
-        final RawKeyEventDataLinux data = event.data;
-        for (ModifierKey key in ModifierKey.values) {
-          if (modifierTests[modifier].key == key) {
-            expect(
-              data.isModifierPressed(key, side: modifierTests[modifier].side),
-              isTrue,
-              reason: "$key should be pressed with metaState $modifier, but isn't.",
-            );
-            expect(data.getModifierSide(key), equals(modifierTests[modifier].side));
-          } else {
-            expect(
-              data.isModifierPressed(key, side: modifierTests[modifier].side),
-              isFalse,
-              reason: '$key should not be pressed with metaState $modifier.',
-            );
+        for (bool isDown in <bool>[true, false]) {
+          for (bool isLeft in <bool>[true, false]) {
+            final RawKeyEvent event = RawKeyEvent.fromMessage(<String, dynamic>{
+              'type': isDown ? 'keydown' : 'keyup',
+              'keymap': 'linux',
+              'toolkit': 'glfw',
+              'keyCode': keyCodeForModifier(modifier, isLeft: isLeft),
+              'scanCode': 0x00000026,
+              'unicodeScalarValues': 97,
+              // GLFW modifiers don't include the current key event.
+              'modifiers': isDown ? 0 : modifier,
+            });
+            final RawKeyEventDataLinux data = event.data;
+            for (ModifierKey key in ModifierKey.values) {
+              if (modifierTests[modifier].key == key) {
+                expect(
+                  data.isModifierPressed(key, side: modifierTests[modifier].side),
+                  isDown ? isTrue : isFalse,
+                  reason: "${isLeft ? 'left' : 'right'} $key ${isDown ? 'should' : 'should not'} be pressed with metaState $modifier, when key is ${isDown ? 'down' : 'up'}, but isn't.",
+                );
+                expect(data.getModifierSide(key), equals(modifierTests[modifier].side));
+              } else {
+                expect(
+                  data.isModifierPressed(key, side: modifierTests[modifier].side),
+                  isFalse,
+                  reason: "${isLeft ? 'left' : 'right'} $key should not be pressed with metaState $modifier, wwhen key is ${isDown ? 'down' : 'up'}, but is.",
+                );
+              }
+            }
           }
         }
       }