Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a registry for global hotkeys #1445

Closed
jessegreenberg opened this issue Sep 1, 2022 · 16 comments
Closed

Create a registry for global hotkeys #1445

jessegreenberg opened this issue Sep 1, 2022 · 16 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Sep 1, 2022

From phetsims/phet-info#188. There is support for adding global hotkeys. but there is no way to check for duplicates, collisions, or quickly see all of the hotkeys you are using in an app. Would be great to have some kind of automated registry. KeyStateTracker would be responsible for this or use it so this issue is in scenery.

If it does not seem appropriate to do this in code, we could fallback to having a place for documentation that will work for PhET.

In quarterly planning this was identified as something to work on Q3 2022.

@emily-phet
Copy link

@jessegreenberg This sounds like a great idea! This won't fit onto any of my budgets currently, so would need to be done from one of Kathy's grants.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Oct 14, 2022

@zepumph and I did some brainstorming on this today for the upcoming work:

Something like KeyboardListener - adds keyup and keydown listeners to implement an API for hotkeys.
    myNode.addInputListener( new KeyboardListener( ... ) );
    Handles scenery input events for a Node in the scene graph 
    It adds things to the global registry
    FireListener could be composed with a KeyboardListener!

GlobalKeyboardListener
    - Adds hotkeys to the global key registry.
    - This would extend KeyboardListener!

GlobalKeyRegistry (KeyStateTracker?)
    - Registers all hotkeys (global and for individual Nodes).
    - Use the same input langauge as scenery so that even when we need to work with DOM events
    (to capture things outside of the PDOM root) the API for global and scene graph element is the same.
    - This is really like the KeyStateTracker that we already have! A lot is here but we can
        rewrite KeyStateTracker to support things like:
        - KeyStateTracker.addListener
        - KeyStateTracker.addHotkey
    - Move implementation of hotkeys from KeyboardDragListener here (or look there for inspiration)
    - The global registry should disallow duplicates. When a set of keys is added in a KeyboardListener to a Node, those keys should be blocked from use in a global hotkey.

Lets talk about HotKey.
    - The name is overloaded and weird. We need to define it.
    - This work is really about adding a new/better way for all keyboard interaction.

