PDOM listeners should respect Node.pickable

[zepumph]

While working on phetsims/ratio-and-proportion#74, @jessegreenberg and I realized that Node.pickable isn't listened to at all for PDOM listeners. We have only ever had a manual approach that ignores code within the listener. This is different from how other listeners handle input, using pickable as a killswitch of sorts to turn off input. Let's try to add this!

[zepumph]

@jessegreenberg and I worked on this today. We decided that the best way to handle this was in Input.js. It was a pretty simple line of code, but I guess we had never thought to handle this in a scenery-centric way. Instead it was always based on respecting the enabledProperty value in listeners. We also added support for setting aria-disabled in a few spots, including EnabledNode.

Two more things here:

• Look at a few more listeners, perhaps in Accessible Traits, and see if enabledProperty logic can be removed.
• Have @jonathanolson spot check 88cf880. It would be good to make sure that, althought the PDOM approach is different code, that it seems correct, and aligned with how pickability is handled for other Pointers (like in branchChangeEvents.

These can both be done immediately.

[jonathanolson]

Commit looks good to me.

[zepumph]

I fixed an issue that pre-commit unit tests were failing on because Puppeteer was having trouble serializing the PDOMPeer I was using as an argument to assert.ok. I want to bring this up to developer meeting as a PSA:

Unit tests will appear passed in the browser qunit harness, but will fail when run as part of pre-commit hooks in git in a certain case. They will fail if arguments to an assert call (like ok or equal) cannot be serialized over an jsonify or structured-cloning-like algorithm. I'm not totally sure what the exact algorithm is, but it failed when trying to send a PDOMPeer instance over from pupeteer back to the Nodejs process, whereas it didn't when sending a boolean.

For future reference, this works:
assert.ok( !!a11yNode.accessibleInstances[ 0 ].peer, 'should have a peer' );

This does not:
assert.ok( a11yNode.accessibleInstances[ 0 ].peer, 'should have a peer' );

This seems worth a PSA because it is sneaker, and only happens on the pre-commit hook. I'm also open to suggestions if we want to try to make hook-pre-commit.js more robust, though I don't feel like we need to.

[samreid]

In today's discussion, we discovered that removing the logging eliminates the circular conversion. @zepumph volunteered to make this improvement, thanks!

[zepumph]

I was able to keep most of the same logging, swapping out "expected/actual" for a source stack trace. By passing through component pieces, we work around the inability to jsonify potential expected/actual args. @samreid please review and feel free to close.

[samreid]

That's a great idea, thanks! Closing.

[jessegreenberg]

The original commit in 88cf880 is actually causing phetsims/molecules-and-light#358. RadioButtonGroup sets pickable to false on the selected radio button, preventing all keyboard input on it. I would like to add keydown and keyup to the PDOM_UNPICKABLE_EVENTS list. That leaves input, change, and click events (logical interactions/changes) to be blocked if the Node's Trail is not pickable.

@zepumph does this seem OK to you?

[zepumph]

If we go with the above recommendation. I feel like we are tying the implmentation too closely with the example usages we have in the wild. It is just as likely in the future that we come across a case in the future where we want to block keydown, and potentially allow input (maybe?). Maybe it would be good to take a step back and see if there is an issue with the above commit, or with the way we implement alt input in RadioButtonGroup.

[jessegreenberg]

Yea, I think that is very fair. We have identified that we want to receive many PDOM events for on non-pickable things, so I am leaning toward reverting changes for this issue. Adding label again to discuss tomorrow.

[zepumph]

@jessegreenberg can you please note where we are in the progress with this issue and phetsims/molecules-and-light#358, I remember working with you last week on these, and I remember that it, in turn, fixed phetsims/ratio-and-proportion#88 as well, but I don't see any commits in these issues. I should have written up a summary of last week's discussion/work so that I didn't forget like a dingus!

[jessegreenberg]

No, I appologize - I have the changes for this still in a diff but have not had time to complete it - here it is

diff --git a/js/input/Input.js b/js/input/Input.js
index 4229a824..64314d2d 100644
--- a/js/input/Input.js
+++ b/js/input/Input.js
@@ -1027,11 +1027,17 @@ class Input {
   dispatchA11yEvent( trail, eventType, domEvent, bubbles ) {
     Display.userGestureEmitter.emit();
 
-    // Even unpickable (disabled) components should stay in the focus order.
-    const canFireListeners = trail.isPickable() || PDOM_UNPICKABLE_EVENTS.includes( eventType );
-
     // This workaround hopefully won't be here forever, see ParallelDOM.setExcludeLabelSiblingFromInput() and https://github.com/phetsims/a11y-research/issues/156
-    if ( !( domEvent.target && domEvent.target.hasAttribute( PDOMUtils.DATA_EXCLUDE_FROM_INPUT ) ) && canFireListeners ) {
+    if ( !( domEvent.target && domEvent.target.hasAttribute( PDOMUtils.DATA_EXCLUDE_FROM_INPUT ) ) ) {
+
+      // Even unpickable (disabled) components should stay in the focus order.
+      const canFireListeners = trail.isPickable() || PDOM_UNPICKABLE_EVENTS.includes( eventType );
+
+      // if we cannot fire listeners, pass an empty trail so we do not dispatch to targets along the trail, but we
+      // still dispatch to pointer and Display listeners
+      if ( !canFireListeners ) {
+        trail = new Trail( [] );
+      }
 
       // To support PhET-iO playback, any potential playback events downstream of this playback event must be marked as
       // non playback events. This is to prevent the PhET-iO playback engine from repeating those events. This is here
diff --git a/js/listeners/AnimatedPanZoomListener.js b/js/listeners/AnimatedPanZoomListener.js
index c48a2ad3..219a9cb3 100644
--- a/js/listeners/AnimatedPanZoomListener.js
+++ b/js/listeners/AnimatedPanZoomListener.js
@@ -21,6 +21,7 @@ import PDOMUtils from '../accessibility/pdom/PDOMUtils.js';
 import Display from '../display/Display.js';
 import Pointer from '../input/Pointer.js';
 import scenery from '../scenery.js';
+import MultiListener from './MultiListener.js';
 import PanZoomListener from './PanZoomListener.js';
 
 // constants
@@ -158,26 +159,42 @@ class AnimatedPanZoomListener extends PanZoomListener {
     this.animateToTargets( dt );
   }
 
-  /**
-   * Attach a MiddlePress for drag panning, if detected.
-   * @public
-   * @override
-   *
-   * @param {SceneryEvent} event
-   */
-  down( event ) {
-    PanZoomListener.prototype.down.call( this, event );
-
-    this._downTrail = event.trail;
-    this._downInDragBounds = this._dragBounds.containsPoint( event.pointer.point );
-
-    // begin middle press panning if we aren't already in that state
-    if ( event.pointer.type === 'mouse' && event.pointer.middleDown && !this.middlePress ) {
-      this.middlePress = new MiddlePress( event.pointer, event.trail );
-      event.pointer.cursor = MOVE_CURSOR;
-    }
-    else {
-      this.cancelMiddlePress();
+  // /**
+  //  * Attach a MiddlePress for drag panning, if detected.
+  //  * @public
+  //  * @override
+  //  *
+  //  * @param {SceneryEvent} event
+  //  */
+  // down( event ) {
+  //   PanZoomListener.prototype.down.call( this, event );
+  //
+  //   this._downTrail = event.trail;
+  //   this._downInDragBounds = this._dragBounds.containsPoint( event.pointer.point );
+  //
+  //   // begin middle press panning if we aren't already in that state
+  //   if ( event.pointer.type === 'mouse' && event.pointer.middleDown && !this.middlePress ) {
+  //     this.middlePress = new MiddlePress( event.pointer, event.trail );
+  //     event.pointer.cursor = MOVE_CURSOR;
+  //   }
+  //   else {
+  //     this.cancelMiddlePress();
+  //   }
+  // }
+
+  createPress( event ) {
+    const createdSuccessfully = PanZoomListener.prototype.createPress.call( this, event );
+
+    if ( createdSuccessfully ) {
+
+      // begin middle press panning if we aren't already in that state
+      if ( event.pointer.type === 'mouse' && event.pointer.middleDown && !this.middlePress ) {
+        this.middlePress = new MiddlePress( event.pointer, event.trail );
+        event.pointer.cursor = MOVE_CURSOR;
+      }
+      else {
+        this.cancelMiddlePress();
+      }
     }
   }
 
@@ -216,23 +233,25 @@ class AnimatedPanZoomListener extends PanZoomListener {
    * @param {SceneryEvent} event
    */
   move( event ) {
-    if ( this._downTrail && this._downInDragBounds ) {
-      this._repositionDuringDragPoint = null;
+    if ( this._downTrail ) {
+      if ( this._downInDragBounds ) {
+        this._repositionDuringDragPoint = null;
 
-      const hasDragIntent = this.hasDragIntent( event.pointer );
-      const currentTargetExists = event.currentTarget !== null;
+        const hasDragIntent = this.hasDragIntent( event.pointer );
+        const currentTargetExists = event.trail.lastNode() !== null;
 
-      if ( currentTargetExists && hasDragIntent ) {
+        if ( currentTargetExists && hasDragIntent ) {
 
-        const globalTargetBounds = this._downTrail.parentToGlobalBounds( this._downTrail.lastNode().bounds );
-        const targetInBounds = this._panBounds.containsBounds( globalTargetBounds );
-        if ( !targetInBounds ) {
-          this._repositionDuringDragPoint = event.pointer.point;
+          const globalTargetBounds = this._downTrail.parentToGlobalBounds( this._downTrail.lastNode().bounds );
+          const targetInBounds = this._panBounds.containsBounds( globalTargetBounds );
+          if ( !targetInBounds ) {
+            this._repositionDuringDragPoint = event.pointer.point;
+          }
         }
       }
-    }
-    else {
-      this._downInDragBounds = this._dragBounds.containsPoint( event.pointer.point );
+      else {
+        this._downInDragBounds = this._dragBounds.containsPoint( event.pointer.point );
+      }
     }
   }
 
diff --git a/js/listeners/MultiListener.js b/js/listeners/MultiListener.js
index f0b5e805..89601bae 100644
--- a/js/listeners/MultiListener.js
+++ b/js/listeners/MultiListener.js
@@ -271,25 +271,17 @@ class MultiListener {
     return !pointer.hasIntent( Pointer.Intent.DRAG );
   }
 
-  /**
-   * Part of the scenery event API.
-   * @public (scenery-internal)
-   * @param event
-   */
-  down( event ) {
-    sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'MultiListener down' );
+  createPress( event ) {
+    if ( !_.includes( event.trail.nodes, this._targetNode ) ) {
+      sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'MultiListener abort: targetNode doesnt go through event trail' );
+      return false;
+    }
 
     if ( event.pointer instanceof Mouse && event.domEvent.button !== this._mouseButton ) {
       sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'MultiListener abort: wrong mouse button' );
-
-      return;
+      return false;
     }
 
-    sceneryLog && sceneryLog.InputListener && sceneryLog.push();
-
-    assert && assert( _.includes( event.trail.nodes, this._targetNode ),
-      'MultiListener down trail does not include targetNode?' );
-
     const press = new Press( event.pointer, event.trail.subtrailTo( this._targetNode, false ) );
 
     if ( !this._allowMoveInterruption && !this._allowMultitouchInterruption ) {
@@ -309,6 +301,18 @@ class MultiListener {
       sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'MultiListener attached, adding background press' );
       this.addBackgroundPress( press );
     }
+  }
+
+  /**
+   * Part of the scenery event API.
+   * @public (scenery-internal)
+   * @param event
+   */
+  down( event ) {
+    sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'MultiListener down' );
+    sceneryLog && sceneryLog.InputListener && sceneryLog.push();
+
+    this.createPress( event );
 
     sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
   }

We had decided that PDOM events should respect pickability, but we should not add more to our PDOM_UNPICKABLE_EVENTS list. Instead, listeners that need to fire regardless of pickability along a Trail should be attached to the Display (matching behavior of mouse/touch). Part of this (reflected in the diff above) is that we need to change how we dispatch PDOM events so that we still notify listeners on the Display even when a Trail is not pickable. The other part involved changes to MultiListener so that it can be attached to a Display, it currently assumes it is added to a Node.

[zepumph]

That makes sense! Thanks for the update, I remember now. Will you please let me know if you want me to help you out in any way. Do you want to pair on this?

[jessegreenberg]

I worked on this again today - the big challenge that I encountered in addition to #1056 (comment) was that the reserveForDrag functions on the Pointer assumed that they be called during dispatch along scene graph Trails. And so that method to prevent behavior on global listeners (like MultiListener) will not work if the listener is added to the Display and if the Node is not pickable, so we would need another way.

I explored an alternative today that worked by flagging a Node with intent of behavior rather than in the input listener:

• On Input, determine the Trail for the Pointer from a hit test that ignores pickability.
• Search Nodes along this Trail for a Node that is flagged with reserveForDrag (or other), if we find one call Pointer.reserveForDrag().
• Then dispatch to Pointer, Trail, Display listeners, along the Trail that considers pickability (as usual).

Pros:

• This method will allow global listeners on Nodes that are not pickable, as long as the listener is added to the Display.
• This method also works for cases where SceneryEvent.abort() and SceneryEvent.handle() are used, something I hadn't considered before (as long as the listener is added to the Display).
• This patch offers a potential fix for the issue where calling reserveForDrag() wouldn't work when focus changed in the listener callback because intent is reserved for entire dispatch.

Cons:

• More performance intensive - basically hit testing twice as often (once for pickability, once without). Maybe there is a better way to do this in Picker though.
• reserveForDrag and others like it cannot be factored into reusable listeners. They must be used on each Node where necessary. This work feels to me like it should be done in event listeners during dispatch. Maybe that is just because I am used to event.preventDefault() being done in listeners and not in specified on DOMElements.

Here is the patch (verrrry hacky, only lightly tested, but quick way to get hit testing without caring about pickability):

Index: scenery/js/listeners/DragListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/listeners/DragListener.js	(revision 40eb3c92a6ae990a0142a6c4a81ae71abc5e503b)
+++ scenery/js/listeners/DragListener.js	(date 1595905470731)
@@ -283,7 +283,7 @@
 
       // signify that this listener is reserved for dragging so that other listeners can change
       // their behavior during scenery event dispatch
-      this.pointer.reserveForDrag();
+      //this.pointer.reserveForDrag();
 
       this.attachTransformTracker();
 
Index: joist/js/Sim.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/Sim.js	(revision 87ae3a422a575c4e361ab5b3e40da7d4ee85cfeb)
+++ joist/js/Sim.js	(date 1595888143094)
@@ -625,7 +625,8 @@
   this.panZoomListener = null;
   if ( this.supportsZoom ) {
     this.panZoomListener = new AnimatedPanZoomListener( this.simulationRoot );
-    this.simulationRoot.addInputListener( this.panZoomListener );
+    //this.simulationRoot.addInputListener( this.panZoomListener );
+    this.display.addInputListener( this.panZoomListener );
   }
 
   // @public (joist-internal)
Index: scenery/js/util/Picker.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/util/Picker.js	(revision 40eb3c92a6ae990a0142a6c4a81ae71abc5e503b)
+++ scenery/js/util/Picker.js	(date 1595953344846)
@@ -12,6 +12,7 @@
 import Bounds2 from '../../../dot/js/Bounds2.js';
 import Vector2 from '../../../dot/js/Vector2.js';
 import inherit from '../../../phet-core/js/inherit.js';
+import merge from '../../../phet-core/js/merge.js';
 import scenery from '../scenery.js';
 import Trail from './Trail.js';
 
@@ -19,8 +20,14 @@
  * @constructor
  *
  * @param {Node} node - Our node.
+ * @param {Object} [options] -
  */
-function Picker( node ) {
+function Picker( node, options ) {
+  options = merge( {
+
+    ignorePickability: false
+  }, options );
+
   // @private {Node} - Our node reference (does not change)
   this.node = node;
 
@@ -74,6 +81,8 @@
   this.touchInclusiveDirty = true;
   this.touchExclusiveDirty = true;
 
+  this.ignorePickability = options.ignorePickability;
+
   // @private {Vector2} - Used to minimize garbage created in the hit-testing process
   this.scratchVector = new Vector2( 0, 0 );
 }
@@ -174,7 +183,8 @@
       const child = this.node._children[ i ];
 
       sceneryLog && sceneryLog.hitTest && sceneryLog.push();
-      const childHit = child._picker.recursiveHitTest( localPoint, useMouse, useTouch, isInclusive );
+      const picker = this.ignorePickability ? child._pickerIgnoringPickability : child._picker;
+      const childHit = picker.recursiveHitTest( localPoint, useMouse, useTouch, isInclusive );
       sceneryLog && sceneryLog.hitTest && sceneryLog.pop();
 
       // If there was a hit, immediately add our node to the start of the Trail (will recursively build the Trail).
@@ -236,7 +246,8 @@
     if ( !this.selfPruned && wasNotDirty ) {
       const parents = this.node._parents;
       for ( let i = 0; i < parents.length; i++ ) {
-        parents[ i ]._picker.invalidate( andExclusive || this.selfInclusive, false );
+        const picker = this.ignorePickability ? parents[ i ]._pickerIgnoringPickability : parents[ i ]._picker;
+        picker.invalidate( andExclusive || this.selfInclusive, false );
       }
     }
   },
@@ -259,7 +270,7 @@
 
     const children = this.node._children;
     for ( let i = 0; i < children.length; i++ ) {
-      const childPicker = children[ i ]._picker;
+      const childPicker = this.ignorePickability ? children[ i ]._pickerIgnoringPickability : children[ i ]._picker;
 
       // Since we are "inclusive", we don't care about subtreePrunable (we won't prune for that). Only check
       // if pruning is force (selfPruned).
@@ -297,7 +308,7 @@
 
     const children = this.node._children;
     for ( let i = 0; i < children.length; i++ ) {
-      const childPicker = children[ i ]._picker;
+      const childPicker = this.ignorePickability ? children[ i ]._pickerIgnoringPickability : children[ i ]._picker;
 
       // Since we are not "inclusive", we will prune the search if subtreePrunable is true.
       if ( !childPicker.subtreePrunable ) {
@@ -339,7 +350,7 @@
 
     const children = this.node._children;
     for ( let i = 0; i < children.length; i++ ) {
-      const childPicker = children[ i ]._picker;
+      const childPicker = this.ignorePickability ? children[ i ]._pickerIgnoringPickability : children[ i ]._picker;
 
       // Since we are "inclusive", we don't care about subtreePrunable (we won't prune for that). Only check
       // if pruning is force (selfPruned).
@@ -377,7 +388,7 @@
 
     const children = this.node._children;
     for ( let i = 0; i < children.length; i++ ) {
-      const childPicker = children[ i ]._picker;
+      const childPicker = this.ignorePickability ? children[ i ]._pickerIgnoringPickability : children[ i ]._picker;
 
       // Since we are not "inclusive", we will prune the search if subtreePrunable is true.
       if ( !childPicker.subtreePrunable ) {
@@ -443,8 +454,10 @@
    */
   onInsertChild: function( childNode ) {
     // If the child is selfPruned, we don't have to update any metadata.
-    if ( !childNode._picker.selfPruned ) {
-      const hasPickable = childNode._picker.subtreePickableCount > 0;
+    const childPicker = this.ignorePickability ? childNode._pickerIgnoringPickability : childNode._picker;
+
+    if ( !childPicker.selfPruned ) {
+      const hasPickable = childPicker.subtreePickableCount > 0;
 
       // If it has a non-zero subtreePickableCount, we'll need to increment our own count by 1.
       if ( hasPickable ) {
@@ -468,8 +481,10 @@
    */
   onRemoveChild: function( childNode ) {
     // If the child is selfPruned, we don't have to update any metadata.
-    if ( !childNode._picker.selfPruned ) {
-      const hasPickable = childNode._picker.subtreePickableCount > 0;
+    const childPicker = this._pickerIgnoringPickability ? childNode._pickerIgnoringPickability : childNode._picker;
+
+    if ( !childPicker.selfPruned ) {
+      const hasPickable = childPicker.subtreePickableCount > 0;
 
       // If it has a non-zero subtreePickableCount, we'll need to decrement our own count by 1.
       if ( hasPickable ) {
@@ -600,14 +615,16 @@
    * invalidate ourself.
    */
   checkSelfPruned: function() {
-    const selfPruned = this.node.pickableProperty.value === false || !this.node.isVisible();
+    const pickableFalse = this.ignorePickability ? false : this.node.pickableProperty.value === false;
+    const selfPruned = pickableFalse || !this.node.isVisible();
+
     if ( this.selfPruned !== selfPruned ) {
       this.selfPruned = selfPruned;
 
       // Notify parents
       const parents = this.node._parents;
       for ( let i = 0; i < parents.length; i++ ) {
-        const picker = parents[ i ]._picker;
+        const picker = this.ignorePickability ?  parents[ i ]._pickerIgnoringPickability : parents[ i ]._picker;
 
         // If we have an input listener/pickable:true in our subtree, we'll need to invalidate exclusive bounds also,
         // and we'll want to update the pickable count of our parent.
@@ -629,7 +646,8 @@
    * @private
    */
   checkSelfInclusive: function() {
-    const selfInclusive = this.node.pickableProperty.value === true || this.node._inputListeners.length > 0;
+    const pickableTrue = this.ignorePickability ? true : this.node.pickableProperty.value === true;
+    const selfInclusive = pickableTrue || this.node._inputListeners.length > 0;
     if ( this.selfInclusive !== selfInclusive ) {
       this.selfInclusive = selfInclusive;
 
@@ -643,9 +661,12 @@
    * @private
    */
   checkSubtreePrunable: function() {
-    const subtreePrunable = this.node.pickableProperty.value === false ||
+    const pickableFalse = this.ignorePickability ? false : this.node.pickableProperty.value === false;
+    const pickableNotTrue = this.ignorePickability ? false : this.node.pickableProperty.value !== true;
+
+    const subtreePrunable = pickableFalse ||
                             !this.node.isVisible() ||
-                            ( this.node.pickableProperty.value !== true && this.subtreePickableCount === 0 );
+                            ( pickableNotTrue && this.subtreePickableCount === 0 );
 
     if ( this.subtreePrunable !== subtreePrunable ) {
       this.subtreePrunable = subtreePrunable;
@@ -680,7 +701,8 @@
       // Update our parents if our count changed (AND if it matters, i.e. we aren't selfPruned).
       const len = this.node._parents.length;
       for ( let i = 0; i < len; i++ ) {
-        this.node._parents[ i ]._picker.changePickableCount( wasZero ? 1 : -1 );
+        const picker = this.ignorePickability ? this.node._parents[ i ]._pickerIgnoringPickability : this.node._parents[ i ]._picker;
+        picker.changePickableCount( wasZero ? 1 : -1 );
       }
     }
   },
Index: scenery/js/input/Input.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/input/Input.js	(revision 40eb3c92a6ae990a0142a6c4a81ae71abc5e503b)
+++ scenery/js/input/Input.js	(date 1595954965408)
@@ -691,8 +691,20 @@
         sceneryLog && sceneryLog.Input && sceneryLog.push();
 
         const trail = this.updateTrailForPDOMDispatch( event );
+
+        // the trail determined above already does not care about pickability
+        const trailIgnoringPickability = trail;
+        const trailHasNodeReservingKeyboard = _.some( trailIgnoringPickability.nodes, node => {
+          return node.reserveForKeyboardDrag;
+        } );
+        if ( trailHasNodeReservingKeyboard ) {
+          this.a11yPointer.addIntent( Pointer.Intent.KEYBOARD_DRAG );
+        }
+
         this.dispatchA11yEvent( trail, 'keydown', event, true );
 
+        this.a11yPointer.removeIntent( Pointer.Intent.KEYBOARD_DRAG );
+
         sceneryLog && sceneryLog.Input && sceneryLog.pop();
       }, {
         phetioPlayback: true,
@@ -1027,11 +1039,16 @@
   dispatchA11yEvent( trail, eventType, domEvent, bubbles ) {
     Display.userGestureEmitter.emit();
 
-    // Even unpickable (disabled) components should stay in the focus order.
-    const canFireListeners = trail.isPickable() || PDOM_UNPICKABLE_EVENTS.includes( eventType );
-
     // This workaround hopefully won't be here forever, see ParallelDOM.setExcludeLabelSiblingFromInput() and https://github.com/phetsims/a11y-research/issues/156
-    if ( !( domEvent.target && domEvent.target.hasAttribute( PDOMUtils.DATA_EXCLUDE_FROM_INPUT ) ) && canFireListeners ) {
+    if ( !( domEvent.target && domEvent.target.hasAttribute( PDOMUtils.DATA_EXCLUDE_FROM_INPUT ) ) ) {
+
+      // if Trail is not pickable, dispatch with an empty trail - this way listeners are
+      // still called on the Display and to the Poiner, but not along any scene graph
+      // Nodes if Trail is not pickable
+      const canFireListeners = trail.isPickable() || PDOM_UNPICKABLE_EVENTS.includes( eventType );
+      if ( !canFireListeners ) {
+        trail = new Trail( [] );
+      }
 
       // To support PhET-iO playback, any potential playback events downstream of this playback event must be marked as
       // non playback events. This is to prevent the PhET-iO playback engine from repeating those events. This is here
@@ -1531,10 +1548,11 @@
    * @private
    *
    * @param {Pointer}
+   * @param {Boolean} - whether or not Node.pickable should be disregarding in hit testing
    * @returns {Trail}
    */
-  getPointerTrail( pointer ) {
-    return this.rootNode.trailUnderPointer( pointer ) || new Trail( this.rootNode );
+  getPointerTrail( pointer, ignorePickability = false ) {
+    return this.rootNode.trailUnderPointer( pointer, ignorePickability ) || new Trail( this.rootNode );
   }
 
   /**
@@ -1624,6 +1642,16 @@
       }
     }
 
+    if ( pointer.point ) {
+      const trailIgnoringPickability = this.getPointerTrail( pointer, true );
+      const trailHasNodeReservingKeyboard = _.some( trailIgnoringPickability.nodes, node => {
+        return node.reserveForDrag;
+      } );
+      if ( trailHasNodeReservingKeyboard ) {
+        pointer.reserveForDrag();
+      }
+    }
+
     // dispatch after handling display focus in case immediate focusout interferes
     this.dispatchEvent( eventTrail, 'down', pointer, event, true );
 
Index: ratio-and-proportion/js/common/view/RatioHandNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- ratio-and-proportion/js/common/view/RatioHandNode.js	(revision 6fbd62b67bd9fa6a36382025bb0b308c6d922b53)
+++ ratio-and-proportion/js/common/view/RatioHandNode.js	(date 1595905264534)
@@ -51,6 +51,8 @@
       fill: 'white'
     } );
 
+    this.reserveForDrag = true;
+
     // empirical multipliers to center hand on palm. Don't change these without altering the layout for the cue arrows too.
     handImage.right = handImage.width * .4;
     handImage.bottom = handImage.height * .475;
Index: scenery/js/nodes/Node.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/nodes/Node.js	(revision 40eb3c92a6ae990a0142a6c4a81ae71abc5e503b)
+++ scenery/js/nodes/Node.js	(date 1595904041203)
@@ -543,6 +543,7 @@
 
   // Initialize sub-components
   this._picker = new Picker( this );
+  this._pickerIgnoringPickability = new Picker( this, { ignorePickability: true } );
 
   // @public (scenery-internal) {boolean} - There are certain specific cases (in this case due to a11y) where we need
   // to know that a Node is getting removed from its parent BUT that process has not completed yet. It would be ideal
@@ -618,6 +619,7 @@
 
     // needs to be early to prevent re-entrant children modifications
     this._picker.onInsertChild( node );
+    this._pickerIgnoringPickability.onInsertChild( node );
     this.changeBoundsEventCount( node._boundsEventCount > 0 ? 1 : 0 );
     this._rendererSummary.summaryChange( RendererSummary.bitmaskAll, node._rendererSummary.bitmask );
 
@@ -727,6 +729,7 @@
 
     // needs to be early to prevent re-entrant children modifications
     this._picker.onRemoveChild( node );
+    this._pickerIgnoringPickability.onRemoveChild( node );
     this.changeBoundsEventCount( node._boundsEventCount > 0 ? -1 : 0 );
     this._rendererSummary.summaryChange( node._rendererSummary.bitmask, RendererSummary.bitmaskAll );
 
@@ -1460,6 +1463,7 @@
       this._selfBoundsDirty = true;
       this.invalidateBounds();
       this._picker.onSelfBoundsDirty();
+      this._pickerIgnoringPickability.onSelfBoundsDirty();
     }
     // Otherwise, set the self bounds directly
     else {
@@ -1475,6 +1479,7 @@
         // set repaint flags
         this.invalidateBounds();
         this._picker.onSelfBoundsDirty();
+        this._pickerIgnoringPickability.onSelfBoundsDirty();
 
         // record the new bounds
         ourSelfBounds.set( newSelfBounds );
@@ -1813,14 +1818,22 @@
    * @param {boolean} [isTouch] - Whether touchAreas should be used.
    * @returns {Trail|null} - Returns null if the point is not contained in the subtree.
    */
-  hitTest: function( point, isMouse, isTouch ) {
+  hitTest: function( point, isMouse, isTouch, ignorePickability ) {
     assert && assert( point instanceof Vector2 && point.isFinite(), 'The point should be a finite Vector2' );
     assert && assert( isMouse === undefined || typeof isMouse === 'boolean',
       'If isMouse is provided, it should be a boolean' );
     assert && assert( isTouch === undefined || typeof isTouch === 'boolean',
       'If isTouch is provided, it should be a boolean' );
 
-    return this._picker.hitTest( point, !!isMouse, !!isTouch );
+    ignorePickability = ignorePickability || false;
+
+    if ( ignorePickability ) {
+      return this._pickerIgnoringPickability.hitTest( point, !!isMouse, !!isTouch );
+    }
+    else {
+      return this._picker.hitTest( point, !!isMouse, !!isTouch );
+    }
+
   },
 
   /**
@@ -1832,8 +1845,9 @@
    * @param {Pointer} pointer
    * @returns {Trail|null}
    */
-  trailUnderPointer: function( pointer ) {
-    return this.hitTest( pointer.point, pointer instanceof Mouse, pointer instanceof Touch || pointer instanceof Pen );
+  trailUnderPointer: function( pointer, ignorePickability ) {
+    ignorePickability = ignorePickability || false;
+    return this.hitTest( pointer.point, pointer instanceof Mouse, pointer instanceof Touch || pointer instanceof Pen, ignorePickability );
   },
 
   /**
@@ -1962,6 +1976,7 @@
     if ( !_.includes( this._inputListeners, listener ) ) {
       this._inputListeners.push( listener );
       this._picker.onAddInputListener();
+      this._pickerIgnoringPickability.onAddInputListener();
       if ( assertSlow ) { this._picker.audit(); }
     }
     return this;
@@ -1982,6 +1997,7 @@
     if ( index >= 0 ) {
       this._inputListeners.splice( index, 1 );
       this._picker.onRemoveInputListener();
+      this._pickerIgnoringPickability.onRemoveInputListener();
       if ( assertSlow ) { this._picker.audit(); }
     }
 
@@ -2465,6 +2481,7 @@
     this.invalidateBounds();
 
     this._picker.onTransformChange();
+    this._pickerIgnoringPickability.onTransformChange();
     if ( assertSlow ) { this._picker.audit(); }
 
     this.transformEmitter.emit();
@@ -3138,6 +3155,7 @@
   onVisiblePropertyChange: function( visible ) {
     // changing visibility can affect pickability pruning, which affects mouse/touch bounds
     this._picker.onVisibilityChange();
+    this._pickerIgnoringPickability.onVisibilityChange();
 
     if ( assertSlow ) { this._picker.audit(); }
 
@@ -3314,6 +3332,7 @@
       this.pickableProperty.setPropertyValue( pickable );
 
       this._picker.onPickableChange( oldPickable, pickable );
+      this._pickerIgnoringPickability.onPickableChange( oldPickable, pickable );
       if ( assertSlow ) { this._picker.audit(); }
       // TODO: invalidate the cursor somehow? #150
 
@@ -3448,6 +3467,7 @@
       this._mouseArea = area; // TODO: could change what is under the mouse, invalidate!
 
       this._picker.onMouseAreaChange();
+      this._pickerIgnoringPickability.onMouseAreaChange();
       if ( assertSlow ) { this._picker.audit(); }
     }
   },
@@ -3478,6 +3498,7 @@
       this._touchArea = area; // TODO: could change what is under the touch, invalidate!
 
       this._picker.onTouchAreaChange();
+      this._pickerIgnoringPickability.onTouchAreaChange();
       if ( assertSlow ) { this._picker.audit(); }
     }
   },
@@ -3509,6 +3530,7 @@
 
       this.invalidateBounds();
       this._picker.onClipAreaChange();
+      this._pickerIgnoringPickability.onClipAreaChange();
 
       if ( assertSlow ) { this._picker.audit(); }
     }
Index: scenery/js/listeners/AnimatedPanZoomListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/listeners/AnimatedPanZoomListener.js	(revision 40eb3c92a6ae990a0142a6c4a81ae71abc5e503b)
+++ scenery/js/listeners/AnimatedPanZoomListener.js	(date 1595953316580)
@@ -216,23 +216,25 @@
    * @param {SceneryEvent} event
    */
   move( event ) {
-    if ( this._downTrail && this._downInDragBounds ) {
-      this._repositionDuringDragPoint = null;
+    if ( this._downTrail ) {
+      if ( this._downInDragBounds ) {
+        this._repositionDuringDragPoint = null;
 
-      const hasDragIntent = this.hasDragIntent( event.pointer );
-      const currentTargetExists = event.currentTarget !== null;
+        const hasDragIntent = this.hasDragIntent( event.pointer );
+        const currentTargetExists = event.currentTarget !== null;
 
-      if ( currentTargetExists && hasDragIntent ) {
+        if ( currentTargetExists && hasDragIntent ) {
 
-        const globalTargetBounds = this._downTrail.parentToGlobalBounds( this._downTrail.lastNode().bounds );
-        const targetInBounds = this._panBounds.containsBounds( globalTargetBounds );
-        if ( !targetInBounds ) {
-          this._repositionDuringDragPoint = event.pointer.point;
-        }
-      }
-    }
-    else {
-      this._downInDragBounds = this._dragBounds.containsPoint( event.pointer.point );
+          const globalTargetBounds = this._downTrail.parentToGlobalBounds( this._downTrail.lastNode().bounds );
+          const targetInBounds = this._panBounds.containsBounds( globalTargetBounds );
+          if ( !targetInBounds ) {
+            this._repositionDuringDragPoint = event.pointer.point;
+          }
+        }
+      }
+      else {
+        this._downInDragBounds = this._dragBounds.containsPoint( event.pointer.point );
+      }
     }
   }
 
Index: sun/js/buttons/RadioButtonGroup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/RadioButtonGroup.js	(revision e8f067c20290d63e3d2e416e0089fe6f8a0e7c0d)
+++ sun/js/buttons/RadioButtonGroup.js	(date 1595908306480)
@@ -316,8 +316,9 @@
   } );
 
   // zoom - signify that key input is reserved and we should not pan when user presses arrow keys
-  const intentListener = { keydown: event => event.pointer.reserveForKeyboardDrag() };
-  this.addInputListener( intentListener );
+  // const intentListener = { keydown: event => event.pointer.reserveForKeyboardDrag() };
+  this.reserveForKeyboardDrag = true;
+  // this.addInputListener( intentListener );
 
   // When the entire RadioButtonGroup gets disabled, gray them out and make them unpickable (and vice versa)
   const enabledListener = function( isEnabled ) {
Index: sun/js/ComboBoxListBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBoxListBox.js	(revision e8f067c20290d63e3d2e416e0089fe6f8a0e7c0d)
+++ sun/js/ComboBoxListBox.js	(date 1595953572551)
@@ -198,6 +198,8 @@
     // @public {ComboBoxListItemNode|null} the ComboBoxListItemNode that has focus
     this.focusedItemNode = null;
 
+    this.reserveForKeyboardDrag = true;
+
     // pdom listener for the entire list box
     this.addInputListener( {
 

[jessegreenberg]

Discussed today with @jonathanolson and @zepumph -

Rather than go through #1056 (comment) we actually decided that not firing listeners that would call reserveForDrag is the correct behavior, as would be defined by pickable: false.

In the case of RadioButtonGroup where these listeners were not called on the selected radio button, we think that pickable: false should be removed from their implementation since that would also allow anything behind the radio buttons from being picked.

For this issue, we are going to continue making changes so that MultiListener can be added to the display as initially described in #1056 (comment).

[jessegreenberg]

Changes for this issue and #1056 (comment) are done. The above issues will track the requested change to RadioButtonGroup.

[jessegreenberg]

From discussion today for alt-input sprint, we identified this issue as needing a re-visit. pickable: false is no longer the shut-off valve for input listeners. inputEnabled is now to be used for that. The changes here may need to be reverted.