how to disable input listeners

[pixelzoom]

Over in phetsims/ph-scale#118 (comment) @kathy-phet noted about the dropper in ph-scale:

I will note it is not currently possible to make it unmoveable/unpickable without also making the add liquid button unpickable/not working.

I said:

Perhaps there should be a general solution for disabling a Node's drag listener?

@arouinfar said:

@kathy-phet and I think this is likely to come up in other sims like Bending Light. A client may want to fix the position of the LaserNode, but leave the button on it interactive. We're going to mark this issue for further discussion at PhET-iO meeting.

[pixelzoom]

pickableProperty (like visibleProperty and opacityProperty) applies to a Node and all of its children. So it makes sense that setting pickableProperty: false for the dropperNode would disable interactivity for all parts of dropperNode, including its push button. And I don't think we want to change that behavior.

How about adding the ability to disable listeners? We could add enabledProperty to all listeners (SimpleDragHandler, DragListener, PressListener,...) Then you would disable dragging of dropperNode by setting dropperNode.dragHandler.enabledProperty: false, rather than setting dropperNode.pickableProperty:false. Or if you wanted to to disable interactivity of the dropper's button, you'd set dropperNode.button.pressListener.enabledProperty: false.

[pixelzoom]

Over in phetsims/ph-scale#118, ph-scale design team decided that they want to move forward with this. I'll discuss with @jonathanolson.

[pixelzoom]

@jonathanolson The proposal is to add enabledProperty to all input listeners, so that they can be disabled directly via Studio or the PhET-iO API. Does this sound feasible? And what's the time estimate?

[kathy-phet]

@pixelzoom - My understanding from the meeting was to add it to the dragListener, but I didn't think it was to add it to all the listeners - since button presses don't need it for instance?

[pixelzoom]

@kathy-phet That was not my understanding from the design meeting, and doesn't match my meeting notes in phetsims/ph-scale#118 (comment). Making this feature specific to drag listeners is also something that I do not agree with. We are not trying to solve only the dropper button problem in ph-scale. This is a general issue that could occur for any input listener. And DragListener is a subclass of PressListener, so whatever is done for drag listeners should be applicable to buttons (which use PressListener). So I think we should first investigate how this would be done uniformly for input listeners.

[jonathanolson]

@jonathanolson The proposal is to add enabledProperty to all input listeners, so that they can be disabled directly via Studio or the PhET-iO API. Does this sound feasible? And what's the time estimate?

Only instrumented input listeners (e.g. SimpleDragHandler, PressListener variants, etc.), or ALL input listeners (e.g. node.addInputListener( { down: ... } ), DragListener.createForwardingListener, etc.)?

Presumably changing enabled to false would do the equivalent of interrupting the listener? How would "enabled" false work for hover state?

[jonathanolson]

Time estimate for SimpleDragHandler/PressListener wouldn't be too long, as long as the behavior is well defined. It will probably take longer to hook it up everywhere (as sun buttons or other things have their own "enabled" Properties, and it's unclear if we would want to tie those together).

[pixelzoom]

Presumably changing enabled to false would do the equivalent of interrupting the listener?

Yes.

How would "enabled" false work for hover state?

I don't know, mainly because I don't really understand the question.

While the immediate need for this is for drag listeners, I recommend doing this generally for input listeners, rather than just for drag listeners. That said, I'm going to bow out of this issue. This is a general PhET-iO design issue, and I'm way behind on other work. So designers (@arouinfar @kathy-phet) please communicate with @jonathanolson directly.

Note that this is needed for the dropper in pH Scale, dev version to be delivered to client by 3/30/2020.

[jonathanolson]

I'd like to understand more about the desired behavior. Disabling the input listener also wouldn't change the mouse hover cursor for most draggable things.

This also has more wide-ranging implications, if we need to change many simple input listeners from raw objects to something that can take a tandem. Additionally, what else in our input listeners should be instrumented?

@arouinfar and @kathy-phet, if this needs to be live by 3/30, it would help to discuss it soon!

[kathy-phet]

@arouinfar - Can you schedule a zoom discuss for this with @jonathanolson?