Other thoughts:
    - What are the ramifications for screen reader support (since keyboard events often don't work there?)
    - Is this all going to replace `keydown` and `keyup` event listeners throughout? Probably!!
 

Maybe an example that could fire on enter/space:

    // The new KeyboardListener to fire on enter/space
    const myKeyboardListener = new KeyboardListener( {
      keys: [ KeyboardUtils.KEY_ENTER, KeyboardUtils.KEY_SPACE ],
      callback: fireAction.execute,
      onKeyUp: true, // do you want things to fire on keyup or keydown?
      fireOnClick: true // also fire on the click listener when using assistive technology!
    } );
    listItemNode.addInputListener( myKeyboardListener );

For global keys we would use a global instance of the KeyboardListener

     globalKeyboardListener.addHotkeys( {
      keys: [ KeyboardUtils.KEY_ENTER, KeyboardUtils.KEY_SPACE ],
      callback: fireAction.execute,
      onKeyUp: true, // do you want things to fire on keyup or keydown?
      fireOnClick: true // also fire on the click listener when using assistive technology!
    } );

We thought that for "firing", FireListener could be composed with a KeyboardListener like this so that implementing fire behavior is as simple as adding a single FireListener to the component for keyboard/mouse/touch inputs.

@samreid
Copy link
Member

samreid commented Oct 20, 2022

The examples above seem to need a modifiers API. For example, If the keys were ctrl+enter or alt/opt+space.

@zepumph
Copy link
Member

zepumph commented Oct 20, 2022

As well as an API for "these keys in order" vs "any order".

As well as an API for "all keys must be down" vs "any of these keys".

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Oct 20, 2022

Some notes from brainstorming:

  • If the Display adds listeners to the window, we might be able to do global input through scenery's event system with bubbling instead of a separate system for "global" event listeners. (Display option assumeFullWindow)

    • This might not work for scenery use cases outside of a sim.
    • But maybe there will be some singleton that a Display could use to add global input listeners.
    • Currently, accessibility events go through the PDOM root. That is why we could not use scenery input system for "global" events. But we should be able to move it to the window or root with assumeFullWindow and that would greatly simplify things.
    • Maybe just a keystate tracker per display? Maybe a keystate tracker singleton that each Display listens to?
  • Totally different idea - we have different scenery names like globalkeydown that would dispatch to all visible/input enabled components.

    • Then in "global" listeners you don't need to put a check for visible/pdomVisible/input enabled

@samreid
Copy link
Member

samreid commented Oct 20, 2022

It was proposed that we would implement this through Input.ts so that listeners can be per-ScreenView or per-Display and all the scenery bubbling will work great. globalKeyStateTracker would get deleted/integrated into Input.ts.

@zepumph
Copy link
Member

zepumph commented Oct 20, 2022

Notes from today's brainstorm

// The new KeyboardListener to fire on enter/space
const myKeyboardListener = new KeyboardListener( {
  keys: [ KeyboardUtils.KEY_ENTER, KeyboardUtils.KEY_SPACE ],
  callback: () => { submit() },
  onKeyUp: true, // do you want things to fire on keyup or keydown?
  fireOnClick: true, // also fire on the click listener when using assistive technology!
  modifierKeys: [],
  ordered: true,
  allKeysVsAnyKeys: 'any'
} );

// this node or a DOM child need focus to get this.
myNode.addInputListener( myKeyboardListener );

//
display.addInputListener( myKeyboardListener )


// two new scenery events "globalKeydown" "globalKeyup" ignores focus to have this, but respects visible, pdomVisible, inputEnabled.

const myKeyboardListener = new KeyboardListener( {

  // MUTUALLY EXCLUSIVE OPTIONS
  keySets: [ [ KEY.CTRL, KeyboardUtils.KEY_ENTER ], [ KEY.CTRL, KeyboardUtils.KEY_SPACE ] ],
  keys: [], // for just one combination,
  // nah let's not key: KEY.ENTER, // just for a single key BUT THIS ISN'T HOW WE DO NODE CHILDREN!!?!??!
  /////////////

  //


  callback: () => { submit() },
  onKeyUp: true, // do you want things to fire on keyup or keydown?
  fireOnClick: true, // also fire on the click listener when using assistive technology!
  holdTime: 8, // ms
  ordered: true // order matters? Or is it the combination BOOOOO
  // allKeysVsAnyKeys: 'any'
} );


// The BASE hot key from jump to sweater:
const myKeyboardListener = new KeyboardListener( {
  keys: [ {
    key: Key.S
    modifierKeys: [ KEY.J ]
  } ]
} );
const myKeyboardListener = new KeyboardListener( {
  keys: [ [ Key.J, Key.S ] ] // both must be pressed down at the same time. Last key last, all others are modifiers
} );
const myKeyboardListener = new KeyboardListener( {
  keys: [ [ Key.J ], [ Key.S ] ] // this means press j or s
} );

this.addInputListener( new GlobalKeyboardListener( {
  keys: [ KeyboardUtils.KEY_C ],
  modifierKeys: [ KEY.ALT ]
} ) );


// Listener on Ctrl+A, Shift+B ->
const myKeyboardListener = new KeyboardListener( {
  keys: [ KEY.A, Key.B ],
  modifierKeys: [ Ctrl, Shift ]
} );

type Key = string;
type KeyCombination = {
  key: Key;
  modifierKeys?: Key[]
}


type KeyboardListenerOptions = {
  keys: KeyCombination[];
}

// The BASE hot key from jump to sweater:
const myKeyboardListener = new KeyboardListener( {
  keys: [ {
    key: Key.S
    modifierKeys: [ KEY.J ]
  } ]
} );


// The BASE hot key from jump to sweater:
const myKeyboardListener = new KeyboardListener( {
  keys: [ strings.keyboardShorcut.jumpToSweater ]
} );

// The BASE hot key from jump to sweater:
const myKeyboardListener = new KeyboardListener( {
  keys: [ {
    key: 'S',
    modifierKeys: [ 'J' ]
  } ]
} );


'Ctrl+S'
'Ctrl+s'
'ctrl+s'
'ctrl+arrowLeft'
'0|1|2|3|4|5|6|7|7|8|9'


keys: [ 'ArrowLeft','ArrowDown' ]
keys: [ 'Ctrl+ArrowLeft','Ctrl+ArrowDown' ]

'ArrowLeft|ArrowDown' +
'ArrowRight|ArrowUp';
'ArrowLeft|ArrowDown+ArrowRight|ArrowUp';
'shift+ArrowLeft/Down';
'shift+KeyS';
'shift+S';

'Shift+s'
'Ctrl+s+l';
'ctrl+S+L';
'ctrl+S+L,shift+S'; // sequence WE DON"T SUPPORT THIS YET

// jump ruler to optical objects
'j+o+1' // jump optical to object 1
'j+o'




// The BASE hot key from jump to sweater:
const myKeyboardListener = new KeyboardListener( {
  keys: [ 'j+o' ], // logical OR, any of these succeed to fire the callback.
  callback: (key) => {
    if( key is arrowDown){
      submit()
    }
    if( key is arrowUp){

    }},
  onKeyUp: true, // do you want things to fire on keyup or keydown? Defaults to keydown
  holdTime: 8, // ms
} );


// FOR NOW:
// don't support key sequences (comma), just modifier keys. Would probably
// Look at keyboard shortcut javascript library to see how we parse it. https://github.com/jaywcjlove/hotkeys
// don't support hyphens for character ranges (0-9 is just 0|1|2|3. . . .)
// 

JG summary notes: Towards the end of the meeting we liked a "natural language" API where you can define the keys you want to use with strings describing the key and would like to try that. This API might be friendlier for i18n later and could also be reused for the representation in the keyboard help or other displays.

EDIT: I looked into the mapping at https://github.com/jaywcjlove/hotkeys. It uses

const code = (x) => _keyMap[x.toLowerCase()]
  || _modifier[x.toLowerCase()]
  || x.toUpperCase().charCodeAt(0);

x.toUpperCase().charCodeAt(0); apparently maps to the keyCode, except for certain special and modifier keys. That won't work for our system that uses Event.code.

I used silly snippets like this to populate our own english string -> KeyboardEvent.code and put the output into a JavaScript object now.

'q,w,e,r,t,y,,u,i,o,a,s,d,f,g,h,j,k,l,z,x,c,v,b,n,m'.split(',').forEach( letter => {
  console.log( `${letter}: KeyboardUtils.KEY_${letter.toUpperCase()}`);
} );
'0,1,2,3,4,5,6,7,8,9'.split(',').forEach( letter => {
  console.log( `${letter}: KeyboardUtils.KEY_${letter.toUpperCase()}`);
} );

@jessegreenberg
Copy link
Contributor Author

Patch so I can switch computers:

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
new file mode 100644
--- /dev/null	(date 1666316197598)
+++ b/scenery/js/listeners/KeyboardListener.ts	(date 1666316197598)
@@ -0,0 +1,200 @@
+// Copyright 2022, University of Colorado Boulder
+
+// globalKeyStateTracker - the state of the keyboard. Everything new will be driven from the state of the keyboard.
+//   // The problem with globalKeyStateTracker is that is that it does not use scenery events.
+// it also is "global" and we want to go through the scenery input event system.
+
+/**
+ * @author Jesse Greenberg (PhET Interactive Simulations)
+ */
+
+import CallbackTimer from '../../../axon/js/CallbackTimer.js';
+import optionize from '../../../phet-core/js/optionize.js';
+import { EnglishStringToCodeMap, KeyStateTracker, scenery, SceneryEvent, TInputListener } from '../imports.js';
+import KeyboardUtils from '../accessibility/KeyboardUtils.js';
+import assertMutuallyExclusiveOptions from '../../../phet-core/js/assertMutuallyExclusiveOptions.js';
+
+const KEY_GROUP_SEPARATOR = '|';
+
+type KeyboardListenerOptions = {
+
+  // Keys that trigger functionality for this listener.
+  // 'j+o+1' // all three keys need to be held down in order
+  // 'j+o' // these two keys need to be pressed down
+  // '1|2|3' // any of these keys are pressed
+  // 'j+1|j+2' // any of these are pressed
+  // all keys except for the last are considered "modifier" keys
+  keys: string;
+  callback?: ( naturalKeys: string ) => void;
+  fireOnKeyUp?: boolean;
+
+  fireOnHold?: boolean;
+  fireOnHoldDelay?: number;
+  fireOnHoldInterval?: number;
+};
+
+type KeyGroup = {
+  modifierKeys: string[];
+  key: string;
+  allKeys: string[];
+  naturalKeys: string;
+};
+
+class KeyboardListener implements TInputListener {
+  private keyStateTracker: KeyStateTracker;
+
+  private _timer?: CallbackTimer;
+
+  // Mostly needed so we can refer to the key group key in a bound function
+  private _activeKeyGroup: KeyGroup | null;
+
+  private readonly _callback: ( key: string ) => void;
+  private readonly _fireOnKeyUp: boolean;
+  private readonly _keyGroups: KeyGroup[];
+
+  public constructor( providedOptions: KeyboardListenerOptions ) {
+    assert && assertMutuallyExclusiveOptions( providedOptions, [ 'fireOnKeyUp' ], [ 'fireOnHold' ] );
+
+    const options = optionize<KeyboardListenerOptions>()( {
+      callback: _.noop,
+      fireOnKeyUp: false,
+      fireOnHold: false,
+      fireOnHoldDelay: 400,
+      fireOnHoldInterval: 100
+    }, providedOptions );
+
+    this._callback = options.callback;
+    this._fireOnKeyUp = options.fireOnKeyUp;
+
+    // convert the keys array into something that we can look for in the event. Will need to be a key.code.
+    // modifier keys can be pressed in any order, then the last key last.
+    this._keyGroups = this.convertKeysToKeyGroups( options.keys );
+    // TODO: Creating a KeyStateTracker every listener seems unnecessary and inefficient. Can one globalKeyStateTracker
+    // drive everything but the listener queries it on keyboard events?
+    this.keyStateTracker = new KeyStateTracker();
+
+    this._activeKeyGroup = null;
+
+    if ( options.fireOnHold ) {
+      this._timer = new CallbackTimer( {
+        callback: this.fireCallback.bind( this ),
+        delay: options.fireOnHoldDelay,
+        interval: options.fireOnHoldInterval
+      } );
+    }
+  }
+
+  /**
+   * Mostly required to fire with CallbackTimer since the callback cannot take arguments.
+   */
+  public fireCallback(): void {
+    assert && assert( this._activeKeyGroup, 'Need an active keyGroup down to fire' );
+    this._callback( this._activeKeyGroup!.naturalKeys );
+  }
+
+  public keydown( event: SceneryEvent ): void {
+
+    // TODO: Ideally the tracker will soon go through scenery input system
+    // TODO: Ideally we will query a globalKeyStateTracker instead?
+    this.keyStateTracker.keydownUpdate( event.domEvent as KeyboardEvent );
+
+    if ( !this._fireOnKeyUp && !this._activeKeyGroup ) {
+
+      // modifier keys can be pressed in any order but the last key in the group must be pressed last
+      this._keyGroups.forEach( keyGroup => {
+        if ( this.keyStateTracker.areKeysDown( keyGroup.allKeys ) &&
+             this.keyStateTracker.mostRecentKeyFromList( keyGroup.allKeys ) === keyGroup.key ) {
+
+          this._activeKeyGroup = keyGroup;
+
+          if ( this._timer ) {
+            this._timer.start();
+          }
+          this.fireCallback();
+        }
+      } );
+    }
+  }
+
+  public keyup( event: SceneryEvent ): void {
+
+    // TODO: Ideally the tracker will soon go through scenery input system
+    // TODO: Ideally we will query a globalKeyStateTracker instead?
+    this.keyStateTracker.keyupUpdate( event.domEvent as KeyboardEvent );
+
+    // FOR NEXT TIME - just moved this up here so will need to re-test some things.
+
+    if ( this._activeKeyGroup ) {
+      if ( !this.keyStateTracker.areKeysDown( this._activeKeyGroup.allKeys ) ) {
+        console.log( 'stopping' );
+        if ( this._timer ) {
+          console.log( 'stopping' );
+          this._timer.stop( false );
+        }
+        this._activeKeyGroup = null;
+      }
+    }
+
+    if ( this._fireOnKeyUp ) {
+      this._keyGroups.forEach( keyGroup => {
+        if ( this.keyStateTracker.areKeysDown( keyGroup.modifierKeys ) &&
+             KeyboardUtils.getEventCode( event.domEvent ) === keyGroup.key ) {
+          this._activeKeyGroup = keyGroup;
+          this.fireCallback();
+          this._activeKeyGroup = null;
+        }
+      } );
+    }
+  }
+
+  public cancel(): void {
+    // TODO
+  }
+
+  public interrupt(): void {
+    // TODO
+  }
+
+  public dispose(): void {
+    // TODO
+  }
+
+  private convertKeysToKeyGroups( keys: string ): KeyGroup[] {
+
+    // An array of all the groups we want still in their "natural" form
+    const naturalKeyGroupStrings = keys.split( KEY_GROUP_SEPARATOR );
+
+    const keyGroups = naturalKeyGroupStrings.map( naturalKeys => {
+
+      // all of the keys in this group in an array
+      const groupKeys = naturalKeys.split( '+' );
+      assert && assert( groupKeys.length > 0, 'no keys provided?' );
+
+      // @ts-ignore - because a string shouldn't be used for lookup like this in the object type
+      const key = EnglishStringToCodeMap[ groupKeys.slice( -1 )[ 0 ] ];
+      assert && assert( key, 'Key not found, do you need to add it to EnglishStringToCodeMap?' );
+
+      let modifierKeys: string[] = [];
+      if ( groupKeys.length > 1 ) {
+        modifierKeys = groupKeys.slice( 0, groupKeys.length - 1 ).map( naturalModifierKey => {
+
+          // @ts-ignore - because a string shouldn't be used for lookup like this in the object type
+          const modifierKey = EnglishStringToCodeMap[ naturalModifierKey ];
+          assert && assert( modifierKey, 'Key not found, do you need to add it to EnglishStringToCodeMap' );
+          return modifierKey;
+        } );
+      }
+      return {
+        key: key,
+        modifierKeys: modifierKeys,
+        naturalKeys: naturalKeys,
+        allKeys: modifierKeys.concat( key )
+      };
+    } );
+
+    return keyGroups;
+  }
+}
+
+scenery.register( 'KeyboardListener', KeyboardListener );
+export default KeyboardListener;
Index: scenery/js/accessibility/EnglishStringToCodeMap.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/EnglishStringToCodeMap.ts b/scenery/js/accessibility/EnglishStringToCodeMap.ts
new file mode 100644
--- /dev/null	(date 1666304997481)
+++ b/scenery/js/accessibility/EnglishStringToCodeMap.ts	(date 1666304997481)
@@ -0,0 +1,58 @@
+// Copyright 2022, University of Colorado Boulder
+
+/**
+ * @author Jesse Greenberg (PhET Interactive Simulations)
+ */
+
+import { KeyboardUtils, scenery } from '../imports.js';
+
+const EnglishStringToCodeMap = {
+  q: KeyboardUtils.KEY_Q,
+  w: KeyboardUtils.KEY_W,
+  e: KeyboardUtils.KEY_E,
+  r: KeyboardUtils.KEY_R,
+  t: KeyboardUtils.KEY_T,
+  y: KeyboardUtils.KEY_Y,
+  u: KeyboardUtils.KEY_U,
+  i: KeyboardUtils.KEY_I,
+  o: KeyboardUtils.KEY_O,
+  a: KeyboardUtils.KEY_A,
+  s: KeyboardUtils.KEY_S,
+  d: KeyboardUtils.KEY_D,
+  f: KeyboardUtils.KEY_F,
+  g: KeyboardUtils.KEY_G,
+  h: KeyboardUtils.KEY_H,
+  j: KeyboardUtils.KEY_J,
+  k: KeyboardUtils.KEY_K,
+  l: KeyboardUtils.KEY_L,
+  z: KeyboardUtils.KEY_Z,
+  x: KeyboardUtils.KEY_X,
+  c: KeyboardUtils.KEY_C,
+  v: KeyboardUtils.KEY_V,
+  b: KeyboardUtils.KEY_B,
+  n: KeyboardUtils.KEY_N,
+  m: KeyboardUtils.KEY_M,
+  0: KeyboardUtils.KEY_0,
+  1: KeyboardUtils.KEY_1,
+  2: KeyboardUtils.KEY_2,
+  3: KeyboardUtils.KEY_3,
+  4: KeyboardUtils.KEY_4,
+  5: KeyboardUtils.KEY_5,
+  6: KeyboardUtils.KEY_6,
+  7: KeyboardUtils.KEY_7,
+  8: KeyboardUtils.KEY_8,
+  9: KeyboardUtils.KEY_9,
+
+  // TODO: what about right vs left?
+  ctrl: KeyboardUtils.KEY_CONTROL_LEFT,
+  alt: KeyboardUtils.KEY_ALT_LEFT,
+
+  space: KeyboardUtils.KEY_SPACE,
+  arrowLeft: KeyboardUtils.KEY_LEFT_ARROW,
+  arrowRight: KeyboardUtils.KEY_RIGHT_ARROW,
+  arrowUp: KeyboardUtils.KEY_UP_ARROW,
+  arrowDown: KeyboardUtils.KEY_DOWN_ARROW
+};
+
+scenery.register( 'EnglishStringToCodeMap', EnglishStringToCodeMap );
+export default EnglishStringToCodeMap;
\ No newline at end of file
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 b4ac2850e20a6fd9b43f7dedfad38ae03f3c1a03)
+++ b/scenery/js/accessibility/KeyStateTracker.ts	(date 1666312056913)
@@ -167,7 +167,7 @@
    * `Node.addInputListener` only supports type properties as event listeners, and not the event keys as
    * prototype methods. Please see https://github.com/phetsims/scenery/issues/851 for more information.
    */
