Add support for Alt to CharacterActivator, add tests (#113466)

gspencergoog · web-flow · commit 1b4800c9ea31 · 2022-10-17T20:46:28.000Z
diff --git a/packages/flutter/lib/src/widgets/platform_menu_bar.dart b/packages/flutter/lib/src/widgets/platform_menu_bar.dart
@@ -45,78 +45,100 @@ class ShortcutSerialization {
   /// Creates a [ShortcutSerialization] representing a single character.
   ///
   /// This is used by a [CharacterActivator] to serialize itself.
-  ShortcutSerialization.character(String character)
-      : _internal = <String, Object?>{_kShortcutCharacter: character},
+  ShortcutSerialization.character(String character, {
+    bool alt = false,
+    bool control = false,
+    bool meta = false,
+  })  : assert(character.length == 1),
         _character = character,
-        assert(character.length == 1);
+        _trigger = null,
+        _alt = alt,
+        _control = control,
+        _meta = meta,
+        _shift = null,
+        _internal = <String, Object?>{
+          _kShortcutCharacter: character,
+          _kShortcutModifiers: (control ? _shortcutModifierControl : 0) |
+              (alt ? _shortcutModifierAlt : 0) |
+              (meta ? _shortcutModifierMeta : 0),
+        };
 
   /// Creates a [ShortcutSerialization] representing a specific
   /// [LogicalKeyboardKey] and modifiers.
   ///
   /// This is used by a [SingleActivator] to serialize itself.
   ShortcutSerialization.modifier(
     LogicalKeyboardKey trigger, {
-    bool control = false,
-    bool shift = false,
     bool alt = false,
+    bool control = false,
     bool meta = false,
-  })  : assert(trigger != LogicalKeyboardKey.shift &&
-               trigger != LogicalKeyboardKey.shiftLeft &&
-               trigger != LogicalKeyboardKey.shiftRight &&
-               trigger != LogicalKeyboardKey.alt &&
+    bool shift = false,
+  })  : assert(trigger != LogicalKeyboardKey.alt &&
                trigger != LogicalKeyboardKey.altLeft &&
                trigger != LogicalKeyboardKey.altRight &&
                trigger != LogicalKeyboardKey.control &&
                trigger != LogicalKeyboardKey.controlLeft &&
                trigger != LogicalKeyboardKey.controlRight &&
                trigger != LogicalKeyboardKey.meta &&
                trigger != LogicalKeyboardKey.metaLeft &&
-               trigger != LogicalKeyboardKey.metaRight,
+               trigger != LogicalKeyboardKey.metaRight &&
+               trigger != LogicalKeyboardKey.shift &&
+               trigger != LogicalKeyboardKey.shiftLeft &&
+               trigger != LogicalKeyboardKey.shiftRight,
                'Specifying a modifier key as a trigger is not allowed. '
                'Use provided boolean parameters instead.'),
         _trigger = trigger,
-        _control = control,
-        _shift = shift,
+        _character = null,
         _alt = alt,
+        _control = control,
         _meta = meta,
+        _shift = shift,
         _internal = <String, Object?>{
           _kShortcutTrigger: trigger.keyId,
-          _kShortcutModifiers: (control ? _shortcutModifierControl : 0) |
-              (alt ? _shortcutModifierAlt : 0) |
-              (shift ? _shortcutModifierShift : 0) |
-              (meta ? _shortcutModifierMeta : 0),
+          _kShortcutModifiers: (alt ? _shortcutModifierAlt : 0) |
+            (control ? _shortcutModifierControl : 0) |
+            (meta ? _shortcutModifierMeta : 0) |
+            (shift ? _shortcutModifierShift : 0),
         };
 
   final Map<String, Object?> _internal;
 
   /// The keyboard key that triggers this shortcut, if any.
   LogicalKeyboardKey? get trigger => _trigger;
-  LogicalKeyboardKey? _trigger;
+  final LogicalKeyboardKey? _trigger;
 
   /// The character that triggers this shortcut, if any.
   String? get character => _character;
-  String? _character;
+  final String? _character;
+
+  /// If this shortcut has a [trigger], this indicates whether or not the
+  /// alt modifier needs to be down or not.
+  bool? get alt => _alt;
+  final bool? _alt;
 
   /// If this shortcut has a [trigger], this indicates whether or not the
   /// control modifier needs to be down or not.
   bool? get control => _control;
-  bool? _control;
+  final bool? _control;
+
+  /// If this shortcut has a [trigger], this indicates whether or not the meta
+  /// (also known as the Windows or Command key) modifier needs to be down or
+  /// not.
+  bool? get meta => _meta;
+  final bool? _meta;
 
   /// If this shortcut has a [trigger], this indicates whether or not the
   /// shift modifier needs to be down or not.
   bool? get shift => _shift;
-  bool? _shift;
+  final bool? _shift;
 
-  /// If this shortcut has a [trigger], this indicates whether or not the
-  /// alt modifier needs to be down or not.
-  bool? get alt => _alt;
-  bool? _alt;
+  /// The bit mask for the [LogicalKeyboardKey.alt] key (or it's left/right
+  /// equivalents) being down.
+  static const int _shortcutModifierAlt = 1 << 2;
 
-  /// If this shortcut has a [trigger], this indicates whether or not the meta
-  /// (also known as the Windows or Command key) modifier needs to be down or
-  /// not.
-  bool? get meta => _meta;
-  bool? _meta;
+  /// The bit mask for the [LogicalKeyboardKey.control] key (or it's left/right
+  /// equivalents) being down.
+  static const int _shortcutModifierControl = 1 << 3;
 
   /// The bit mask for the [LogicalKeyboardKey.meta] key (or it's left/right
   /// equivalents) being down.
@@ -126,14 +148,6 @@ class ShortcutSerialization {
   /// equivalents) being down.
   static const int _shortcutModifierShift = 1 << 1;
 
-  /// The bit mask for the [LogicalKeyboardKey.alt] key (or it's left/right
-  /// equivalents) being down.
-  static const int _shortcutModifierAlt = 1 << 2;
-
-  /// The bit mask for the [LogicalKeyboardKey.alt] key (or it's left/right
-  /// equivalents) being down.
-  static const int _shortcutModifierControl = 1 << 3;
-
   /// Converts the internal representation to the format needed for a
   /// [PlatformMenuItem] to include it in its serialized form for sending to the
   /// platform.
diff --git a/packages/flutter/lib/src/widgets/shortcuts.dart b/packages/flutter/lib/src/widgets/shortcuts.dart
@@ -580,25 +580,40 @@ class SingleActivator with Diagnosticable, MenuSerializableShortcut implements S
 /// See also:
 ///
 ///  * [SingleActivator], an activator that represents a single key combined
-///    with modifiers, such as `Ctrl+C`.
+///    with modifiers, such as `Ctrl+C` or `Ctrl-Right Arrow`.
 class CharacterActivator with Diagnosticable, MenuSerializableShortcut implements ShortcutActivator {
   /// Triggered when the key event yields the given character.
   ///
-  /// The [control] and [meta] flags represent whether the respect modifier
-  /// keys should be held (true) or released (false). They default to false.
-  /// [CharacterActivator] can not check Shift keys or Alt keys yet, and will
-  /// accept whether they are pressed or not.
+  /// The [alt], [control], and [meta] flags represent whether the respective
+  /// modifier keys should be held (true) or released (false). They default to
+  /// false. [CharacterActivator] cannot check Shift keys, since the shift key
+  /// affects the resulting character, and will accept whether either of the
+  /// Shift keys are pressed or not, as long as the key event produces the
+  /// correct character.
   ///
   /// By default, the activator is checked on all [RawKeyDownEvent] events for
-  /// the [character]. If `includeRepeats` is false, only the [character]
-  /// events with a false [RawKeyDownEvent.repeat] attribute will be
-  /// considered.
+  /// the [character] in combination with the requested modifier keys. If
+  /// `includeRepeats` is false, only the [character] events with a false
+  /// [RawKeyDownEvent.repeat] attribute will be considered.
   const CharacterActivator(this.character, {
+    this.alt = false,
     this.control = false,
     this.meta = false,
     this.includeRepeats = true,
   });
 
+  /// Whether either (or both) alt keys should be held for the [character] to
+  /// activate the shortcut.
+  ///
+  /// It defaults to false, meaning all Alt keys must be released when the event
+  /// is received in order to activate the shortcut. If it's true, then either
+  /// or both Alt keys must be pressed.
+ ///
+  /// See also:
+  ///
+  /// * [LogicalKeyboardKey.altLeft], [LogicalKeyboardKey.altRight].
+  final bool alt;
+
   /// Whether either (or both) control keys should be held for the [character]
   /// to activate the shortcut.
   ///
@@ -631,7 +646,7 @@ class CharacterActivator with Diagnosticable, MenuSerializableShortcut implement
   /// attribute will be considered.
   final bool includeRepeats;
 
-  /// The character of the triggering event.
+  /// The character which triggers the shortcut.
   ///
   /// This is typically a single-character string, such as '?' or 'œ', although
   /// [CharacterActivator] doesn't check the length of [character] or whether it
@@ -653,6 +668,7 @@ class CharacterActivator with Diagnosticable, MenuSerializableShortcut implement
     return event is RawKeyDownEvent
       && event.character == character
       && (includeRepeats || !event.repeat)
+      && (alt == (pressed.contains(LogicalKeyboardKey.altLeft) || pressed.contains(LogicalKeyboardKey.altRight)))
       && (control == (pressed.contains(LogicalKeyboardKey.controlLeft) || pressed.contains(LogicalKeyboardKey.controlRight)))
       && (meta == (pressed.contains(LogicalKeyboardKey.metaLeft) || pressed.contains(LogicalKeyboardKey.metaRight)));
   }
@@ -662,6 +678,7 @@ class CharacterActivator with Diagnosticable, MenuSerializableShortcut implement
     String result = '';
     assert(() {
       final List<String> keys = <String>[
+        if (alt) 'Alt',
         if (control) 'Control',
         if (meta) 'Meta',
         "'$character'",
@@ -674,7 +691,7 @@ class CharacterActivator with Diagnosticable, MenuSerializableShortcut implement
 
   @override
   ShortcutSerialization serializeForMenu() {
-    return ShortcutSerialization.character(character);
+    return ShortcutSerialization.character(character, alt: alt, control: control, meta: meta);
   }
 
   @override
diff --git a/packages/flutter/test/widgets/platform_menu_bar_test.dart b/packages/flutter/test/widgets/platform_menu_bar_test.dart
@@ -228,6 +228,34 @@ void main() {
       ]);
     });
   });
