[GrabDragInteraction] Default behavior for showDragCueNode

[jessegreenberg]

In https://github.com/phetsims/balloons-and-static-electricity/issues/534, it was requested that the drag cue node be hidden after a successful keyboard drag. This seems like a reasonable default, but the default behavior for showDragCueNode is to always be shown.

In https://github.com/phetsims/balloons-and-static-electricity/issues/534 I hade to implement create a variable that represents if there was a successful keyboard drag and then use it with option showDragCueNode. If this is the default behavior it would be great to support in GrabDragInteraction, is there a way we can do this? @zepumph what do you think of this?

```patch Index: js/accessibility/GrabDragInteraction.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/accessibility/GrabDragInteraction.js (revision 388ceca27a430642b2fa9f709e2afe676092be88) +++ js/accessibility/GrabDragInteraction.js (date 1631651335798) @@ -160,7 +160,7 @@ // if a options.dragCueNode is specified. This will only be shown if draggable node has focus // from alternative input showDragCueNode: () => { - return true; + return this.numberOfKeyboardDrags < 1; }, // {Tandem} - For instrumenting @@ -300,6 +300,10 @@ // specifically to provide hints specific to alternative input this.numberOfKeyboardGrabs = 0; + // @private {number} - the number of times this component has been dragged with a keyboard specifically to + // specific to keyboard interaction + this.numberOfKeyboardDrags = 0; + // @private {string|null} // set the help text, if provided - it will be associated with aria-describedby when in the "grabbable" state this.node.descriptionContent = this.supportsGestureDescription ? options.gestureHelpText : options.keyboardHelpText; @@ -530,9 +534,13 @@ } ); this.node.addInputListener( this.pressListener ); - // upon successful drag with the KeyboardDragListener, keep this Node in view const dragListener = () => { + + // upon successful drag with the KeyboardDragListener, keep this Node in view animatedPanZoomSingleton.listener.keepNodeInView( this.node ); + + // there must have been a successful keyboard drag + this.numberOfKeyboardDrags++; }; keyboardDragListener.dragEmitter.addListener( dragListener ); @@ -768,6 +776,7 @@ // turnToGrabbable will increment this, so reset it again this.numberOfGrabs = 0; this.numberOfKeyboardGrabs = 0; + this.numberOfKeyboardDrags = 0; this.grabCueNode.visible = true; if ( this.dragCueNode ) { this.dragCueNode.visible = true; ```

[zepumph]

That sounds totally fine, and like the way we should proceed. I would recommend making numberOfKeyboardDrags public and read-only, because we may want to use it in the future to show the drag cue for the first X drags instead of just until 1 success.

I also want to note again that I am not really much of a fan of having keyboardDragListener be a required argument. It feels much more valuable to support a grabDragInteraction that doesn't make assumptions on how the interaction is handled. I remember noting that opinion when I first discovered the refactor to support pan/zoom. This is a step towards making it even harder to get rid of that dependency, which feels less maintainable me. It is only a feeling, and so it is hard to weigh the potential of having a different sort of drag listener type in the future vs. the effort to keep things flexible now.

I assume that we will continue with your patch. An alternative would be to support a structured interface where we iterate through all listenersForDragState, and any instance with a dragEmitter, on it, we add this listener to. Then we wouldn't need to depend on the type KeyboardDragListener, requiring one be provided.

What do you think about this patch that is untested, and will likely need documentation updates?

```diff Index: js/accessibility/GrabDragInteraction.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/accessibility/GrabDragInteraction.js b/js/accessibility/GrabDragInteraction.js --- a/js/accessibility/GrabDragInteraction.js (revision 032f9abe48127cb698e7a30dd55c813e3725a5fb) +++ b/js/accessibility/GrabDragInteraction.js (date 1631652657712) @@ -46,6 +46,7 @@ * @author Michael Kauzmann (PhET Interactive Simulations) */ +import Emitter from '../../../axon/js/Emitter.js'; import assertHasProperties from '../../../phet-core/js/assertHasProperties.js'; import getGlobal from '../../../phet-core/js/getGlobal.js'; import merge from '../../../phet-core/js/merge.js'; @@ -78,10 +79,9 @@ /** * @param {Node} node - will be mutated with a11y options to have the grab/drag functionality in the PDOM - * @param {KeyboardDragListener} keyboardDragListener - added to the Node when it is draggable * @param {Object} [options] */ - constructor( node, keyboardDragListener, options ) { + constructor( node, options ) { options = merge( { // A string that is filled in to the appropriate button label @@ -205,7 +205,6 @@ assert && assert( options.grabCueOptions instanceof Object ); assert && assert( options.grabCueOptions.visible === undefined, 'Should not set visibility of the cue node' ); assert && assert( options.draggableOptions instanceof Object ); - assert && assert( !options.listenersForDragState.includes( keyboardDragListener ), 'GrabDragInteraction adds the KeyboardDragListener to listenersForDragState' ); if ( options.dragCueNode !== null ) { assert && assert( options.dragCueNode instanceof Node ); assert && assert( !options.dragCueNode.parent, 'GrabDragInteraction adds dragCueNode to focusHighlight' ); @@ -501,7 +500,7 @@ }; // @private - this.listenersForDragState = options.listenersForDragState.concat( [ dragDivListener, keyboardDragListener ] ); + this.listenersForDragState = options.listenersForDragState.concat( [ dragDivListener ] ); // @private - from non-PDOM pointer events, change representations in the PDOM - necessary for accessible tech that // uses pointer events like iOS VoiceOver. The above listeners manage input from the PDOM. @@ -530,11 +529,19 @@ } ); this.node.addInputListener( this.pressListener ); - // upon successful drag with the KeyboardDragListener, keep this Node in view + // @private + this.dragListenersWithThisListenerOnIt = []; + + // upon successful drag with the a drag listener, keep this Node in view const dragListener = () => { animatedPanZoomSingleton.listener.keepNodeInView( this.node ); }; - keyboardDragListener.dragEmitter.addListener( dragListener ); + options.listenersForDragState.forEach( listener => { + if ( listener.dragEmitter && listener.dragEmitter instanceof Emitter ) { + listener.dragEmitter.addListener( dragListener ); + this.dragListenersWithThisListenerOnIt.push( listener ); + } + } ); // Initialize the Node as a grabbable (button) to begin with this.turnToGrabbable(); @@ -543,7 +550,7 @@ this.disposeGrabDragInteraction = () => { this.node.removeInputListener( this.pressListener ); - keyboardDragListener.dragEmitter.removeListener( dragListener ); + this.dragListenersWithThisListenerOnIt.forEach( listener => listener.dragEmitter.removeListener( dragListener ) ); // Remove listeners according to what state we are in if ( this.grabbable ) {

[pixelzoom]

Reviewing scenery-phet issues for Q4 planning...

No progress in over year. Assigning to @kathy-phet to prioritize.

[zepumph]

Subset of https://github.com/phetsims/scenery/issues/1298