-  private keydownUpdate( domEvent: KeyboardEvent ): void {
+  public keydownUpdate( domEvent: KeyboardEvent ): void {
     this.enabled && this.keydownUpdateAction.execute( domEvent );
   }
 
@@ -225,7 +225,7 @@
    * `Node.addInputListener` only supports type properties as event listeners, and not the event keys as
    * prototype methods. Please see https://github.com/phetsims/scenery/issues/851 for more information.
    */
-  private keyupUpdate( domEvent: KeyboardEvent ): void {
+  public keyupUpdate( domEvent: KeyboardEvent ): void {
     this.enabled && this.keyupUpdateAction.execute( domEvent );
   }
 
@@ -262,6 +262,11 @@
     return false;
   }
 
+  public mostRecentKeyFromList( keys: string[] ): string {
+    assert && assert( this.areKeysDown( keys ), 'Not all the keys in the list are down.' );
+    return _.minBy( keys, key => this.timeDownForKey( key ) )!;
+  }
+
   /**
    * Returns true if ALL of the keys in the list are currently down. Values of the keyList array are the
    * KeyboardEvent.code for the keys you are interested in.
Index: scenery/js/imports.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/imports.ts b/scenery/js/imports.ts
--- a/scenery/js/imports.ts	(revision b4ac2850e20a6fd9b43f7dedfad38ae03f3c1a03)
+++ b/scenery/js/imports.ts	(date 1666313997958)
@@ -24,6 +24,7 @@
 export { default as Utils } from './util/Utils.js';
 export { default as Focus } from './accessibility/Focus.js';
 export { default as KeyboardUtils } from './accessibility/KeyboardUtils.js';
+export { default as EnglishStringToCodeMap } from './accessibility/EnglishStringToCodeMap.js';
 export { default as EventIO } from './input/EventIO.js';
 export { default as SceneryStyle } from './util/SceneryStyle.js';
 export { default as CanvasContextWrapper } from './util/CanvasContextWrapper.js';
@@ -227,6 +228,7 @@
 export { default as HandleDownListener } from './listeners/HandleDownListener.js';
 export { default as KeyboardDragListener } from './listeners/KeyboardDragListener.js';
 export type { KeyboardDragListenerOptions } from './listeners/KeyboardDragListener.js';
+export { default as KeyboardListener } from './listeners/KeyboardListener.js';
 export { default as SpriteListenable } from './listeners/SpriteListenable.js';
 export { default as SwipeListener } from './listeners/SwipeListener.js';
 
Index: scenery-phet/js/buttons/ResetAllButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/buttons/ResetAllButton.ts b/scenery-phet/js/buttons/ResetAllButton.ts
--- a/scenery-phet/js/buttons/ResetAllButton.ts	(revision 7d330829d496ffb6ce91b1e123981ea6d0f66d1e)
+++ b/scenery-phet/js/buttons/ResetAllButton.ts	(date 1666315825509)
@@ -8,7 +8,7 @@
  * @author Chris Malley (PixelZoom, Inc.)
  */
 
-import { voicingUtteranceQueue } from '../../../scenery/js/imports.js';
+import { KeyboardListener, voicingUtteranceQueue } from '../../../scenery/js/imports.js';
 import StrictOmit from '../../../phet-core/js/types/StrictOmit.js';
 import resetAllSoundPlayer from '../../../tambo/js/shared-sound-players/resetAllSoundPlayer.js';
 import optionize from '../../../phet-core/js/optionize.js';
@@ -127,6 +127,31 @@
       } );
     } );
 
