Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enabledProperty should run on inputEnabled instead of pickable #1175

Closed
zepumph opened this issue Mar 12, 2021 · 3 comments
Closed

enabledProperty should run on inputEnabled instead of pickable #1175

zepumph opened this issue Mar 12, 2021 · 3 comments

Comments

@zepumph
Copy link
Member

zepumph commented Mar 12, 2021

With #1116 now complete, such that inputEnabled supports dynamic toggling with proper enter/exit events with scenery input, we are ready to flip the switch in our enabledAppearanceStrategies. There are two spots for this:

SunConstants.componentEnabledListener and in ButtonNode's default enabledAppearanceStrategy option.

@zepumph
Copy link
Member Author

zepumph commented Mar 12, 2021

@jonathanolson and I used the snapshot comparison tool and compared master (including all the changes from the end of #1116) with this patch:

Index: js/SunConstants.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/SunConstants.js b/js/SunConstants.js
--- a/js/SunConstants.js	(revision a3b580915543874ffe99a2cb55c8d64f8761cb84)
+++ b/js/SunConstants.js	(date 1615584957091)
@@ -40,7 +40,7 @@
       disabledOpacity: SunConstants.DISABLED_OPACITY
     }, options );
 
-    node.pickable = enabled;
+    node.inputEnabled = enabled;
     node.opacity = enabled ? 1.0 : options.disabledOpacity;
   }
 };
Index: js/buttons/ButtonNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/ButtonNode.js b/js/buttons/ButtonNode.js
--- a/js/buttons/ButtonNode.js	(revision a3b580915543874ffe99a2cb55c8d64f8761cb84)
+++ b/js/buttons/ButtonNode.js	(date 1615584957086)
@@ -85,8 +85,7 @@
       enabledAppearanceStrategy: ( enabled, button, background, content ) => {
         background.filters = enabled ? [] : [ CONTRAST_FILTER, BRIGHTNESS_FILTER ];
 
-        // TODO: this is a workaround until we can use Node.interactive, see https://github.com/phetsims/scenery/issues/1116
-        button.pickable = enabled ? null : false;
+        button.inputEnabled = enabled;
 
         if ( content ) {
           content.filters = enabled ? [] : [ Grayscale.FULL ];

There were a good number of cases that changed (yay! the tool works). They seem to all be reasonable and acceptable improvements, and not regressions. I'll explain each one:

  • StopwatchNode's reset button used to be set to pickable:false, which means that it had pass-through behavior. Thus its disabled state meant you could drag the tool with presses on it. This came up in CCK, MAS, and others with a stopwatch node. After the above patch, The disabled button absorbs the press, and so you can't drag on the StopwatchNode by dragging on the disabled reset button.

  • In CCK, the "cut vertex" button is disabled on leaf vertices in a circuit. In the old code, clicking on it would pass through and act like you clicked away from the cutout button/modal. After the above patch, the disabled button absorbs the hit, so the cut out button stays visible, and the press does nothing. @samreid, this feels like better usability to me, but please create an issue if this is not acceptable.
    image (5)

  • In Fraction Comparison, you can drag the "fraction block" behind a disabled button. Basically this is the same behavior as the above: with the above patch, the press is absorbed by the disabled button:
    image

  • In Friction, there are many differences in the position of the atoms, but I can't figure out why and how. This is still a bit of a question to me, but even so, I feel good enough to commit and figure this out as a single case later on.
    image

For investigating Friction, I tried the following:

  • Looked for any usages of enabled in friction
  • Looked for usages of non-deterministic randomness, like Math.random()
  • Looked for any usages of inputEnabled or enabled in canvas-related common code types (which is how the atom was drawn)
  • I wondered if it had to do with clipArea, but that only effects hit testing to generate a trail, and not whether input listeners are fired (kinda). @jonathanolson does this sound like it could be part of the problem?

I couldn't find anything substantive, for Friction.

I'm going to commit.

@zepumph
Copy link
Member Author

zepumph commented Mar 12, 2021

@jonathanolson, can you think of why friction would be showing these differences? Also please review in general.

@jonathanolson
Copy link
Contributor

Review in general is good! I looked more into Friction, and I'm not precisely sure. Since it seems to be operating fine, I don't have enough concerns to continue diving into why there was a potential change.

Looks good, closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants