Add PhET-iO support for Node.inputEnabledProperty

[zepumph]

From #1041, many devs feel like it would be nice to create an interactiveProperty that toggles whether or not input listeners fire on a Node. Up until this point, we have been using pickable to achieve this, but @jonathanolson feels that this is not really the point of pickable, as pickable:false will just let the pointer pass through the Node.

Marking for dev meeting to see what is next here.

[jessegreenberg]

Is there a difference between the proposed Node.interactiveProperty and Node.inputEnabledProperty which already exists? Is this already a supported feature?

[samreid]

It's unclear how this interleaves with #1041, --perhaps they can be handled separately.

[samreid]

@kathy-phet asked me to take the lead, and coordinate with @jonathanolson and bring proposals to the dev team.

@pixelzoom suggests that maybe #1041 would make this issue unnecessary.

[pixelzoom]

While interactiveProperty may still be useful for making a Node totally interactive, I'm feeling like interactiveProperty is too coarse-grained to meet some recent requirements that have come up. For example in bending-light: disable transforming the laser, but keep the button interactive.

As I described in #1041 (comment), I think we should consider Properties that correspond to specific high-level interactions (drag, press, ...) so that the client can disable specific interactions, while keeping other interactions enabled.

[zepumph]

Is there a difference between the proposed Node.interactiveProperty and Node.inputEnabledProperty which already exists? Is this already a supported feature?

Currently inputEnabledProperty exits but doesn't do anything. Perhaps it can be co-opted into whatever we want here.

We half (three quarters?) discussed this today. It would be nice to get this off of dev meeting. Marking high to finish off the discussion.

[jessegreenberg]

inputEnabledProperty exits but doesn't do anything

It currently works to prevent listeners from firing but keeps things in the hit test, but maybe there are other things that are needed here that are not supported yet.

[zepumph]

Ahh sorry, I see that now @jessegreenberg thanks.

It seems like Node.inputEnabledProperty is the thing to use here. I will check in with @jonathanolson to see if this aligns with what we are trying to accomplish.