+
+  group('ShortcutSerialization', () {
+    testWidgets('character constructor', (WidgetTester tester) async {
+      final ShortcutSerialization serialization = ShortcutSerialization.character('?');
+      expect(serialization.toChannelRepresentation(), equals(<String, Object?>{
+        'shortcutCharacter': '?',
+        'shortcutModifiers': 0,
+      }));
+      final ShortcutSerialization serializationWithModifiers = ShortcutSerialization.character('?', alt: true, control: true, meta: true);
+      expect(serializationWithModifiers.toChannelRepresentation(), equals(<String, Object?>{
+        'shortcutCharacter': '?',
+        'shortcutModifiers': 13,
+      }));
+    });
+
+    testWidgets('modifier constructor', (WidgetTester tester) async {
+      final ShortcutSerialization serialization = ShortcutSerialization.modifier(LogicalKeyboardKey.home);
+      expect(serialization.toChannelRepresentation(), equals(<String, Object?>{
+        'shortcutTrigger': LogicalKeyboardKey.home.keyId,
+        'shortcutModifiers': 0,
+      }));
+      final ShortcutSerialization serializationWithModifiers = ShortcutSerialization.modifier(LogicalKeyboardKey.home, alt: true, control: true, meta: true, shift: true);
+      expect(serializationWithModifiers.toChannelRepresentation(), equals(<String, Object?>{
+        'shortcutTrigger': LogicalKeyboardKey.home.keyId,
+        'shortcutModifiers': 15,
+      }));
+    });
+  });
 }
 
 const List<String> mainMenu = <String>[
diff --git a/packages/flutter/test/widgets/shortcuts_test.dart b/packages/flutter/test/widgets/shortcuts_test.dart
@@ -1162,10 +1162,10 @@ void main() {
       invoked = 0;
     }, variant: KeySimulatorTransitModeVariant.all());
 
