Alt input and UI sound for BicyclePumpNode

[pixelzoom]

Needed for Gas Properties suite PhET-iO release, phetsims/gas-properties#191.

From phetsims/gas-properties#213:

• BicyclePumpNode - somewhat like a slider, except particle come out only on the down stroke.

From phetsims/gas-properties#214:

• BicyclePumpNode - are grab/release sufficient, or should this sound like a slider, or something else?

[pixelzoom]

From phetsims/gas-properties#213 (comment), here are @terracoda's thoughts on alt input BicyclePumpNode:

For the bicycle pump we could look at mapping that to a slider, going up/left is loading with air and going down/right is pumping out particles and increasing pressure. We could explore the standard types of slider shortcuts to pump a little or a lot.

[jessegreenberg]

Proof of concept in a patch, but I am not sure about it. I used KeyboardDragListener instead of AccessibleSlider. That was a bit easier because of the Property the BicyclePumpNode controls - the number of particles instead of the height of the handle.

Subject: [PATCH] Remove opt out of Interactive Highlights, see https://github.com/phetsims/friction/issues/201
---
Index: js/BicyclePumpNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/BicyclePumpNode.ts b/js/BicyclePumpNode.ts
--- a/js/BicyclePumpNode.ts	(revision 69f356768c26e0d2af3ca23e272f118cc16f3ef2)
+++ b/js/BicyclePumpNode.ts	(date 1713383486083)
@@ -19,7 +19,7 @@
 import { Shape } from '../../kite/js/imports.js';
 import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
 import optionize, { combineOptions } from '../../phet-core/js/optionize.js';
-import { Circle, DragListener, DragListenerOptions, TColor, LinearGradient, Node, NodeOptions, PaintColorProperty, Path, PressedDragListener, PressListenerEvent, Rectangle, SceneryConstants } from '../../scenery/js/imports.js';
+import { Circle, DragListener, DragListenerOptions, KeyboardDragListener, LinearGradient, Node, NodeOptions, PaintColorProperty, Path, PressedDragListener, PressListenerEvent, Rectangle, SceneryConstants, TColor } from '../../scenery/js/imports.js';
 import Tandem from '../../tandem/js/Tandem.js';
 import sceneryPhet from './sceneryPhet.js';
 import SegmentedBarGraphNode from './SegmentedBarGraphNode.js';
@@ -95,8 +95,8 @@
   private readonly pumpShaftNode: Node;
   private readonly pumpHandleNode: Node;
 
-  // DragListener for the pump handle
-  private readonly handleDragListener: PumpHandleDragListener;
+  // Contains the drag listeners for the pump handle
+  private readonly inputListenerContainer: InputListenerContainer;
 
   private readonly disposeBicyclePumpNode: () => void;
 
@@ -287,13 +287,14 @@
     const maxHandleYOffset = this.pumpHandleNode.centerY;
     const minHandleYOffset = maxHandleYOffset + ( -PUMP_SHAFT_HEIGHT_PROPORTION * pumpBodyHeight );
 
