Finish KeyboardListener

[jessegreenberg]

From #1445, where we are making it easier to add global key listeners. That issue spawned a general KeyboardListener that should be used in lots of cases. While that issue is about the hotkey registry, this issue should track the remaining work for KeyboardListener.

List of known work currently:

• fireOnKeyDown and fireOnKeyUp options combinations are confusing. Combine into one.
• Remove assertMutuallyExclusiveOptions for firing on key up and fire on hold behavior, both should be supported now.
• Make sure the listener supports handled and aborted well. KeyboardListener should support capture.
• These listeners should be interrupted when FocusManager.windowHasFocusProperty becomes false. See Problem with global shift key listener quadrilateral#311
• EnglishStringToCodeMap and the listener need to support left and right modifier keys.
• Update accessibility documentation to use the new listener instead of KeyboardDragListener and globalKeyStateTracker.
• EnglishStringToCodeMap should NOT offer left vs right keys for modifiers. It should be generalized to have values of type Array that take all of the key codes that match the english key. Then '1' maps to both key for 1 and Numpad1, and so on. (After discussion with @zepumph related to Add keyboard input to Keypad scenery-phet#790)

Work to do after these are done, probably in their own issues:

• Re-implement KeyboardDragListener with KeyboardListener.
• Remove most usages of globalKeyStateTracker for KeyboardListener.

[jessegreenberg]

WIP Patch for the first checkbox

Index: js/listeners/KeyboardListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/listeners/KeyboardListener.ts b/js/listeners/KeyboardListener.ts
--- a/js/listeners/KeyboardListener.ts	(revision 14b4ed70fbf92fe6ba57892e92fcdbbcb7b82da7)
+++ b/js/listeners/KeyboardListener.ts	(date 1674000956266)
@@ -48,8 +48,6 @@
 import optionize from '../../../phet-core/js/optionize.js';
 import { EnglishStringToCodeMap, globalKeyStateTracker, scenery, SceneryEvent, TInputListener } from '../imports.js';
 import KeyboardUtils from '../accessibility/KeyboardUtils.js';
-import assertMutuallyExclusiveOptions from '../../../phet-core/js/assertMutuallyExclusiveOptions.js';
-
 
 // NOTE: The typing for ModifierKey and OneKeyStroke is limited TypeScript, there is a limitation to the number of
 //       entries in a union type. If that limitation is not acceptable remove this typing. OR maybe TypeScript will
@@ -83,12 +81,6 @@
   // Called when the listener detects that the set of keys are pressed.
   callback?: ( event: SceneryEvent<KeyboardEvent> | null, listener: KeyboardListener<Keys> ) => void;
 
-  // When true, the listener will fire when the keys are released.
-  fireOnKeyUp?: boolean;
-
-  // When true, the listener will fire when the keys are first pressed down.
-  fireOnKeyDown?: boolean;
-
   // When true, the listener will fire continuously while keys are held down, at the following intervals.
   fireOnHold?: boolean;
 
@@ -100,6 +92,8 @@
 
   // TODO: Potential option to allow overlap between the keys of this KeyboardListener and another.
   allowKeyOverlap?: boolean;
+
+  listenerFireTrigger?: ListenerFireTrigger;
 };
 
 type KeyGroup<Keys extends readonly OneKeyStroke[]> = {
@@ -120,6 +114,12 @@
   timer: CallbackTimer | null;
 };
 