• Can inputEnabledProperty work or do we need our own, new Node.interactiveProperty, YES
• If these are the same, should we rename inputEnabledProperty to Node.interactiveProperty, UNCERTAIN LET PHET_IO DESIGNERS DECIDE.
• We should potentially update sun enabled listeners to be run on this, YES
• add optional phet-io instrumentation for this (this issue), and remove it from pickable (see Uninstrument Node.pickableProperty in exchange for Node.inputEnabledProperty #1158). YES
• We may want to make sure that the whole event doesn't fire? NO
• (see Uninstrument Node.pickableProperty in exchange for Node.inputEnabledProperty #1158) There are 303 usages of pickable:false (and more with pickable = false, many of which likely are just trying to prevent input, and not represent a transparent-to-input Node
  • @jonathanolson's thoughts: You definitely want to set pickable on "foreground" components.
  • "Do I need to pick things through it"
  • "Do I want to prevent hit test calculation on children.
  • pickable: false was largely used correctly, on items that were never going to get input, but .pickable = is more dynamic. AreaBuilderGameView.challengeLayer sets pickable dynamically (between false and null) where it could likely get away with setting inputEnabled instead.
  • canStartPress: was another spot that demonstrated that you COULD potentially use Node.setInputEnabled (so long as all listeners on that Node would apply, since canStartPress is just for a single input listener. I.E. ChargedParticleNode.js
  • (see Uninstrument Node.pickableProperty in exchange for Node.inputEnabledProperty #1158) Likely the only work to be done here (retroactively change usages) is on PhET-iO sims using instrumented Pickable.
• (see Uninstrument Node.pickableProperty in exchange for Node.inputEnabledProperty #1158) Uninstrument pickable for phet-io, so long as there are no cases that need it. (new issue?)

[zepumph]

Next steps are for me to discuss with @jonathanolson, does anyone else want to be involved in this work?

[zepumph]

From meeting with @jonathanolson:

Likely we can use Node.inputEnabledProperty
re the rename, "interactive" sounds like there is always a change in the Node you are blocking, as opposed to just the input being blocked. We should ask designers.

I edited #1116 (comment) with the rest of our notes.

[zepumph]

Today in PhET-iO meeting we decided that inputEnabledProperty is acceptable. We will not rename. I'll change the name of the issue.

[pixelzoom]

I just did a pull-all.sh, and I'm seeing MAJOR broken-ness of UI components in all sims. For example, none of the checkboxes, radio buttons, or push buttons work in Natural Selection. Ditto in Molecule Polarity, Graphing Quadratics, Beer’s Law Lab, Fourier, … every sim that I’m working on now. I’m dead in the water, can’t continue. Can we please revert whatever is responsible for this ASAP?

[samreid]

I confirmed that reverting 3f7946d fixes inputs.

[samreid]

I pushed a revert for now, but @zepumph and @jessegreenberg should comment and proceed as appropriate.

[jonathanolson]

It seems like for 3f7946d, the conditional is reversed (we'd want i <= inputEnabledIndex).

For the rest of the changes in #1116 (comment), presumably we could branch scenery, both to inspect changes there, and also to test?

[zepumph]

Yes, I'm sorry. I changed the inequality right before commit and didn't commit. This is what @jessegreenberg and I tested, and what was working.

Index: js/input/Input.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/input/Input.js b/js/input/Input.js
--- a/js/input/Input.js	(revision 9adba82eff9ed09d351573ebba6b674e4f0cef72)
+++ b/js/input/Input.js	(date 1615422785482)
@@ -1862,11 +1862,12 @@
     if ( inputEvent.aborted || inputEvent.handled ) {
       return;
     }
+    const inputEnabledIndex = trail.getLastInputEnabledIndex();
 
     for ( let i = trail.nodes.length - 1; i >= 0; bubbles ? i-- : i = -1 ) {
 
       const target = trail.nodes[ i ];
-      if ( target.isDisposed || !target.inputEnabled ) {
+      if ( target.isDisposed || inputEnabledIndex < i ) {
         continue;
       }
 

[zepumph]

It seems like for 3f7946d, the conditional is reversed (we'd want i <= inputEnabledIndex).

Yes, but the opposite to continue; (therefore not calling listeners)

[zepumph]

For this issue, I think we may be done when we do two things:

• Apply the patch in Add PhET-iO support for Node.inputEnabledProperty #1116 (comment)
• Apply the patch in Add PhET-iO support for Node.inputEnabledProperty #1116 (comment)
• UPDATE: And apply the patch in sun to have enabled run on inputEnabled instead of pickable. UPDATE: This will be done in enabledProperty should run on inputEnabled instead of pickable #1175

I will need to discuss with @jonathanolson further though.

[zepumph]

I pushed commits to a new branch called inputEnabled.

Please:

• checkout the branch
• Apply this patch

Index: sun/js/SunConstants.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/SunConstants.js b/sun/js/SunConstants.js
--- a/sun/js/SunConstants.js	(revision 367951f76fb6bc1cab0fac43218d0dd5cae4e70c)
+++ b/sun/js/SunConstants.js	(date 1615492250500)
@@ -40,7 +40,7 @@
       disabledOpacity: SunConstants.DISABLED_OPACITY
     }, options );
 
-    node.pickable = enabled;
+    node.inputEnabled = enabled;
     node.opacity = enabled ? 1.0 : options.disabledOpacity;
   }
 };
Index: acid-base-solutions/js/common/view/ViewsPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/acid-base-solutions/js/common/view/ViewsPanel.js b/acid-base-solutions/js/common/view/ViewsPanel.js
--- a/acid-base-solutions/js/common/view/ViewsPanel.js	(revision 51da005dc0ff477a67da47aa3b2b0b3943bcd4e2)
+++ b/acid-base-solutions/js/common/view/ViewsPanel.js	(date 1615492250471)
@@ -115,6 +115,12 @@
       ]
     } );
 
+    let swap = true;
+    setInterval( () => {
+      viewModeProperty.value = swap ? ViewMode.GRAPH : ViewMode.MOLECULES;
+      swap = !swap;
+    }, 1000 );
+
     const content = new AlignBox( new VBox( {
       spacing: 8,
       align: 'left',

Then there are two ways to test this in sims:

1. In Acid base solutions, open the first screen and note the enabled of the solvent checkbox toggling. Hover over it and play with it as it toggles, and see that it works well. (at least it did for me)
2. Open ratio and proportion in Studio, go to the first screen and toggle ratioAndProportion.discoverScreen.view.antecedentRatioHalf.inputEnabledProperty to be false, then note that you can't move the left hand, and while you can keyboard focus it, you can't interact with it with the arrow keys.

I'm looking forward to our meeting tomorrow @jonathanolson!

[zepumph]

@jonathanolson and I met this morning, and basically my above code is really verbose and sloppy. We can rethink this issue by saying that branchChangeEvents should only care about inputEnabled trails, so we don't need to run exit/enter events both both the full trail and the inputEnabled trail differences, but rather just the inputEnabled ones.

While working on this we realized that scenery event ordering was backwards from the mouseenter/mouseleave spec in https://www.w3.org/TR/uievents/#events-mouseevent-event-order. @jonathanolson is also cleaning that up as we go too.

In summary, inputEnabled was not a dynamic feature of scenery at all before this issue, but will be well set up to take on that responsibility after this work.

[zepumph]

@jonathanolson and I worked on this today, and felt good enough to commit to master. Snapshot comparison did not show any differences in all phetsims, and we messaged slack to be on the look out. Swapping enabled logic will happen in #1175. We also created a couple of other side issues to investigate along the way. I think we are ready for review and cleanup for this issue. We should also see if CT has anything to say about these changes. I deleted the inputEnabled branch.

[zepumph]

I just went through this whole issue and cleaned up loose ends. Pretty much the majority of the committing occurred in the first commit (to add inputEnabledProperty as an optionally instrumented Property), and at the end, to support enter/exit events properly occurring on inputEnabled/Disabled Nodes, both while toggling inputEnabled, and in general.

@jonathanolson, I'm not sure how this issue should be reviewed. Presumably it would get a fresh set of eyes on it right? I'm not sure who that would be though. Do you have a recommendation on how to proceed?

[zepumph]

We discussed this during PhET-iO meeting today, and @jonathanolson and I think that we can close this. We feel good about the changes, but hope for QA to hammer on the following items for the next RC that has it.

PhET-iO Studio may be helpful in testing these:

• Focus on toggling enabled/inputEnabled and the behavior when it changes.
• Focus on input events, and perhaps multi-touch on and around those components getting inputEnabled or enabled toggled
• Take a look also at hover behavrior as enabled/inputEnabled is toggled.

QA, please create new issues for any problems you find, and link to this issue if applicable.