+    this.addInputListener( new KeyboardListener( {
+      keys: 'a+b|a+c|alt+g+t|1+2+3+4+5',
+      fireOnHold: true,
+      fireOnHoldInterval: 1000,
+      fireOnHoldDelay: 2000,
+      callback: ( keys: string ) => {
+        switch( keys ) {
+          case 'a+b':
+            console.log( 'you just pressed a+b!' );
+            break;
+          case 'a+c':
+            console.log( 'you just pressed a+c!' );
+            break;
+          case 'alt+g+t':
+            console.log( 'you just pressed alt+g+t' );
+            break;
+          case '1+2+3+4+5':
+            console.log( 'you just pressed 12345' );
+            break;
+          default:
+            console.log( 'you will not get here' );
+        }
+      }
+    } ) );
+
     this.disposeResetAllButton = () => {
       ariaEnabledOnFirePerUtteranceQueueMap.clear();
     };

@samreid
Copy link
Member

samreid commented Oct 21, 2022

I observed that const EnglishStringToCodeMap is missing the letter 'p'.

@jessegreenberg
Copy link
Contributor Author

Another patch from pairing with @samreid and @zepumph where we discussed pros/cons of API differences of using "natural" english strings vs a structure constants to specify keys for the listener. We also started using the type checker to encode/enforce the keys in the listener which is working well. We would like to try using the type checker to make sure that keys are in the right format when provided.

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
new file mode 100644
--- /dev/null	(date 1666371282548)
+++ b/scenery/js/listeners/KeyboardListener.ts	(date 1666371282548)
@@ -0,0 +1,228 @@
+// Copyright 2022, University of Colorado Boulder
+
+/**
+ * A listener keyboard input. Specify the keys that you want to listen to with the `keys` option in a format that looks
+ * like this:
+ *      'shift+t|alt+shift+r'
+ *
+ * A group of keys are assembled with
+ *
+ * A typical usage would like this:
+ *
+ *     someNode.addInputListener( new KeyboardListener( {
+ *       keys: 'a+b|a+c',
+ *       callback: ( keys: string ) => {
+ *         if ( keys === 'a+b' ) {
+ *           // ...
+ *         }
+ *         else if ( keys === 'a+c' ) {
+ *           // ...
+ *         }
+ *      }
+ *     } ) );
+ *
+ * By default the callback will fire when the last key is pressed down.
+ *
+ * @author Jesse Greenberg (PhET Interactive Simulations)
+ */
+
+import CallbackTimer from '../../../axon/js/CallbackTimer.js';
+import optionize from '../../../phet-core/js/optionize.js';
+import { EnglishStringToCodeMap, KeyStateTracker, scenery, SceneryEvent, TInputListener } from '../imports.js';
+import KeyboardUtils from '../accessibility/KeyboardUtils.js';
+import assertMutuallyExclusiveOptions from '../../../phet-core/js/assertMutuallyExclusiveOptions.js';
+
+const KEY_GROUP_SEPARATOR = '|';
+
+type AllowedKeys =`${number| ''}${number}:${number}${number}`;
+
+type KeyboardListenerOptions<Keys extends readonly string[]> = {
+
+  // Keys that trigger functionality for this listener.
+  // 'j+o+1' // all three keys need to be held down in order
+  // 'j+o' // these two keys need to be pressed down
+  // '1|2|3' // any of these keys are pressed
+  // 'j+1|j+2' // any of these are pressed
+  keys: Keys;
+  callback?: ( keysPressed: Keys[number] ) => void;
+  fireOnKeyUp?: boolean;
+
+  fireOnHold?: boolean;
+  fireOnHoldDelay?: number;
+  fireOnHoldInterval?: number;
+};
+
+type KeyGroup = {
+  modifierKeys: string[];
+  key: string;
+  allKeys: string[];
+  naturalKeys: string;
+};
+
+class KeyboardListener<Keys extends readonly string[]> implements TInputListener {
+
+  private readonly _keyStateTracker: KeyStateTracker;
+
+  // The CallbackTimer that will manage firing when this listener supports fireOnHold.
+  private readonly _timer?: CallbackTimer;
+
+  // The KeyGroup that is currently "active", firing the callback because keys are pressed or just released.
+  private _activeKeyGroup: KeyGroup | null;
+
+  // The function called when a KeyGroup is pressed (or just released).
+  private readonly _callback: ( key: Keys ) => void;
+
+  // Will it the callback fire on keys up or down?
+  private readonly _fireOnKeyUp: boolean;
+
+  // All of the KeyGroups of this listener from the keys provided in natural language.
+  private readonly _keyGroups: KeyGroup[];
+
+  public constructor( providedOptions: KeyboardListenerOptions<Keys> ) {
+    assert && assertMutuallyExclusiveOptions( providedOptions, [ 'fireOnKeyUp' ], [ 'fireOnHold', 'fireOnHoldInterval', 'fireOnHoldDelay' ] );
+
+    const options = optionize<KeyboardListenerOptions<Keys>>()( {
+      callback: _.noop,
+      fireOnKeyUp: false,
+      fireOnHold: false,
+      fireOnHoldDelay: 400,
+      fireOnHoldInterval: 100
+    }, providedOptions );
+
+    this._callback = options.callback;
+    this._fireOnKeyUp = options.fireOnKeyUp;
+    this._activeKeyGroup = null;
+
+    // TODO: Creating a KeyStateTracker every listener seems unnecessary and inefficient. Can one globalKeyStateTracker
+    //       drive everything but the listener queries it on keyboard events?
+    this._keyStateTracker = new KeyStateTracker();
+
+
+    // convert the provided keys to data that we can respond to with scenery's Input system
+    this._keyGroups = this.convertKeysToKeyGroups( options.keys );
+
+    if ( options.fireOnHold ) {
+      this._timer = new CallbackTimer( {
+        callback: this.fireCallback.bind( this ),
+        delay: options.fireOnHoldDelay,
+        interval: options.fireOnHoldInterval
+      } );
+    }
+  }
+
+  /**
+   * Mostly required to fire with CallbackTimer since the callback cannot take arguments.
+   */
+  public fireCallback(): void {
+    assert && assert( this._activeKeyGroup, 'Need an active keyGroup down to fire' );
+    this._callback( this._activeKeyGroup!.naturalKeys );
+  }
+
+  public keydown( event: SceneryEvent ): void {
+
+    // TODO: Ideally the tracker will soon go through scenery input system
+    // TODO: Ideally we will query a globalKeyStateTracker instead?
+    this._keyStateTracker.keydownUpdate( event.domEvent as KeyboardEvent );
+
+    if ( !this._fireOnKeyUp && !this._activeKeyGroup ) {
+
+      // modifier keys can be pressed in any order but the last key in the group must be pressed last
+      this._keyGroups.forEach( keyGroup => {
+        if ( this._keyStateTracker.areKeysDown( keyGroup.allKeys ) &&
+             this._keyStateTracker.mostRecentKeyFromList( keyGroup.allKeys ) === keyGroup.key ) {
+
+          this._activeKeyGroup = keyGroup;
+
+          if ( this._timer ) {
+            this._timer.start();
+          }
+          this.fireCallback();
+        }
+      } );
+    }
+  }
+
+  public keyup( event: SceneryEvent ): void {
+
+    // TODO: Ideally the tracker will soon go through scenery input system
+    // TODO: Ideally we will query a globalKeyStateTracker instead?
+    this._keyStateTracker.keyupUpdate( event.domEvent as KeyboardEvent );
+
+    if ( this._activeKeyGroup ) {
+      if ( !this._keyStateTracker.areKeysDown( this._activeKeyGroup.allKeys ) ) {
+        if ( this._timer ) {
+          this._timer.stop( false );
+        }
+        this._activeKeyGroup = null;
+      }
+    }
+
+    if ( this._fireOnKeyUp ) {
+      this._keyGroups.forEach( keyGroup => {
+        if ( this._keyStateTracker.areKeysDown( keyGroup.modifierKeys ) &&
+             KeyboardUtils.getEventCode( event.domEvent ) === keyGroup.key ) {
+          this._activeKeyGroup = keyGroup;
+          this.fireCallback();
+          this._activeKeyGroup = null;
+        }
+      } );
+    }
+  }
+
+  public cancel(): void {
+    // TODO
+  }
+
+  public interrupt(): void {
+    // TODO
+  }
+
+  public dispose(): void {
+    if ( this._timer ) {
+      this._timer.dispose();
+    }
+  }
+
+  /**
+   * Converts the provided keys into a collection of KeyGroups to easily track what keys are down. For example,
+   * will take a string that defines the keys for this listener like 'a+c|1+2+3+4|shift+leftArrow' and return an array
+   * with three KeyGroups, one describing 'a+c', one describing '1+2+3+4' and one describing 'shift+leftArrow'.
+   */
+  private convertKeysToKeyGroups( keys: Keys ): KeyGroup[] {
+
+    const keyGroups = keys.map( naturalKeys => {
+
+      // all of the keys in this group in an array
+      const groupKeys = naturalKeys.split( '+' );
+      assert && assert( groupKeys.length > 0, 'no keys provided?' );
+
+      const naturalKey = groupKeys.slice( -1 )[ 0 ];
+
+      // @ts-ignore - because a string shouldn't be used for lookup like this in the object type
+      const key = EnglishStringToCodeMap[ naturalKey ];
+      assert && assert( key, `Key not found, do you need to add it to EnglishStringToCodeMap? ${naturalKey}` );
+
+      let modifierKeys: string[] = [];
+      if ( groupKeys.length > 1 ) {
+        modifierKeys = groupKeys.slice( 0, groupKeys.length - 1 ).map( naturalModifierKey => {
+
+          // @ts-ignore - because a string shouldn't be used for lookup like this in the object type
+          const modifierKey = EnglishStringToCodeMap[ naturalModifierKey ];
+          assert && assert( modifierKey, `Key not found, do you need to add it to EnglishStringToCodeMap? ${naturalModifierKey}` );
+          return modifierKey;
+        } );
+      }
+      return {
+        key: key,
+        modifierKeys: modifierKeys,
+        naturalKeys: naturalKeys,
+        allKeys: modifierKeys.concat( key )
+      };
+    } );
+
+    return keyGroups;
+  }
+}
+
+scenery.register( 'KeyboardListener', KeyboardListener );
+export default KeyboardListener;
Index: scenery/js/accessibility/EnglishStringToCodeMap.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/EnglishStringToCodeMap.ts b/scenery/js/accessibility/EnglishStringToCodeMap.ts
new file mode 100644
--- /dev/null	(date 1666368148713)
+++ b/scenery/js/accessibility/EnglishStringToCodeMap.ts	(date 1666368148713)
@@ -0,0 +1,60 @@
+// Copyright 2022, University of Colorado Boulder
+
+/**
+ * @author Jesse Greenberg (PhET Interactive Simulations)
+ */
+
+import { KeyboardUtils, scenery } from '../imports.js';
+
+const EnglishStringToCodeMap = {
+  q: KeyboardUtils.KEY_Q,
+  w: KeyboardUtils.KEY_W,
+  e: KeyboardUtils.KEY_E,
+  r: KeyboardUtils.KEY_R,
+  t: KeyboardUtils.KEY_T,
+  y: KeyboardUtils.KEY_Y,
+  u: KeyboardUtils.KEY_U,
+  i: KeyboardUtils.KEY_I,
+  o: KeyboardUtils.KEY_O,
+  p: KeyboardUtils.KEY_P,
+  a: KeyboardUtils.KEY_A,
+  s: KeyboardUtils.KEY_S,
+  d: KeyboardUtils.KEY_D,
+  f: KeyboardUtils.KEY_F,
+  g: KeyboardUtils.KEY_G,
+  h: KeyboardUtils.KEY_H,
+  j: KeyboardUtils.KEY_J,
+  k: KeyboardUtils.KEY_K,
+  l: KeyboardUtils.KEY_L,
+  z: KeyboardUtils.KEY_Z,
+  x: KeyboardUtils.KEY_X,
+  c: KeyboardUtils.KEY_C,
+  v: KeyboardUtils.KEY_V,
+  b: KeyboardUtils.KEY_B,
+  n: KeyboardUtils.KEY_N,
+  m: KeyboardUtils.KEY_M,
+  0: KeyboardUtils.KEY_0,
+  1: KeyboardUtils.KEY_1,
+  2: KeyboardUtils.KEY_2,
+  3: KeyboardUtils.KEY_3,
+  4: KeyboardUtils.KEY_4,
+  5: KeyboardUtils.KEY_5,
+  6: KeyboardUtils.KEY_6,
+  7: KeyboardUtils.KEY_7,
+  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,
+
+  space: KeyboardUtils.KEY_SPACE,
+  arrowLeft: KeyboardUtils.KEY_LEFT_ARROW,
+  arrowRight: KeyboardUtils.KEY_RIGHT_ARROW,
+  arrowUp: KeyboardUtils.KEY_UP_ARROW,
+  arrowDown: KeyboardUtils.KEY_DOWN_ARROW
+};
+
+scenery.register( 'EnglishStringToCodeMap', EnglishStringToCodeMap );
+export default EnglishStringToCodeMap;
\ No newline at end of file
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 b4ac2850e20a6fd9b43f7dedfad38ae03f3c1a03)
+++ b/scenery/js/accessibility/KeyStateTracker.ts	(date 1666312056913)
@@ -167,7 +167,7 @@
    * `Node.addInputListener` only supports type properties as event listeners, and not the event keys as
    * prototype methods. Please see https://github.com/phetsims/scenery/issues/851 for more information.
    */