@pixelzoom - We can deliver a dev version without this feature being possible, but it is a bigger issue for other simulations - like Bending light and anything with a timer node.

[pixelzoom]

We can deliver a dev version without this feature being possible ...

@kathy-phet Is this a prerequisite to delivering the final PhET-iO version?

[kathy-phet]

Only becomes blocking if the client tags it as a blocking issue in their dev review. It wasn't requested by the client for this sim, and it can be added later if needed by a client. However, it did highlight a bigger issue that will occur across simulations, and be more critical in other simulations, so a conversation with @jonathanolson would be good.

[pixelzoom]

I'm wondering if input listeners is the correct (or only) place to disable interactivity.

A Node may have more than 1 input listener. If a client wants to disable interactivity for a Node, they may need to located multiple input listeners (all of which may not necessarily be featured) and set their enabledProperty to false. Strange things may happen if they don't disable all of a Node's input listeners. (No - limiting to a single input listener is not the solution, because it does not support separation of responsibilities.) On the other hand, if you want to be ultimately flexible, so you can disable some listeners and not others, then having an enabledProperty on each input listener seems useful.

It seems like it's more typical to want to disable all interactivity for a Node. Should there be a Node Property to do that? I know that sounds like pickableProperty, but I was thinking of something that's boolean, and doesn't have to deal with restoring pickableProperty to null or true.

[jonathanolson]

There are many trade-offs here, so it would be good to have a better understanding of what the general needs are.

[zepumph]

Possible options:

1. instrument pickableProperty. @jonathanolson feels like this is not really the ideal way of handling this, see (4).
2. create sim-specific Property that adds and remove the necessary input listeners to achieve the goals of the specific case.
3. Add enabledProperty to listeners. This is not a good solution because you may have to do one for the drag listener, and another for the keyboard listener. This in general would be clunky for the PhET-iO API client (studio user).
4. Add Node.interactiveProperty which is different from enabled, because enabled handles visual parts of Node too. This boolean Property will be more explicity than Pickable, which is focused around "transparency" of input instead of interactivity. This would likely replace every instrumented pickableProperty in the sim.
5. Leverage Node.enabledProperty, but remove the visual aspect of the component. This feels backwards to the group. Instead enabled should look at interactive to determine if you can use the component.

Basically this boils down to preferring (4), but needing to implement. I'll create a scenery issue for it. Otherwise use (2) if it feels right in a sim-specific case, or if it is needed before (4) is ready. This issue has been discussed and solved, now we just need to implement. Closing.

[samreid]

Reopening based on discussion in phetsims/scenery-phet#660. In that issue, there is a draggable LaserPointerNode with a button on it. Currently, the draggability is attend by adding the input listener directly to the laser. However, this means when you disable the drag listener (by setting the laser node to be non-interactive or non-pickable) you also disable the button. @pixelzoom indicated one solution would be to make it possible to disable the individual and put listeners. We decided against that in #1041 (comment), but maybe we need to re-evaluate.

Another solution investigated in phetsims/scenery-phet#660 (comment) was to add the input listeners to different Nodes, so they could be disabled separately. Is that a workaround or a solution to be embraced?

Also, in #1041 (comment) it was argued:

Add enabledProperty to listeners. This is not a good solution because you may have to do one for the drag listener, and another for the keyboard listener. This in general would be clunky for the PhET-iO API client (studio user).

Is there a way to pair the pointer input with the keyboard input so they can be disabled together?

Let's start by assigning to @zepumph for discussion, but we may bring this back to dev or phet-io meeting.

[zepumph]

Is that a workaround or a solution to be embraced?

This was the workaround used for the same thing in StopWatchNode, see https://github.com/phetsims/scenery-phet/blob/cc74b53ee659908ce9a7876be3f85c9446232ba6/js/StopwatchNode.js#L213.

Is there a way to pair the pointer input with the keyboard input so they can be disabled together?

If we decided to add enabledProperty to scenery listener Types (likely just PressListener), presumably we would do so with options, so you could pass through your own enabledProperty. If that was the case, then you could pass the same enabledProperty to your collection of listeners to make this a single Property control all input.