-    this.handleDragListener = new PumpHandleDragListener( numberProperty, rangeProperty, this.nodeEnabledProperty,
+    this.inputListenerContainer = new InputListenerContainer( numberProperty, rangeProperty, this.nodeEnabledProperty,
       options.injectionEnabledProperty, minHandleYOffset, maxHandleYOffset, this.pumpHandleNode, this.pumpShaftNode,
       combineOptions<PumpHandleDragListenerOptions>( {
         tandem: options.tandem.createTandem( 'handleDragListener' )
       }, options.dragListenerOptions )
     );
-    this.pumpHandleNode.addInputListener( this.handleDragListener );
+    this.pumpHandleNode.addInputListener( this.inputListenerContainer.dragListener );
+    this.pumpHandleNode.addInputListener( this.inputListenerContainer.keyboardDragListener );
 
     // add the pieces with the correct layering
     this.addChild( pumpBaseNode );
@@ -318,7 +319,7 @@
     assert && phet?.chipper?.queryParameters?.binder && InstanceRegistry.registerDataURL( 'scenery-phet', 'BicyclePumpNode', this );
 
     this.disposeBicyclePumpNode = () => {
-      this.handleDragListener.dispose(); // to unregister tandem
+      this.inputListenerContainer.dispose(); // to unregister tandem
 
       if ( ownsEnabledProperty ) {
         this.nodeEnabledProperty.dispose();
@@ -339,7 +340,7 @@
 
   public reset(): void {
     this.setPumpHandleToInitialPosition();
-    this.handleDragListener.reset();
+    this.inputListenerContainer.reset();
   }
 
   public override dispose(): void {
@@ -644,10 +645,13 @@
 
 type PumpHandleDragListenerOptions = PumpHandleDragListenerSelfOptions & StrictOmit<DragListenerOptions<PressedDragListener>, 'drag'>;
 
-class PumpHandleDragListener extends DragListener {
+class InputListenerContainer {
 
   private lastHandlePosition: number | null;
 
+  public readonly dragListener: DragListener;
+  public readonly keyboardDragListener: KeyboardDragListener;
+
   public constructor( numberProperty: TProperty<number>,
                       rangeProperty: TReadOnlyProperty<Range>,
                       nodeEnabledProperty: TReadOnlyProperty<boolean>,
@@ -675,18 +679,15 @@
     const pumpingDistanceRequiredToAddParticle =
       ( maxHandleYOffset - minHandleYOffset ) / options.numberOfParticlesPerPumpAction - 0.01;
 
-    options.drag = ( event: PressListenerEvent ) => {
+    const handleDrag = ( newHandlePosition: number ) => {
 
-      // update the handle and shaft position based on the user's pointer position
-      const dragPositionY = pumpHandleNode.globalToParentPoint( event.pointer.point ).y;
-      const handlePosition = Utils.clamp( dragPositionY, minHandleYOffset, maxHandleYOffset );
-      pumpHandleNode.centerY = handlePosition;
+      pumpHandleNode.centerY = newHandlePosition;
       pumpShaftNode.top = pumpHandleNode.bottom;
 
       let numberOfBatchParticles = 0; // number of particles to add all at once
 
       if ( this.lastHandlePosition !== null ) {
-        const travelDistance = handlePosition - this.lastHandlePosition;
+        const travelDistance = newHandlePosition - this.lastHandlePosition;
         if ( travelDistance > 0 ) {
 
           // This motion is in the downward direction, so add its distance to the pumping distance.
@@ -719,14 +720,44 @@
         assert && assert( numberOfBatchParticles === 0, 'unexpected batched particles' );
       }
 
-      this.lastHandlePosition = handlePosition;
+      this.lastHandlePosition = newHandlePosition;
     };
 
-    super( options );
+    // Handle the drag event when using mouse/touch - find the next handle position based on the position of the Pointer
+    const dragListenerDrag = ( event: PressListenerEvent ) => {
+
+      // update the handle and shaft position based on the user's pointer position
+      const dragPositionY = pumpHandleNode.globalToParentPoint( event.pointer.point ).y;
+      const handlePosition = Utils.clamp( dragPositionY, minHandleYOffset, maxHandleYOffset );
+      handleDrag( handlePosition );
+    };
+    options.drag = dragListenerDrag;
+    this.dragListener = new DragListener( options );
+
+    // Handle the drag event when using the keyboard - find the next handle position from the reported delta from the listener
+    const keyboardListenerDrag = ( vectorDelta: Vector2 ) => {
+      const handlePosition = Utils.clamp( pumpHandleNode.centerY + vectorDelta.y, minHandleYOffset, maxHandleYOffset );
+      handleDrag( handlePosition );
+    };
+
+    const viewRange = maxHandleYOffset - minHandleYOffset;
+    this.keyboardDragListener = new KeyboardDragListener( {
+      keyboardDragDirection: 'upDown',
+      moveOnHoldDelay: 750,
+      moveOnHoldInterval: 200,
+      dragDelta: viewRange / 10,
+      shiftDragDelta: viewRange / 20,
+      drag: keyboardListenerDrag
+    } );
 
     this.lastHandlePosition = null;
   }
 
+  public dispose(): void {
+    this.dragListener.dispose();
+    this.keyboardDragListener.dispose();
+  }
+
   public reset(): void {
     this.lastHandlePosition = null;
   }

https://phet-dev.colorado.edu/html/jg-tests/bicycle-pump-demo.html?screens=2&component=BicyclePumpNode

[jessegreenberg]

Notes from discussion with @arouinfar @terracoda @pixelzoom :

1. Using KeyboardDragListener instead of AccessibleSlider makes sense for this component.
2. It should use RichKeyboardDragListener and RichDragListener to get default sounds.
3. No specific keyboard help content necessary, sim specific content using 'handle' terminology will be used in gas-properties.

[jessegreenberg]

Also during the meeting @pixelzoom offered to take a look at the patch from #848 (comment). The patch proposes one way to factor out the drag code for DragListener and KeyboardDragListener. Assigning, and thanks!

[terracoda]

We also discussed the Grab/Drag sounds.

There was some discussion about when the the grab/release sounds play for ALT input. For mouse they play on mouse down and mouse-up. @jessegreenberg, could you point me to the general issue about the sounds.

[pixelzoom]

@terracoda Default grab/release sounds are being added to drag listeners in #849. @zepumph and @AgustinVallejo are the leads.

[pixelzoom]

I reviewed the patch in #848 (comment). InputListenerContainer doesn't seem like an unreasonable way to factor out the code shared by the DragListener and the KeyboardDragListener. But I wonder if a class is really necessary. And it's going to make PhET-iO instrumentation of those listeners a bit more complicated -- we will need to instrument both listeners, and PumpHandleDragListenerOptions isn't going to do the job. It will also complicate future features, like providing sim-specific sounds for dragging.

Stand by while I experiment with another approach.

[pixelzoom]

A more common pattern for factoring out shared code is the Delegate pattern, which uses composition instead of inheritance. In 1b677a7, I've used the Delegate pattern to factor out code that is shared by both drag listeners -- see class DragDelegate. @jessegreenberg I hope you don't mind that I took the liberty of committing and pushing this. I felt pretty confident about it. And it involved a minor API change that affected a couple of sims - see phetsims/gas-properties@011850e and phetsims/states-of-matter@3c207b6.

Also in 1b677a7, I addressed default UI sound by using RichDragListener and RichKeyboardDragListener. And I addressed PhET-iO instrumentation of both RichDragListener and RichKeyboardDragListener.

@jessegreenberg please review. Let me know if you see any problems, have concerns/questions, etc.

Btw... The patch in #848 (comment) was very helpful, and was easily adapted to the Delegate pattern. So thanks for that @jessegreenberg!

[pixelzoom]

Reminder that we still have an issue with the pump handle's focus highlight:

    //TODO https://github.com/phetsims/scenery-phet/issues/848 scale causes the focus highlight to also be scaled.
    this.pumpHandleNode.scale( pumpHandleHeight / this.pumpHandleNode.height );

[pixelzoom]

Now that I've been using BicyclePumpNode with the keyboard for a few days, I have to say that it feels kind of jerky. @jessegreenberg did I recall you saying that there was an option to make it move smoothly, instead of incrementally? How would I test-drive that?

[jessegreenberg]

Yes, dragSpeed/shiftDragSpeed instead of dragDelta/shiftDragDelta.

[pixelzoom]

Thanks @jessegreenberg. I added this TODO in BicyclePumpNode.ts:

      //TODO https://github.com/phetsims/scenery-phet/issues/848 This feels jerky. Would dragSpeed/shiftDragSpeed be better?
      dragDelta: viewRange / 10,
      shiftDragDelta: viewRange / 20,

[pixelzoom]

I reviewed the behavior with @arouinfar, and she agreed that using dragSpeed and shiftDragSpeed feels a lot better - "we want it to feel like a phyiscal pump". So in f8b3429, I made this change to the KeyboardDragListener's options, with values that were tuned with @arouinfar:

-      moveOnHoldDelay: 750,
-      moveOnHoldInterval: 200,
-      dragDelta: viewRange / 10,
-      shiftDragDelta: viewRange / 20,
+      dragSpeed: 200,
+      shiftDragSpeed: 50,

@jessegreenberg Let me know if you see any problems with this.

[pixelzoom]

In Slack#dev-public, @jessegreenberg said:

There has been a change to the way focus highlight line widths are set. Let me know something seems wrong in that area.

I confirmed that the focus highlight looks good for the pump handle. So I removed the TODO in ae9ceb6.

So... Next step for this issue is to complete the review requested in #848 (comment).

[jessegreenberg]

The delegate pattern looks great, thanks! Commits look good. And I have no problems with using dragSpeed for this. I tried it out in states-of-matter, and it works nicely. I think we are done with this one, closing.