+// Possible input types that decide when the callbacks of this listener should fire.
+// - 'up': Callbacks fire on release of keys.
+// - 'down': Callbacks fire on press of keys.
+// - 'both': Callbacks fire on both press and release of keys.
+type ListenerFireTrigger = 'up' | 'down' | 'both';
+
 class KeyboardListener<Keys extends readonly OneKeyStroke[]> implements TInputListener {
 
   // The function called when a KeyGroup is pressed (or just released). Provides the SceneryEvent that fired the input
@@ -128,8 +128,8 @@
   private readonly _callback: ( event: SceneryEvent<KeyboardEvent> | null, listener: KeyboardListener<Keys> ) => void;
 
   // Will it the callback fire on keys up or down?
-  private readonly _fireOnKeyUp: boolean;
-  private readonly _fireOnKeyDown: boolean;
+
+  private readonly _listenerFireTrigger: ListenerFireTrigger;
 
   // Does the listener fire the callback continuously when keys are held down?
   private readonly _fireOnHold: boolean;
@@ -158,13 +158,10 @@
   private readonly _allowKeyOverlap: boolean;
 
   public constructor( providedOptions: KeyboardListenerOptions<Keys> ) {
-    assert && assertMutuallyExclusiveOptions( providedOptions, [ 'fireOnKeyUp' ], [ 'fireOnHold', 'fireOnHoldInterval', 'fireOnHoldDelay' ] );
-
     const options = optionize<KeyboardListenerOptions<Keys>>()( {
       callback: _.noop,
       global: false,
-      fireOnKeyDown: true,
-      fireOnKeyUp: false,
+      listenerFireTrigger: 'down',
       fireOnHold: false,
       fireOnHoldDelay: 400,
       fireOnHoldInterval: 100,
@@ -172,9 +169,8 @@
     }, providedOptions );
 
     this._callback = options.callback;
-    this._fireOnKeyUp = options.fireOnKeyUp;
-    this._fireOnKeyDown = options.fireOnKeyDown;
 
+    this._listenerFireTrigger = options.listenerFireTrigger;
     this._fireOnHold = options.fireOnHold;
     this._fireOnHoldDelay = options.fireOnHoldDelay;
     this._fireOnHoldInterval = options.fireOnHoldInterval;
@@ -213,7 +209,7 @@
    * fire callbacks and start CallbackTimers.
    */
   private handleKeyDown( event: SceneryEvent<KeyboardEvent> ): void {
-    if ( this._fireOnKeyDown ) {
+    if ( this._listenerFireTrigger === 'down' || this._listenerFireTrigger === 'both' ) {
 
       // modifier keys can be pressed in any order but the last key in the group must be pressed last
       this._keyGroups.forEach( keyGroup => {
@@ -255,7 +251,7 @@
       } );
     }
 
-    if ( this._fireOnKeyUp ) {
+    if ( this._listenerFireTrigger === 'up' || this._listenerFireTrigger === 'both' ) {
       this._keyGroups.forEach( keyGroup => {
         if ( globalKeyStateTracker.areKeysExclusivelyDown( keyGroup.modifierKeys ) &&
              KeyboardUtils.getEventCode( event.domEvent ) === keyGroup.key ) {

EDIT: WIP for "EnglishStringToCodeMap and the listener need to support left and right modifier keys.". For next time, KeyStateTracker.correctModifierKeys is making this difficult because it "fakes" modifier key presses specifically for the left buttons.

Subject: [PATCH] Combine fireOnKeyDown and fireOnKeyUp options, see https://github.com/phetsims/scenery/issues/1520
---
Index: js/accessibility/KeyboardUtils.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/KeyboardUtils.ts b/js/accessibility/KeyboardUtils.ts
--- a/js/accessibility/KeyboardUtils.ts	(revision 9a5aab10a87c1a085a5551d8b57f8ed1f124b086)
+++ b/js/accessibility/KeyboardUtils.ts	(date 1674085047444)
@@ -36,6 +36,10 @@
 const KEY_8 = 'Digit8';
 const KEY_9 = 'Digit9';
 
+const SHIFT_PREFIX = 'Shift';
+const ALT_PREFIX = 'Alt';
+const CONTROL_PREFIX = 'Control';
+
 const ARROW_KEYS = [ KEY_RIGHT_ARROW, KEY_LEFT_ARROW, KEY_UP_ARROW, KEY_DOWN_ARROW ];
 const WASD_KEYS = [ KEY_W, KEY_S, KEY_A, KEY_D ];
 const NUMBER_KEYS = [ KEY_0, KEY_1, KEY_2, KEY_3, KEY_4, KEY_5, KEY_6, KEY_7, KEY_8, KEY_9 ];
@@ -121,6 +125,11 @@
   CONTROL_KEYS: CONTROL_KEYS,
   ALT_KEYS: ALT_KEYS,
 
+  CONTROL_PREFIX: CONTROL_PREFIX,
+  ALT_PREFIX: ALT_PREFIX,
+  SHIFT_PREFIX: SHIFT_PREFIX,
+
+
   /**
    * Returns whether the key corresponds to pressing an arrow key
    */
Index: js/accessibility/KeyStateTracker.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/KeyStateTracker.ts b/js/accessibility/KeyStateTracker.ts
--- a/js/accessibility/KeyStateTracker.ts	(revision 9a5aab10a87c1a085a5551d8b57f8ed1f124b086)
+++ b/js/accessibility/KeyStateTracker.ts	(date 1674085691896)
@@ -249,16 +249,28 @@
   }
 
   /**
-   * Returns true if a key with the KeyboardEvent.code is currently down.
+   * Returns true if a key with the KeyboardEvent.code is currently down. KeyboardEvent.code has different codes
+   * for
    */
   public isKeyDown( key: string ): boolean {
-    if ( !this.keyState[ key ] ) {
+    if ( key === KeyboardUtils.ALT_PREFIX ) {
+      return this.altKeyDown;
+    }
+    else if ( key === KeyboardUtils.SHIFT_PREFIX ) {
+      return this.shiftKeyDown;
+    }
+    else if ( key === KeyboardUtils.CONTROL_PREFIX ) {
+      return this.ctrlKeyDown;
+    }
+    else {
+      if ( !this.keyState[ key ] ) {
 
-      // key hasn't been pressed once yet
-      return false;
-    }
+        // key hasn't been pressed once yet
+        return false;
+      }
 
-    return this.keyState[ key ].keyDown;
+      return this.keyState[ key ].keyDown;
+    }
   }
 
   /**
@@ -294,6 +306,7 @@
    * are the KeyboardEvent.code for keys you are interested in.
    */
   public areKeysExclusivelyDown( keyList: string [] ): boolean {
+    console.log( Object.keys( this.keyState ).length );
     return Object.keys( this.keyState ).length === keyList.length && this.areKeysDown( keyList );
   }
 
Index: js/accessibility/EnglishStringToCodeMap.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/EnglishStringToCodeMap.ts b/js/accessibility/EnglishStringToCodeMap.ts
--- a/js/accessibility/EnglishStringToCodeMap.ts	(revision 9a5aab10a87c1a085a5551d8b57f8ed1f124b086)
+++ b/js/accessibility/EnglishStringToCodeMap.ts	(date 1674085311491)
@@ -45,10 +45,22 @@
   8: KeyboardUtils.KEY_8,
   9: KeyboardUtils.KEY_9,
 
-  // TODO: what about right vs left?
-  ctrl: KeyboardUtils.KEY_CONTROL_LEFT,
-  alt: KeyboardUtils.KEY_ALT_LEFT,
-  shift: KeyboardUtils.KEY_SHIFT_LEFT,
+  // Beware of the special case here. Event.code uses different codes for left/right modifier keys, but
+  // you usually want to observe both left/right keys when listening for a key combination.
+  // KeyStateTracker.isKeyDown will return true if either modifier key is pressed down if these codes
+  // are passed to the function.
+  // will
+  ctrl: KeyboardUtils.CONTROL_PREFIX,
+  alt: KeyboardUtils.ALT_PREFIX,
+  shift: KeyboardUtils.SHIFT_PREFIX,
+
+  ctrlLeft: KeyboardUtils.KEY_CONTROL_LEFT,
+  altLeft: KeyboardUtils.KEY_ALT_LEFT,
+  shiftLeft: KeyboardUtils.KEY_SHIFT_LEFT,
+
+  ctrlRight: KeyboardUtils.KEY_CONTROL_RIGHT,
+  altRight: KeyboardUtils.KEY_ALT_RIGHT,
+  shiftRight: KeyboardUtils.KEY_SHIFT_RIGHT,
 
   enter: KeyboardUtils.KEY_ENTER,
   tab: KeyboardUtils.KEY_TAB,

[jessegreenberg]

Make sure the listener supports handled and aborted well.

handled and aborted will work fine on a single SceneryEvent, but press and hold sends many SceneryEvents through scenery. The first may be handled/aborted, but after the first callback fire, many other events may go through. I can't think of a good way around this using Input/SceneryEvent so Ill try adding it at the listener level as an option so it can be called more frequently than the callback. Then every keydown event that comes through will get aborted.

Also, the callback is not guaranteed a SceneryEvent because of the timer, so calling abort from the callback wouldn't be possible anyway.

Notes from testing before changes:

Scene graph:

A
 \
  B
    \
     C

A has global keyboardListener
B has a KeyboardListener
C has a KeyboardListener

I see each fire their callback when I hit Node C. Then I add event.handle() to Node C callback. That should stop bubbling and the listener on B. (Should still see A because it is global). It works correctly on the callback from keydown. But if I hold the keys down I see the listener on B is called, confirming my suspicions of the problem. Same test with abort and I saw the same behavior. I thought abort might stop listener on Node A...Ill have to look into that.

thought abort might stop listener on Node A...Ill have to look into that.

Input.dispatchGlobalEvent creates its own SceneryEvent, so it won't get prevented on abort. We currently do not have the ability to prevent a global event listener from an event listener that went through normal dispatch.

After changes, handle/abort for scene graph dispatch and capture for global events are working well. We need a new issue to support handle/abort for global events.

[jessegreenberg]

Ready for final cleanup/documentation updates. Will probably be a week or two before I touch this again.

[zepumph]

Here is a patch @jessegreenberg and I worked on today. Woo!

Subject: [PATCH] remove allowOtherKeys and hard code the notion of "modifier" keys for ctrl/alt/shift changing key nature, https://github.com/phetsims/scenery-phet/issues/790
---
Index: scenery-phet/js/keypad/Keypad.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/keypad/Keypad.ts b/scenery-phet/js/keypad/Keypad.ts
--- a/scenery-phet/js/keypad/Keypad.ts	(revision 30f9fca579525b4dc060c7562f0faa28c636da03)
+++ b/scenery-phet/js/keypad/Keypad.ts	(date 1678914534539)
@@ -202,7 +202,6 @@
 
     const keyboardListener = new KeyboardListener( {
       keys: Object.keys( keyboardKeys ),
-      allowOtherKeys: true,
       callback: ( sceneryEvent, listener ) => {
         const keyObject = keyboardKeys[ listener.keysPressed! ];
         this.keyAccumulator.handleKeyPressed( keyObject!.identifier );
Index: scenery/js/listeners/KeyboardListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/listeners/KeyboardListener.ts b/scenery/js/listeners/KeyboardListener.ts
--- a/scenery/js/listeners/KeyboardListener.ts	(revision fcd7f9b9e9b77439a09a2ebba588a78a2f67b242)
+++ b/scenery/js/listeners/KeyboardListener.ts	(date 1678916814510)
@@ -77,11 +77,6 @@
   // level documentation for more information and an example of providing keys.
   keys: Keys;
 
-  // If true, the listener will fire callbacks if keys other than keys in the key group happen to be down at the same
-  // time. If false, callbacks will fire only when the keys of a group are exclusively down. Setting this to true is
-  // also useful if you want multiple key groups from your provided keys to fire callbacks at the same time.
-  allowOtherKeys?: boolean;
-
   // If true, the listener will fire for keys regardless of where focus is in the document. Use this when you want
   // to add some key press behavior that will always fire no matter what the event target is. If this listener
   // is added to a Node, it will only fire if the Node (and all of its ancestors) are visible with inputEnabled: true.
@@ -122,7 +117,7 @@
 
 type KeyGroup<Keys extends readonly OneKeyStroke[]> = {
 
-  // All must be pressed fully before the key is pressed to activate the command.
+  // All must be pressed fully before the key is pressed to activate the command. TODO: These are not codes!?!?! see https://github.com/phetsims/scenery/issues/1520
   modifierKeys: string[];
 
   // the final key that is pressed (after modifier keys) to trigger the listener
@@ -175,7 +170,6 @@
   private readonly _global: boolean;
   private readonly _handle: boolean;
   private readonly _abort: boolean;
-  private readonly _allowOtherKeys: boolean;
 
   private readonly _windowFocusListener: ( windowHasFocus: boolean ) => void;
 
@@ -190,8 +184,7 @@
       listenerFireTrigger: 'down',
       fireOnHold: false,
       fireOnHoldDelay: 400,
-      fireOnHoldInterval: 100,
-      allowOtherKeys: false
+      fireOnHoldInterval: 100
     }, providedOptions );
 
     this._callback = options.callback;
@@ -201,7 +194,6 @@
     this._fireOnHold = options.fireOnHold;
     this._fireOnHoldDelay = options.fireOnHoldDelay;
     this._fireOnHoldInterval = options.fireOnHoldInterval;
-    this._allowOtherKeys = options.allowOtherKeys;
 
     this._activeKeyGroups = [];
 
@@ -303,7 +295,8 @@
    * provided keys are down.
    */
   private areKeysDownForListener( keys: string[] ): boolean {
-    return this._allowOtherKeys ? globalKeyStateTracker.areKeysDown( keys ) : globalKeyStateTracker.areKeysExclusivelyDown( keys );
+
+    return globalKeyStateTracker.areKeysDownRespectModifiers( keys );
   }
 
   /**
@@ -425,7 +418,7 @@
       assert && assert( groupKeys.length > 0, 'no keys provided?' );
 
       const naturalKey = groupKeys.slice( -1 )[ 0 ];
-      const key = EnglishStringToCodeMap[ naturalKey ];
+      const key = EnglishStringToCodeMap[ naturalKey ]; // TODO: but this could be an array of strings, to support ctrlLeft and ctrlRight, https://github.com/phetsims/scenery/issues/1520
       assert && assert( key, `Key not found, do you need to add it to EnglishStringToCodeMap? ${naturalKey}` );
 
       let modifierKeys: string[] = [];
Index: scenery/js/accessibility/KeyStateTracker.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/KeyStateTracker.ts b/scenery/js/accessibility/KeyStateTracker.ts
--- a/scenery/js/accessibility/KeyStateTracker.ts	(revision fcd7f9b9e9b77439a09a2ebba588a78a2f67b242)
+++ b/scenery/js/accessibility/KeyStateTracker.ts	(date 1678916859106)
@@ -21,6 +21,13 @@
 import PickOptional from '../../../phet-core/js/types/PickOptional.js';
 import TEmitter from '../../../axon/js/TEmitter.js';
 
+
+// Modifiers are "LEFT" because of implementation of correctModifierKeys()
+const MODIFIER_KEYS = [
+  KeyboardUtils.KEY_ALT_LEFT, KeyboardUtils.KEY_CONTROL_LEFT, KeyboardUtils.KEY_SHIFT_LEFT,
+  KeyboardUtils.KEY_ALT_RIGHT, KeyboardUtils.KEY_CONTROL_RIGHT, KeyboardUtils.KEY_SHIFT_RIGHT
+];
+
 // Type describing the state of a single key in the KeyState.
 type KeyStateInfo = {
 
@@ -290,38 +297,50 @@
   }
 
   /**
-   * Returns true if ALL keys in the list are down and ONLY the keys in the list are down. Values of keyList array
-   * are the KeyboardEvent.code for keys you are interested in OR the KeyboardEvent.key in the special case of
-   * modifier keys.
+   * Like areKeysDown, but treats ctrl+d as a different "key" as d, so
+   * `areKeysDownRespectModifiers(['d'])` is false if any modifiers are down.
+   * If any modifier key is down that isn't in this list, then fail
    *
-   * (scenery-internal)
    */
-  public areKeysExclusivelyDown( keyList: string [] ): boolean {
-    const keyStateKeys = Object.keys( this.keyState );
+  public areKeysDownRespectModifiers( keyList: string[] ): boolean {
+    // TODO: maybe keyslist is actually a string[][] to support left/right modifiers
 
-    // quick sanity check for equality first
-    if ( keyStateKeys.length !== keyList.length ) {
+    // TODO: we have to handle modifiers differently because they aren't codes at this time, https://github.com/phetsims/scenery/issues/1520
+    if ( !this.areModifiersDownExclusive( keyList ) ) {
       return false;
     }
 
-    // Now make sure that every key in the list is in the keyState
-    let onlyKeyListDown = true;
+    // TODO: This will be better when the keyList is strictly Codes. https://github.com/phetsims/scenery/issues/1520
+    const withNoWeirdModifierNotCodes = keyList.filter( key => !KeyboardUtils.isModifierKey( key ) );
+
+    // An empty list is "down"
+    return this.areKeysDown( withNoWeirdModifierNotCodes );
+  }
+
+  /**
+   * Returns false if any modifier key (shift/ctrl/alt) is currently down but not in the provided keylist. Otherwise
+   * return if the provided modifiers (and only modifiers) from the param are currently down.
+   */
+  private areModifiersDownExclusive( keyList: string[] ): boolean {
+    const modifiersFromKeyList = [];
     for ( let i = 0; i < keyList.length; i++ ) {
-      const initialKey = keyList[ i ];
-      let keysToCheck = [ initialKey ];
+      const key = keyList[ i ];
+      if ( KeyboardUtils.isModifierKey( key ) ) {
 
-      // If a modifier key, need to look for the equivalent pair of left/right KeyboardEvent.codes in the list
-      // because KeyStateTracker works exclusively with codes.
-      if ( KeyboardUtils.isModifierKey( initialKey ) ) {
-        keysToCheck = KeyboardUtils.MODIFIER_KEY_TO_CODE_MAP.get( initialKey )!;
+        // This is not generally a good solution. It basically maps "ctrl+d" to [CTRL_LEFT,CTRL_RIGHT,D], but it is ok
+        // because we check both left and right modifiers in the loop below too.
+        modifiersFromKeyList.push( ...KeyboardUtils.MODIFIER_KEY_TO_CODE_MAP.get( key )! );
       }
-
-      if ( _.intersection( keyStateKeys, keysToCheck ).length === 0 ) {
-        onlyKeyListDown = false;
+    }
+    for ( let i = 0; i < MODIFIER_KEYS.length; i++ ) {
+      const modifier = MODIFIER_KEYS[ i ];
+      if ( this.isKeyDown( modifier ) && !modifiersFromKeyList.includes( modifier ) ) {
+        return false;
       }
     }
 
-    return onlyKeyListDown;
+    // TODO: but then modifiersFromKeyList has AltLeft and AltRight. Only one of those will be down. https://github.com/phetsims/scenery/issues/1520
+    return this.areKeysDown( _.every( modifiersFromKeyList), this.isAnyKeyInListDown );
   }
 
   /**

JG EDIT: This patch is a better alternative to a new option allowOtherKeys which lets the callback be fired when keys other than those specified for the provided key group are down. We like the direction but got stuck with how left/right modifier keys are hacked into KeyboardListener and the EnglishStringToCodeMap. We hope this will all be easier when we get to the checkbox item that says "EnglishStringToCodeMap should NOT offer left vs right keys for modifiers. ...". From phetsims/scenery-phet#790 (comment).

[jessegreenberg]

We visited this in during dev meeting today to assess priority for common code work.

I would really like to see this get time, but I will be out for a big chunk of this iteration so we are not recommending it. I will recommend it for when I return.

[jessegreenberg]

Today I finished "EnglishStringToCodeMap should NOT..." and it did offer an easy way to proceed with #1520 (comment) only using KeyboardEvent.code, which is great.

[jessegreenberg]

Items in #1520 (comment) are complete, and we have side issues #1570 and phetsims/tasks#1126 for the remaining work.