-    testWidgets('handles Ctrl and Meta', (WidgetTester tester) async {
+    testWidgets('handles Alt, Ctrl and Meta', (WidgetTester tester) async {
       int invoked = 0;
       await tester.pumpWidget(activatorTester(
-        const CharacterActivator('?', meta: true, control: true),
+        const CharacterActivator('?', alt: true, meta: true, control: true),
         (Intent intent) { invoked += 1; },
       ));
       await tester.pump();
@@ -1176,7 +1176,8 @@ void main() {
       await tester.sendKeyUpEvent(LogicalKeyboardKey.slash);
       expect(invoked, 0);
 
-      // Press Ctrl + Meta + Shift + /
+      // Press Left Alt + Ctrl + Meta + Shift + /
+      await tester.sendKeyDownEvent(LogicalKeyboardKey.altLeft);
       await tester.sendKeyDownEvent(LogicalKeyboardKey.metaLeft);
       await tester.sendKeyDownEvent(LogicalKeyboardKey.controlLeft);
       expect(invoked, 0);
@@ -1185,9 +1186,26 @@ void main() {
       await tester.sendKeyUpEvent(LogicalKeyboardKey.slash);
       await tester.sendKeyUpEvent(LogicalKeyboardKey.shiftLeft);
       await tester.sendKeyUpEvent(LogicalKeyboardKey.metaLeft);
+      await tester.sendKeyUpEvent(LogicalKeyboardKey.altLeft);
       await tester.sendKeyUpEvent(LogicalKeyboardKey.controlLeft);
       expect(invoked, 1);
       invoked = 0;
+
+      // Press Right Alt + Ctrl + Meta + Shift + /
+      await tester.sendKeyDownEvent(LogicalKeyboardKey.shiftRight);
+      await tester.sendKeyDownEvent(LogicalKeyboardKey.altRight);
+      await tester.sendKeyDownEvent(LogicalKeyboardKey.metaRight);
+      await tester.sendKeyDownEvent(LogicalKeyboardKey.controlRight);
+      expect(invoked, 0);
+      await tester.sendKeyDownEvent(LogicalKeyboardKey.slash, character: '?');
+      expect(invoked, 1);
+      await tester.sendKeyUpEvent(LogicalKeyboardKey.slash);
+      await tester.sendKeyUpEvent(LogicalKeyboardKey.shiftRight);
+      await tester.sendKeyUpEvent(LogicalKeyboardKey.metaRight);
+      await tester.sendKeyUpEvent(LogicalKeyboardKey.altRight);
+      await tester.sendKeyUpEvent(LogicalKeyboardKey.controlRight);
+      expect(invoked, 1);
+      invoked = 0;
     }, variant: KeySimulatorTransitModeVariant.all());
 
     testWidgets('isActivatedBy works as expected', (WidgetTester tester) async {