I really feel like this is a behavior only for PressListener. scenery listeners in general (potential just an object literal) should not be up-converted to something fancy just for this feature. We can see how far something in PressListener gets us, and there is always the option to utilize EnabledComponent or EnabledProperty in sim-specific cases to get the behavior you need.

What about something like this but much better?

Index: js/listeners/PressListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/listeners/PressListener.js b/js/listeners/PressListener.js
--- a/js/listeners/PressListener.js	(revision 4b032f5b97d71818393499d9ea41cf8f748cf67c)
+++ b/js/listeners/PressListener.js	(date 1612406659218)
@@ -27,6 +27,7 @@
 import DerivedProperty from '../../../axon/js/DerivedProperty.js';
 import stepTimer from '../../../axon/js/stepTimer.js';
 import merge from '../../../phet-core/js/merge.js';
+import EnabledComponent from '../../../sun/js/EnabledComponent.js';
 import EventType from '../../../tandem/js/EventType.js';
 import PhetioObject from '../../../tandem/js/PhetioObject.js';
 import Tandem from '../../../tandem/js/Tandem.js';
@@ -42,7 +43,7 @@
 // Factor out to reduce memory footprint, see https://github.com/phetsims/tandem/issues/71
 const truePredicate = _.constant( true );
 
-class PressListener {
+class PressListener extends EnabledComponent {
   /**
    * @param {Object} [options] - See the constructor body (below) for documented options.
    */
@@ -138,6 +139,8 @@
     assert && assert( typeof options.a11yLooksPressedInterval === 'number',
       'a11yLooksPressedInterval should be a number' );
 
+    super( options );
+
     // @private {number} - Unique global ID for this listener
     this._id = globalID++;
 
@@ -310,6 +313,8 @@
     // update isHighlightedProperty (not a DerivedProperty because we need to hook to passed-in properties)
     this.isHoveringProperty.link( this._isHighlightedListener );
     this.isPressedProperty.link( this._isHighlightedListener );
+
+    this.enabledProperty.lazyLink( enabled => !enabled && this.interrupt() );
   }
 
   /**
@@ -388,6 +393,10 @@
 
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} press` );
 
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
+
     if ( !this.canPress( event ) ) {
       sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} could not press` );
       return false;
@@ -538,6 +547,11 @@
    * @private
    */
   invalidateOver() {
+
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
+
     let pointerAttachedToOther = false;
 
     if ( this._listeningToPointer ) {
@@ -568,6 +582,9 @@
    * @private
    */
   invalidateHovering() {
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
     for ( let i = 0; i < this.overPointers.length; i++ ) {
       const pointer = this.overPointers[ i ];
       if ( !pointer.isDown || pointer === this.pointer ) {
@@ -694,6 +711,10 @@
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} enter` );
     sceneryLog && sceneryLog.InputListener && sceneryLog.push();
 
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
+
     this.overPointers.push( event.pointer );
 
     sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
@@ -727,6 +748,10 @@
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} exit` );
     sceneryLog && sceneryLog.InputListener && sceneryLog.push();
 
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
+
     // NOTE: We don't require the pointer to be included here, since we may have added the listener after the 'enter'
     // was fired. See https://github.com/phetsims/area-model-common/issues/159 for more details.
     if ( this.overPointers.includes( event.pointer ) ) {
@@ -748,6 +773,10 @@
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} pointer up` );
     sceneryLog && sceneryLog.InputListener && sceneryLog.push();
 
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
+
     // Since our callback can get queued up and THEN interrupted before this happens, we'll check to make sure we are
     // still pressed by the time we get here. If not pressed, then there is nothing to do.
     // See https://github.com/phetsims/capacitor-lab-basics/issues/251
@@ -771,6 +800,9 @@
   pointerCancel( event ) {
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} pointer cancel` );
     sceneryLog && sceneryLog.InputListener && sceneryLog.push();
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
 
     // Since our callback can get queued up and THEN interrupted before this happens, we'll check to make sure we are
     // still pressed by the time we get here. If not pressed, then there is nothing to do.
@@ -795,6 +827,9 @@
   pointerMove( event ) {
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} pointer move` );
     sceneryLog && sceneryLog.InputListener && sceneryLog.push();
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
 
     // Since our callback can get queued up and THEN interrupted before this happens, we'll check to make sure we are
     // still pressed by the time we get here. If not pressed, then there is nothing to do.
@@ -823,6 +858,9 @@
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} pointer interrupt` );
     sceneryLog && sceneryLog.InputListener && sceneryLog.push();
 
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
     this.interrupt();
 
     sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
@@ -844,6 +882,10 @@
    * @param {function} [callback] optionally called immediately after press, but only on successful click
    */
   click( event, callback ) {
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
+
     if ( this.canClick() ) {
       this.interrupted = false; // clears the flag (don't set to false before here)
 
@@ -887,6 +929,9 @@
    * @pdom
    */
   focus( event ) {
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
 
     // Get the Display related to this accessible event.
     const accessibleDisplays = event.trail.rootNode().getRootedDisplays().filter( display => display.isAccessible() );
@@ -908,6 +953,10 @@
    * @pdom
    */
   blur() {
+    if ( !this.enabledProperty.value ) {
+      return;
+    }
+
     if ( this.display ) {
       if ( this.display.focusHighlightsVisibleProperty.hasListener( this.boundInvalidateOverListener ) ) {
         this.display.focusHighlightsVisibleProperty.unlink( this.boundInvalidateOverListener );
@@ -958,6 +1007,8 @@
       this.display = null;
     }
 
+    super.dispose();
+
     sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
   }
 }

[pixelzoom]

@zepumph said:

.. I really feel like this is a behavior only for PressListener. scenery listeners in general (potential just an object literal) should not be up-converted to something fancy just for this feature.

I don't think this is exclusive to the PressListener hierachy. This seems important for KeyboardDragListener, and maybe some of the other next-gen input listeners in scenery/listeners/.

If instead you meant "I don't think this is necessary for old-stype input listeners and literal {...} listeners", then I agree with that.

[samreid]

@kathy-phet asked me to take the lead on this, as it pertains to Bending Light. @pixelzoom mentioned we may add LaserPointerNode.draggableEnabled which could turn off keyboard dragging + mouse/touch dragging, and LaserPointerNode.buttonEnabled which could turn off whether the button is enabled.

[pixelzoom]

2/4/21 phet-io meeting:

Expecting clients to set something like enabledProperty on specific listeners is probably asking too much of the clients. They would need to know which listeners correspond to which interactions, and there may be more than one -- for example DragListener and KeyboardDragListener for "dragging".

Using pickable has several disadvantage: it exposes internal structure, it may require changing internal structure, and it treats pickable as a boolean (it's actually 3 values). All of these problems were present in the recent investigation of bending-light LaserNode (phetsims/bending-light#397)

Rather than exposing internal listeners and Nodes, I suggested adding Properties for enabling/disabling types of interactions. For example a draggable laser might have dragEnabledProperty and buttonEnabledProperty. When the client sets these, pickable would be set for the corresponding internal Nodes. And if we decide not to use pickable in the future, then we could change to disable the listener(s) that correspond to the interaction.

[samreid]

I proposed a design and implementation strategy for this problem as it appears in the context of Bending Light, and it is pending review by @arouinfar and @pixelzoom in phetsims/bending-light#397 (comment), putting this more general issue on hold until we have discussed that specific case.

[samreid]

We agreed the implementation in phetsims/bending-light#397 seems like a suitable pattern for simulations (or common code) to use, and @zepumph will have an opportunity to try this pattern in Projectile Motion for some upcoming features. Perhaps after Projectile Motion, we can reflect on this process and determine if additional steps, such as documentation in the instrumentation guide, are warranted. @pixelzoom does that sound reasonable?

[pixelzoom]

Yep, sounds reasonable to me.

Not sure if there's something left to do here, or if this issue should be closed, or if it should stay open until @zepumph uses in Projectile Motion. So back to @samreid.

[pixelzoom]

Actually, I won't assign to @samreid, since this is already assigned to @zepumph.

[zepumph]

I have noted this pattern, and will pick this up over in PM as needed.