-  private keydownUpdate( domEvent: KeyboardEvent ): void {
+  public keydownUpdate( domEvent: KeyboardEvent ): void {
     this.enabled && this.keydownUpdateAction.execute( domEvent );
   }
 
@@ -225,7 +225,7 @@
    * `Node.addInputListener` only supports type properties as event listeners, and not the event keys as
    * prototype methods. Please see https://github.com/phetsims/scenery/issues/851 for more information.
    */
-  private keyupUpdate( domEvent: KeyboardEvent ): void {
+  public keyupUpdate( domEvent: KeyboardEvent ): void {
     this.enabled && this.keyupUpdateAction.execute( domEvent );
   }
 
@@ -262,6 +262,11 @@
     return false;
   }
 
+  public mostRecentKeyFromList( keys: string[] ): string {
+    assert && assert( this.areKeysDown( keys ), 'Not all the keys in the list are down.' );
+    return _.minBy( keys, key => this.timeDownForKey( key ) )!;
+  }
+
   /**
    * Returns true if ALL of the keys in the list are currently down. Values of the keyList array are the
    * KeyboardEvent.code for the keys you are interested in.
Index: scenery/js/imports.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/imports.ts b/scenery/js/imports.ts
--- a/scenery/js/imports.ts	(revision b4ac2850e20a6fd9b43f7dedfad38ae03f3c1a03)
+++ b/scenery/js/imports.ts	(date 1666313997958)
@@ -24,6 +24,7 @@
 export { default as Utils } from './util/Utils.js';
 export { default as Focus } from './accessibility/Focus.js';
 export { default as KeyboardUtils } from './accessibility/KeyboardUtils.js';
+export { default as EnglishStringToCodeMap } from './accessibility/EnglishStringToCodeMap.js';
 export { default as EventIO } from './input/EventIO.js';
 export { default as SceneryStyle } from './util/SceneryStyle.js';
 export { default as CanvasContextWrapper } from './util/CanvasContextWrapper.js';
@@ -227,6 +228,7 @@
 export { default as HandleDownListener } from './listeners/HandleDownListener.js';
 export { default as KeyboardDragListener } from './listeners/KeyboardDragListener.js';
 export type { KeyboardDragListenerOptions } from './listeners/KeyboardDragListener.js';
+export { default as KeyboardListener } from './listeners/KeyboardListener.js';
 export { default as SpriteListenable } from './listeners/SpriteListenable.js';
 export { default as SwipeListener } from './listeners/SwipeListener.js';
 
Index: scenery-phet/js/buttons/ResetAllButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/buttons/ResetAllButton.ts b/scenery-phet/js/buttons/ResetAllButton.ts
--- a/scenery-phet/js/buttons/ResetAllButton.ts	(revision 7d330829d496ffb6ce91b1e123981ea6d0f66d1e)
+++ b/scenery-phet/js/buttons/ResetAllButton.ts	(date 1666371527031)
@@ -8,7 +8,7 @@
  * @author Chris Malley (PixelZoom, Inc.)
  */
 
-import { voicingUtteranceQueue } from '../../../scenery/js/imports.js';
+import { KeyboardListener, voicingUtteranceQueue } from '../../../scenery/js/imports.js';
 import StrictOmit from '../../../phet-core/js/types/StrictOmit.js';
 import resetAllSoundPlayer from '../../../tambo/js/shared-sound-players/resetAllSoundPlayer.js';
 import optionize from '../../../phet-core/js/optionize.js';
@@ -127,6 +127,65 @@
       } );
     } );
 
