Global key events are not captured for PhET-iO input

[jessegreenberg]

Seen in the mirror inputs wrapper, global key events (where focus is not within the simulation div) such as those used in AnimatedPanZoomListener (ctrl + '+' or ctrl + '-') are not captured.

This is for https://github.com/phetsims/phet-io/issues/1705 so is high priority and blocking publication as was that issue.

[jessegreenberg]

I tried instrumenting the Emitters of KeyStateTracker, but that isn't working and I don't think that is the correct approach. I am guess that what we need is Actions like the ones in Input.js that respond to native DOM events on the body. Then KeyStateTracker could attach listeners to these rather than itself attaching to the body.

[jessegreenberg]

This patch is working well, it adds to more Actions to Input.js for global key presses. There are problems with it still, such as the actions update the Display.keyStateTracker and it shouldn't be updating the static. This includes a change to AnimatedPanZoomListener to use Display.keyStateTracker instead of its own KeyStateTracker.

Index: js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/display/Display.js	(revision cbaa703108470e3527151ce9008e7129a23fe3c7)
+++ js/display/Display.js	(date 1606794896449)
@@ -2189,6 +2189,6 @@
 // @public (read-only) {KeyStateTracker} - A global object that tracks the state of the keyboard for all Displays. Use this
 // to get information about which keyboard keys are pressed down and for how long.
 Display.keyStateTracker = new KeyStateTracker();
-Display.keyStateTracker.attachToDocument();
+// Display.keyStateTracker.attachToDocument();
 
 export default Display;
\ No newline at end of file
Index: js/input/Input.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/input/Input.js	(revision cbaa703108470e3527151ce9008e7129a23fe3c7)
+++ js/input/Input.js	(date 1606795486127)
@@ -741,6 +741,37 @@
         }, accessibleEventOptions );
       } );
 
+      this.globalKeydownAction = new Action( event => {
+        Display.keyStateTracker.keydownUpdate( event );
+      }, {
+        phetioPlayback: true,
+        tandem: options.tandem.createTandem( 'globalKeydownAction' ),
+        parameters: [
+          { name: 'event', phetioType: EventIO }
+        ],
+        phetioEventType: EventType.USER,
+        phetioDocumentation: 'Emits when the window receives keydown Events.'
+      } );
+      this.globalKeyupAction = new Action( event => {
+        Display.keyStateTracker.keyupUpdate( event );
+      }, {
+        phetioPlayback: true,
+        tandem: options.tandem.createTandem( 'globalKeyupAction' ),
+        parameters: [
+          { name: 'event', phetioType: EventIO }
+        ],
+        phetioEventType: EventType.USER,
+        phetioDocumentation: 'Emits when the window receives keydown Events.'
+      } );
+
+      window.addEventListener( 'keydown', event => {
+        this.globalKeydownAction.execute( event );
+      }, true );
+
+      window.addEventListener( 'keyup', event => {
+        this.globalKeyupAction.execute( event );
+      }, true );
+
       // Add a listener to the document body that will capture any keydown for a11y before focus is in this display.
       document.body.addEventListener( 'keydown', this.handleDocumentKeydown.bind( this ) );
     }
Index: js/listeners/AnimatedPanZoomListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/listeners/AnimatedPanZoomListener.js	(revision cbaa703108470e3527151ce9008e7129a23fe3c7)
+++ js/listeners/AnimatedPanZoomListener.js	(date 1606796347075)
@@ -43,8 +43,8 @@
     super( targetNode, options );
 
     // @private {KeyStateTracker}
-    this.keyStateTracker = new KeyStateTracker();
-    this.keyStateTracker.attachToDocument();
+    // this.keyStateTracker = new KeyStateTracker();
+    // this.keyStateTracker.attachToDocument();
 
     // @private {null|Vector2} - This point is the center of the transformedPanBounds (see PanZoomListener) in
     // the parent coordinate frame of the targetNode. This is the current center of the transformedPanBounds, and
@@ -122,7 +122,7 @@
 
     // Handle key input from events outside of the PDOM - in this case it is impossible for the a11y pointer
     // to be attached so we have free reign over the keyboard
-    this.keyStateTracker.keydownEmitter.addListener( this.documentKeydown.bind( this ) );
+    Display.keyStateTracker.keydownEmitter.addListener( this.documentKeydown.bind( this ) );
 
     const displayFocusListener = focus => {
       if ( focus ) {
@@ -141,7 +141,7 @@
 
       Display.focusProperty.unlink( displayFocusListener );
 
-      this.keyStateTracker.dispose();
+      // this.keyStateTracker.dispose();
     };
   }
 