+    this.addInputListener( new KeyboardListener( {
+      keys: [ 'a,b', 'a+c', 'alt+g+t', '1+2+3+4+5', 'shift+arrowLeft' ] as const,
+      callback: keys => {
+        switch( keys ) {
+          case 'a,b':
+            console.log( 'you just pressed a+b!' );
+            break;
+          case 'a+c':
+            console.log( 'you just pressed a+c!' );
+            break;
+          case 'alt+g+t':
+            console.log( 'you just pressed alt+g+t' );
+            break;
+          case '1+2+3+4+5':
+            console.log( 'you just pressed 1+2+3+4+5' );
+            break;
+          case 'shift+arrowLeft':
+            console.log( 'you just pressed shift+arrowLeft' );
+            break;
+          default:
+            console.log( 'you will not get here' );
+        }
+      }
+    } ) );
+
+    // THis could be an actual type. keys
+    // 'a,b|a+c|alt+g+t|1+2+3+4+5|shift+arrowLeft'
+
+    type MyKeys = 'a,b' | 'a+c';
+
+    const AC = { key: KEY_C, modifierKeys: [ KEY_A ] };
+    // 'a,b|a+c|alt+g+t|1+2+3+4+5|shift+arrowLeft'
+    this.addInputListener( new KeyboardListener( {
+      keys: [
+        [ { key: KEY_A }, { key: KEY_B } ],
+        AC,
+        { key: KEY_C, modifierKeys: [ ALT, KEY_G, KEY_T ] }
+        // ...
+      ],
+      callback: ( keys: string, listener: any ) => {
+        if ( keys === 'a+b' ) {
+          console.log( 'you just pressed a+b!' );
+        }
+        else if ( keys === AC ) {
+          console.log( 'you just pressed a+c!' );
+        }
+        else if ( keys === 'alt+g+t' ) {
+          console.log( 'you just pressed alt+g+t' );
+        }
+        else if ( keys === '1+2+3+4+5' ) {
+          console.log( 'you just pressed 1+2+3+4+5' );
+        }
+        else if ( keys === 'shift+arrowLeft' ) {
+          console.log( 'you just pressed shift+arrowLeft' );
+        }
+      }
+    } ) );
+
+
     this.disposeResetAllButton = () => {
       ariaEnabledOnFirePerUtteranceQueueMap.clear();
     };

@zepumph
Copy link
Member

zepumph commented Oct 21, 2022

We got to a commit point, and then made an improvement to use globalKeyStateTracker. Thanks for getting the ball rolling @jessegreenberg.

Next big steps that I know of (feel free to edit inline)

  • Callback should run on SceneryEvent so that it can be aborted etc.
  • Rework PDOM events generally, especially when assumeFullWindow
  • Move globalKeyStateTracker logic into Input.ts to ensure that state is updated before Node dispatching/listeners.
  • Unit tests for automation.
  • SceneryEvent likely should store some data from the keyboard state so that KeyboardListener doesn't need to use a global
  • Convert usages of old patterns (using KeyboardUtils equality checking) to KeyboardListener.
  • Create a registry for groups of keys that have behavior.
  • Probably something else!

We should check in on microsoft/TypeScript#41160 every now and then so that our keyboard language can get more powerful than it is now.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Oct 25, 2022

  • Consider how sticky keys might impact this work. That feature from the OS needs to work well with the listener.
  • How to handle meta keys? See some previous work in KeyboardZoomUtils.
  • Bug in KeyStateTracker.mostRecentKeyFromList where we can't rely on time, and should instead store the last key pressed, and let's rename it to something more "keydowny" just in case we need the same logic for "last keyup key", which we may immediately.

Patch from most recent session:

Index: geometric-optics/js/common/view/tools/GOToolKeyboardDragListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/geometric-optics/js/common/view/tools/GOToolKeyboardDragListener.ts b/geometric-optics/js/common/view/tools/GOToolKeyboardDragListener.ts
--- a/geometric-optics/js/common/view/tools/GOToolKeyboardDragListener.ts	(revision 6493ad5aef2397689c336efc8703e8dc7c16998c)
+++ b/geometric-optics/js/common/view/tools/GOToolKeyboardDragListener.ts	(date 1666736324787)
@@ -8,7 +8,7 @@
 
 import Bounds2 from '../../../../../dot/js/Bounds2.js';
 import ModelViewTransform2 from '../../../../../phetcommon/js/view/ModelViewTransform2.js';
-import { KeyboardDragListener, KeyboardDragListenerOptions, KeyboardUtils } from '../../../../../scenery/js/imports.js';
+import { KeyboardDragListener, KeyboardDragListenerOptions, KeyboardListener } from '../../../../../scenery/js/imports.js';
 import geometricOptics from '../../../geometricOptics.js';
 import GOConstants from '../../GOConstants.js';
 import TReadOnlyProperty from '../../../../../axon/js/TReadOnlyProperty.js';
@@ -67,20 +67,23 @@
     } );
 
     // Escape returns the tool to the toolbox.
-    this.addHotkey( {
-      keys: [ KeyboardUtils.KEY_ESCAPE ],
-      callback: () => {
-        phet.log && phet.log( 'hotkey ESCAPE' );
-        returnToToolbox();
-      }
-    } );
-    this.addHotkey( {
-      keys: [ KeyboardUtils.KEY_J ],
-      callback: () => {
-        phet.log && phet.log( 'hotkey J' );
-        toolNode.jumpToPoint();
+    toolNode.addInputListener( new KeyboardListener( {
+      keys: [ 'escape', 'j' ],
+      callback: ( event, listener ) => {
+        const keysFired = listener.keysFired;
+        if ( keysFired === 'escape' ) {
+          phet.log && phet.log( 'hotkey ESCAPE' );
+          returnToToolbox();
+        }
+        else if ( keysFired === 'j' ) {
+          phet.log && phet.log( 'hotkey J' );
+          toolNode.jumpToPoint();
+        }
+        else {
+          assert && assert( false, 'No other supported keys for this listener, how did we get here?', keysFired );
+        }
       }
-    } );
+    } ) );
   }
 }
 
Index: scenery/js/listeners/KeyboardListenerTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/listeners/KeyboardListenerTests.ts b/scenery/js/listeners/KeyboardListenerTests.ts
--- a/scenery/js/listeners/KeyboardListenerTests.ts	(revision 0a57ff86efd38daf0fdfbcc89cbf332e39982764)
+++ b/scenery/js/listeners/KeyboardListenerTests.ts	(date 1666736330361)
@@ -47,6 +47,37 @@
   } ) );
   assert.ok( callbackFired, 'should fire on enter' );
 
+  a.removeInputListener( listener );
+
+  let pFired = false;
+  let ctrlPFired = false;
+  a.addInputListener( new KeyboardListener( {
+    keys: [ 'p', 'ctrl+p' ],
+    callback: ( event, listener ) => {
+
+      debugger;
+      const keysFired = listener.keysFired;
+      if ( keysFired === 'p' ) {
+        pFired = true;
+      }
+      else if ( keysFired === 'ctrl+p' ) {
+        ctrlPFired = true;
+      }
+      else {
+        assert.ok( false, 'never again' );
+      }
+    }
+  } ) );
+  domElement.dispatchEvent( new KeyboardEvent( 'keydown', {
+    code: KeyboardUtils.KEY_P,
+    ctrlKey: true,
+    bubbles: true
+  } ) );
+  assert.ok( pFired, 'p should have fired' );
+  assert.ok( ctrlPFired, 'ctrl P should have fired' );
+
+
+  //////////////////////////////////////////////////////
   document.body.removeChild( display.domElement );
   display.dispose();
 } );
\ No newline at end of file
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 0a57ff86efd38daf0fdfbcc89cbf332e39982764)
+++ b/scenery/js/listeners/KeyboardListener.ts	(date 1666737267913)
@@ -14,23 +14,27 @@
  *
  * An example usage would like this:
  *