@@ -160,7 +160,7 @@
     // the pan bounds so that it is always visible while being moved around
     if ( this._pdomPointerWithIntent ) {
       const pointerHasIntent = this.hasDragIntent( this._pdomPointerWithIntent );
-      if ( pointerHasIntent && this.keyStateTracker.movementKeysDown && Display.focusProperty.value !== null ) {
+      if ( pointerHasIntent && Display.keyStateTracker.movementKeysDown && Display.focusProperty.value !== null ) {
         const focusedNode = Display.focusedNode;
         if ( !this._panBounds.containsBounds( focusedNode.globalBounds ) ) {
           this.panToNode( focusedNode );
@@ -325,7 +325,7 @@
 
       // handle translation without worry of the pointer being attached because there is no pointer at this level
       if ( KeyboardUtils.isArrowKey( domEvent.keyCode ) ) {
-        const keyPress = new KeyPress( this.keyStateTracker, this.getCurrentScale(), this._targetScale );
+        const keyPress = new KeyPress( Display.keyStateTracker, this.getCurrentScale(), this._targetScale );
         this.repositionFromKeys( keyPress );
       }
     }
@@ -357,7 +357,7 @@
         sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'MultiListener handle arrow key down' );
         sceneryLog && sceneryLog.InputListener && sceneryLog.push();
 
-        const keyPress = new KeyPress( this.keyStateTracker, this.getCurrentScale(), this._targetScale );
+        const keyPress = new KeyPress( Display.keyStateTracker, this.getCurrentScale(), this._targetScale );
         this.repositionFromKeys( keyPress );
 
         sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
@@ -389,7 +389,7 @@
       domEvent.preventDefault();
 
       const nextScale = this.getNextDiscreteScale( zoomInCommandDown );
-      const keyPress = new KeyPress( this.keyStateTracker, nextScale, this._targetScale );
+      const keyPress = new KeyPress( Display.keyStateTracker, nextScale, this._targetScale );
       this.repositionFromKeys( keyPress );
     }
     else if ( KeyboardZoomUtils.isZoomResetCommand( domEvent ) ) {

@zepumph can we talk about this approach soon?

[zepumph]

@samreid and I discussed this briefly and think that it may be more valuable to just instrument keyStateTracker itself, and make its Emitters phetioPlayback:true. @jessegreenberg let's talk about this more!

[jessegreenberg]

Discussed again with @zepumph - currently, the KeyStateTracker does not update itself in response to the keyup and keydown Emitters, so instrumenting these Emitters will not convey state. Instead what we need to do is create actions for keydownUpdate and keyUpUpdate so that the KeyStateTracker will be up to date in the downstream sim.

[zepumph]

@jessegreenberg, I don't know if we ever got to the bottom of if this should be Tandem.REQUIRED or Tandem.OPTIONAL. Can you think of any KeyStateTrackers that don't need to be instrumented? I think Tandem.OPTIONAL is best, see BothHandsInteractionListener as an example.

[samreid]

Should KeyStateTracker be a static global singleton? I'm trying to understand why you should be able to create and dispose multiple instances as opposed to just listening to a singleton. If it was a singleton, it would be REQUIRED, and always instrumented, and playbackable:true.

[zepumph]

@jessegreenberg and I have had this conversation in the past. The trade off is about finding a single place that all usages can have a dependency on. Today we discussed why I feel like Display is not the place for a single spot, and Sim doesn't work unless we add a component dependencies on sim (like PlayPauseButton). The burden of creating multiple keystate trackers feel worth it to maintain modular code that is encapsulated.

[samreid]

Can we make it a top-level singleton in scenery (not associated with Display)?

[zepumph]

I don't feel like it is in the scope of scenery, a scene-graph library that deals with the rendering and input within a Display.

We could make it its own type withint phetcommon or something, but whereever we create the singleton, we should likely attach it to the document too. Can we have a type that upon import adds those listeners to the document? Where would we import it as a central location?

[jessegreenberg]

In my opinion, KeyStateTracker is very much a part of input. What if we had a singleton instance initialized and listeners added to the document as part of Display.initializeEvents?

EDIT: Or in BrowserEvents.addDisplay?

[jessegreenberg]

KeyStateTracker was instrumented in the above commit. @zepumph can you please review this change, as well as my last comment?

[zepumph]

In my opinion, KeyStateTracker is very much a part of input.

Yes, input into the sim, but not input into the Display. Global/DOM-level input invents are getting annexed by scenery just because that is where we control input for the Display (100% of the input to the sim other than this global control). I feel like I am stating my opinion, as well as trying to channel what I felt @jonathanolson said in a previous conversation about where Global key event listeners go.

[zepumph]

I made some minor changes and added doc. Please review.

@jessegreenberg and I also discussed making sure that PDOM DOM events won't get played back twice as the capture/bubble to window from the PDOM.

See #1048 (comment) and lower for discussion about where KeyStateTracker should go. We are settling on a singleton in scenery.

[zepumph]

@jessegreenberg anything else here?

[zepumph]

Oh! I think that this should maybe be under general.view instead of model. On hold until the refactor inhttps://github.com//issues/1048.

[zepumph]

@samreid how does the above look? Should it be under "controller" instead? No preference here. After making the phetioID as you like, please pass back to @jessegreenberg to see if there is anything more for this issue.

[samreid]

I considered "model" since it is about key states, but it seems preferable to associate it more with the input side. But I'm not sure whether general.controller or general.controller.input is better.

[zepumph]

I like general.controller. Thoughts?

[samreid]

That seems reasonable to me.

[zepumph]

Thanks! How does the above look to you?

[samreid]

Looks great, thanks! Over to @jessegreenberg

[jessegreenberg]

I reviewed 7ebe567, looks good. I think that is it for this issue.