- *     this.addInputListener( new KeyboardListener( {
- *       keys: [ 'a+b', 'a+c', 'shift+arrowLeft', 'alt+g+t', 'ctrl+3', 'alt+ctrl+t' ] as const,
- *       callback: keys => {
+ *    this.addInputListener( new KeyboardListener( {
+ *       keys: [ 'a+b', 'a+c', 'shift+arrowLeft', 'alt+g+t', 'ctrl+3', 'alt+ctrl+t' ],
+ *       callback: ( event, listener ) => {
+ *         const keysFired = listener.keysFired;
  *
- *         if ( keys === 'a+b' ) {
+ *         if ( keysFired === 'a+b' ) {
  *           console.log( 'you just pressed a+b!' );
+ *           if ( event ) {
+ *             event.handled = true;
+ *           }
  *         }
- *         else if ( keys === 'a+c' ) {
+ *         else if ( keysFired === 'a+c' ) {
  *           console.log( 'you just pressed a+c!' );
  *         }
- *         else if ( keys === 'alt+g+t' ) {
+ *         else if ( keysFired === 'alt+g+t' ) {
  *           console.log( 'you just pressed alt+g+t' );
  *         }
- *         else if ( keys === 'ctrl+3' ) {
+ *         else if ( keysFired === 'ctrl+3' ) {
  *           console.log( 'you just pressed ctrl+3' );
  *         }
- *         else if ( keys === 'shift+arrowLeft' ) {
+ *         else if ( keysFired === 'shift+arrowLeft' ) {
  *           console.log( 'you just pressed shift+arrowLeft' );
  *         }
  *       }
@@ -76,7 +80,7 @@
   // '1|2|3' // any of these keys are pressed
   // 'j+1|j+2' // any of these are pressed
   keys: Keys;
-  callback?: ( keysPressed: Keys[number] ) => void;
+  callback?: ( event: SceneryEvent<KeyboardEvent> | null, listener: KeyboardListener<Keys> ) => void;
   fireOnKeyUp?: boolean;
 
   fireOnHold?: boolean;
@@ -84,6 +88,7 @@
   fireOnHoldInterval?: number;
 };
 
+// TODO: How does this extend OneKeyStroke?
 type KeyGroup<Keys extends readonly OneKeyStroke[]> = {
 
   // All must be pressed fully before the key is pressed to activate the command.
@@ -102,13 +107,12 @@
   private _activeKeyGroup: KeyGroup<Keys> | null;
 
   // The function called when a KeyGroup is pressed (or just released).
-  // TODO: callback should take a SCeneryEvent. https://github.com/phetsims/scenery/issues/1445
-  private readonly _callback: ( keysPressed: Keys[number] ) => void;
+  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;
 
-  // All of the KeyGroups of this listener from the keys provided in natural language.
+  // All the KeyGroups of this listener from the keys provided in natural language.
   private readonly _keyGroups: KeyGroup<Keys>[];
 
   public constructor( providedOptions: KeyboardListenerOptions<Keys> ) {
@@ -131,23 +135,38 @@
 
     if ( options.fireOnHold ) {
       this._timer = new CallbackTimer( {
-        callback: this.fireCallback.bind( this ),
+
+        // TODO: We do want to dispatch to all the key groups!
+        callback: () => this.dispatchToKeyGroups( null ),
         delay: options.fireOnHoldDelay,
         interval: options.fireOnHoldInterval
       } );
     }
   }
 
+  public get keysFired(): Keys[number] {
+    assert && assert( this._activeKeyGroup, 'Need an active keyGroup down to fire' );
+    return this._activeKeyGroup!.naturalKeys;
+  }
+
   /**
    * Mostly required to fire with CallbackTimer since the callback cannot take arguments.
    */
-  public fireCallback(): void {
+  public fireCallback( event: SceneryEvent<KeyboardEvent> | null ): void {
     assert && assert( this._activeKeyGroup, 'Need an active keyGroup down to fire' );
-    this._callback( this._activeKeyGroup!.naturalKeys );
+    this._callback( event, this );
   }
 
   public keydown( event: SceneryEvent<KeyboardEvent> ): void {
 
+    // PROBLEMS:
+    // mostRecentKeyFromList is buggy, it particularly with correctMOdifierKeys, both go down at the same time.
+    // This strategy doesn't work with overlapping key groups like [ 'p', 'ctrl+p' ] - only the first fires
+      // MAYBE:
+      // We want both to fire in this case. So we need to support multiple active key groups at once.
+      // Have an array of active KeyGroups. Each KeyGroup has a CallbackTimer and it is up to
+
+    // TODO: Do we still want to only fire if there is no activeKeyGroup?
     if ( !this._fireOnKeyUp && !this._activeKeyGroup ) {
 
       // modifier keys can be pressed in any order but the last key in the group must be pressed last
@@ -158,12 +177,18 @@
 
           this._activeKeyGroup = keyGroup;
 
+          // TODO: Assert that this only happens once per keyboard event.
           if ( this._timer ) {
             this._timer.start();
           }
-          this.fireCallback();
+          // TODO: FOR NEXT TIME: UPdate the activeKeyGroup here (basically set it to null AFTER the callback)
+          this.fireCallback( event );
         }
       } );
+
+      if ( !this._timer ) {
+        this._activeKeyGroup = null;
+      }
     }
   }
 
@@ -185,7 +210,7 @@
         if ( globalKeyStateTracker.areKeysDown( keyGroup.modifierKeys ) &&
              KeyboardUtils.getEventCode( event.domEvent ) === keyGroup.key ) {
           this._activeKeyGroup = keyGroup;
-          this.fireCallback();
+          this.fireCallback( event );
           this._activeKeyGroup = null;
         }
       } );

This change set includes notes from lots of discussion including

  • Replacing KeyboardDragListener.hotkeys with KeyboardListener.
  • Identifies problem and new unit test for when ctrl key is down and we have key groups like [ 'p', 'ctrl+p' ],
  • Updated documentation for when we support SceneryEvent in the callback AND fire a listener.
  • Identifies a problem with mostRecentKeyFromList

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Oct 26, 2022

The above commit gives each KeyGroup its own CallbackTimer to support listener that can have more than one key groups down at a time. It also replaces mostRecentKeyFromList. That unit test is now passing.

c8c375a passes the scenery event through where possible.

feaaddf Adds unit tests for abort/handle of the SceneryEvent.

Notes from brainstorm with full team on 10/26/22

    // We need to support events to a Node, up the scene graph, to a display, and to the window.
    // only attach to window if your display takes up the full screen (for mouse events)
    // For global keyboard listeners we would NEED to attach to the window.
    // So we have one global thing tracking key state.

    // Each display adds listeners to whatever it is listening to (presumably window) and dispatches accordingly
    // with information from the global key state tracker.

    // Complexities around interruption and cancel when moving outside of the window. Or when changing focus.

    // We will need to be careful of preventDefault() usages throughout.
    // We will need to make sure that attaching listeners to the window works so you don't need to click in the sim to
    // get global listeners to fire.

    // globalKeydown
    // recurses "entire scene graph" for everthing that could possibly receive input and dispatches to them.
    // Hopefully similar to hit testing but there isn't an event trail??
    // Should be able to prune a lot because most things will not have global key listeners.

    // If `keydown` and `globalKeydown` are bth added, BOTH should fire.
    // What order??
    // What order for global events??
    // Visual top to visual bottom
    // depth first vs breadth first
    // If we decide on this we could handle or abort the event.

    // What order in the event dispatch (scene graph -> pointer -> display -> ...)
    // globalKeydown is always last
    // That way leaf most Nodes in the scene graph can handle/abort more global behavior
    // However, it seems like global listeners (like modifier key behavior) should fire first for expected behavior.
    // What about TWO dispatches for global events. Once before the scene-graph dispatch, one after so you can do either?
    // related to useCapture?
    // javascript.info/bubbling-and-capturing
    // A different capture phase would let you basically control the bubbling direction so you can control this on a per-listener level.

    // How/when do we attach the globalKeyStateTracker?
      // globalKeyStateTracker gets attached on the first creation of Input or input.connectListeners/Display.initializeEvents.
      // It never gets detached??
        // Complexity around working with multiple Displays, if you initializeEvents for several, then dispose one, you
        // still want globalKeyStateTracker active for the remaining display.

    // Tentative decisions:
    // globalkeydown, globalkeyup as new events.
    // Add useCapture to TInputListener that new events can use.
    // Ordering the global event dispatch:
      // Dispatching last child first (z-order), then self, (that is depth first in order)
    // The requirements for dispatching to a target is that trails to the instance must have `inputEnabled` and `visible` true.
      // We are going to be more stringent on requirements because it is easier to get events and prevent behavior with logic
      // than it is to work around not getting events at all.


    checkAnswerButton.addInputListener( {

      // BOTH should fire. Convention is to have the event name all lowercase.
      globalkeydown: ( event: SceneryEvent<KeyboardEvent> ) => {

        // inputEnabled, visible
        // excluding enabled, pickable, pdomVisible

        // pickable is very mouse/touch centric and should have no bearing on keyboard input.
        // We want to exclude pickable for the above reason.
        console.log( 'The checkAnswerButton is hittable but focus is anywhere!' );
      },
      keydown: event => {
        console.log( 'You just pressed a key while I (or a descendant) have focus!' );
      }
    } );

    // when my instance of a display has focus
    sim.display.addInputListener( {

      // It is strange that a global listener would be added to a particular Node, what about this?
      // But if we specify a targetNode this way it will be harder to remove listeners.
      targetNode: checkAnswerButton,
      keydown: event => {
        console.log( 'Focus is somewhere inside sim.display so Ill fire!' );
      }
    } );

    // static, no matter where focus is Ill fire
    Display.addInputListener( {
      keydown: event => {
        console.log( 'No matter where focus is, Ill fire!' );
      }
    } );

    // another thought, what about a way to "expand the scope of focus" for firing listener. Analog to expanding mouse/touch area.
    myNode.addInputListener( {
      focusScope: 'any'; // or 'screen' or screenViewNode
      keydown: event => {
        // focus was under the focusScope so Ill fire!
        // For this case the listener can be put on the parent you want.
        // But you would still need guards to make sure myNode is still hittable!
      }
    } );

jonathanolson added a commit that referenced this issue Oct 26, 2022
…ng, but conditionally dispatching based on whether accessible and the previous conditions). See #1445
zepumph added a commit that referenced this issue Oct 26, 2022
@zepumph
Copy link
Member

zepumph commented Oct 26, 2022

Consolidating remainging checkboxes above, and more from discussion with @jessegreenberg and @samreid:

  • use globalkeydown and globalkeyup in KeyboardListener sometimes
  • Convert usages of old patterns (using KeyboardUtils equality checking) to KeyboardListener.
  • Create a registry for groups of keys that have behavior.
  • How to handle meta keys? See some previous work in KeyboardZoomUtils.
  • Consider how sticky keys might impact this work. That feature from the OS needs to work well with the listener.

@jessegreenberg
Copy link
Contributor Author

There are a few usages of globalKeyStateTracker.keyupEmitter that are not in view components like voicingManager, localeProperty, phetioDevSaveLoad. If we re-implement those with KeyboardListener, where should they be added, to the static Display.addInputListener?

@jessegreenberg
Copy link
Contributor Author

This issue became about adding scenery support for "global" keyboard events as well as adding a more generalized KeyboardListener. It also moved the alt input system into BrowserEvents so that alt input behaves exactly like other scenery input (batching, window/Display attachment).

For the next steps of the new KeyboardListener, see #1520.

Creating an automated registry of global hotkeys will take more time than we have right now. See phetsims/phet-info#211 for next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants