enabledAppearanceStrategy will blow away these options set by client

[zepumph]

@jonathanolson and I ran into this in #1116 where we were surprised that this code happened:

const x = new LayoutBox( {
  opacity: .2
})

x.opactity;
  -> 0.45;

This is because of this line of code:

scenery/js/nodes/LayoutBox.js

Lines 113 to 115 in 2285549

	// No need to dispose because enabledProperty is disposed in Node
	this.enabledProperty.link( enabled =>
	options.enabledAppearanceStrategy( enabled, this, { disabledOpacity: options.disabledOpacity } ) );

This link blows away our options mutation. I think this is expected, but it sure seems like a bummer.

[zepumph]

If we want to set opacity on a LayoutBox, then this is how we need to set it currently:

const x = new LayoutBox( {
  enabledAppearanceStrategy(enabled, node){
    node.opacity = .2
  }
})

x.opacity;
  -> .2;

Questions to consider

• Do we only care about this because LayoutBox is different from out other, more sun-like enabled-controlled components (like Slider).
• Should we completely reconsider how we support enabledAppearanceStrategy, doing something less rigid.
• Is anyone else bothered by the above?

Marking for developer meeting as part of the current inputEnabledProperty work being done in #1116.

[zepumph]

Steps for this issue:

• Panel should support enabledProperty
• Apply this patch to move inputEnabled into the Node enabled handler, thus enabledAppearanceStrategies will actually just be
  about appearance (not input functionality)

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 ea1383d9af036c9c652f3fb78e788134ad5b1520)
+++ b/sun/js/SunConstants.js	(date 1616094339704)
@@ -40,7 +40,6 @@
       disabledOpacity: SunConstants.DISABLED_OPACITY
     }, options );
 
-    node.inputEnabled = enabled;
     node.opacity = enabled ? 1.0 : options.disabledOpacity;
   }
 };
Index: sun/js/buttons/ButtonNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/buttons/ButtonNode.js b/sun/js/buttons/ButtonNode.js
--- a/sun/js/buttons/ButtonNode.js	(revision ea1383d9af036c9c652f3fb78e788134ad5b1520)
+++ b/sun/js/buttons/ButtonNode.js	(date 1616094339778)
@@ -85,8 +85,6 @@
       enabledAppearanceStrategy: ( enabled, button, background, content ) => {
         background.filters = enabled ? [] : [ CONTRAST_FILTER, BRIGHTNESS_FILTER ];
 
-        button.inputEnabled = enabled;
-
         if ( content ) {
           content.filters = enabled ? [] : [ Grayscale.FULL ];
           content.opacity = enabled ? 1 : SunConstants.DISABLED_OPACITY;
Index: scenery/js/nodes/Node.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.js b/scenery/js/nodes/Node.js
--- a/scenery/js/nodes/Node.js	(revision 4ac1519ff438ace8e94b358a99ecf750219a190a)
+++ b/scenery/js/nodes/Node.js	(date 1616094339769)
@@ -4269,6 +4269,7 @@
    */
   onEnabledPropertyChange( enabled ) {
     !enabled && this.interruptSubtreeInput();
+    this.inputEnabled = enabled;
   }
 
 

• Go through our usages of enabledAppearanceStrategy and refactor to make sure that client options aren't overridden by enabledProperty changes. The main solution seems to be to apply the enabledAppearanceStrategy would be applied to a child Node which doesn't overwrite what the client set. Therefore opacity would stack.

There was also talk about only applying this to "components" (which generally control their children. There was also talk of creating a default enabled appearance strategy baked into Node. These two ideas are at odds.

We decided to move this on to a sub group. I would like to be part of it. I think @jonathanolson will be part of it too. Not sure who else. @jbphet will also be a part of it.

@jonathanolson will write down his recommendations.

[jonathanolson]

I feel like we should avoid conflicts for node attributes that would be overridden by handling enabled/disabled appearance. Any opacity or filter or other thing that the appearance would set should stack nicely with what the user sets on the component's Node.

The simplest way to do that would be by creating a single child (e.g. for opacity), or by applying things to each child. Having a filter would also be possible (and perhaps more convenient), but may potentially hurt performance.

So, each component would be responsible for handling its own appearance strategy. The common "opacity" method should be factored out so code isn't duplicated. Buttons would be adding filters in the background like they do now. Some components might set opacity of a child, OR create a child to set opacity on in one place.

Notably, this also brought up what I consider to be a "component" - generally this to me means that the component (node) is responsible for all of its children, and you interact with it through defined APIs. Removing or messing with children of a component will likely mess with it.

This does not include layout containers (e.g. LayoutBox), where it gives control of its children explicitly to its clients.

If we implement the conflict-avoidance by adding in a child, it would not be possible to do that for layout containers.

[zepumph]

Apply this patch to move inputEnabled into the Node enabled handler, thus enabledAppearanceStrategies will actually just be
about appearance (not input functionality)

I created #1185 to do this.

[jonathanolson]

I'll add disabledOpacity as a field, that gets multiplied with the normal opacity when disabled.

[jonathanolson]

Pushed to a branch (not master), since the current SunConstants strategy uses the same name, and we'd be doubling up disabled opacities due to it, e.g.:

(new phet.scenery.HBox( { disabledOpacity: 0.5, enabled: false } )).effectiveOpacity
// 0.25

@zepumph can you review the implementation? I'm emitting on filterChangeEmitter whenever opacity or a filter changes, and this simplified things rather than actually listening to the trio of opacity/disabledOpacity/enabled. Ideally we'd use a DerivedProperty, but that would be way too much overhead.

[zepumph]

I reviewed this change by adding in my work for testing. This involved removing enabledAppearanceStrategy from all options besides ButtonNode, and making sure that disabledOpacity was declared if it wasn't already. There was also a usage in BASE of disabledOpacity that I renamed to avoid a collision.

Review of scenery change:

• Error instead of assertion in opacity setters, why? Shouldn't those be assertions?
• I think there is a type here, and that we want to remove listener on the filterChangeEmitter

  scenery/js/display/CanvasBlock.js

  Line 559 in 852f4f8

  node.clipAreaProperty.removeListener( this.opacityDirtyListener );

• scenery/js/scenery.js

  Line 342 in 852f4f8

  'effectiveOpacity',

  should probably be disabledOpacity, as effective doesn't get a setter.
• I don't understand the change in SVGGroup at all, but I'm alright with that if you are. Perhaps just a few words on if this was a necessary change, and if so why, or if it was a refactor bundled with this commit.

DerivedProperty, but that would be way too much overhead.

Does that mean we need a TinyDerivedProperty?

I think I will commit fixes for the small mistakes myself. I have to merge the branch in with the change to prevent regressions, so let's go forward on master.

[jonathanolson]

Error instead of assertion in opacity setters, why? Shouldn't those be assertions?

It's an error because of #1108

I think there is a type here, and that we want to remove listener on the filterChangeEmitter

Agreed with the fix.

should probably be disabledOpacity, as effective doesn't get a setter.

Agreed with the fix.

I don't understand the change in SVGGroup at all, but I'm alright with that if you are. Perhaps just a few words on if this was a necessary change, and if so why, or if it was a refactor bundled with this commit.

Maybe going over it together would be good? It had separate filter/opacity handling, and those were coalesced given the changes needed.

[zepumph]

Maybe going over it together would be good? It had separate filter/opacity handling, and those were coalesced given the changes needed.

That sentence was very helpful thanks

DerivedProperty, but that would be way too much overhead.

Does that mean we need a TinyDerivedProperty?

Could you please create an issue if you feel like this is the right direction, otherwise feel free to close. I don't think this should block publication anymore.

[jonathanolson]

Could you please create an issue if you feel like this is the right direction, otherwise feel free to close. I don't think this should block publication anymore.

If this comes up multiple times in the future, I'd propose it then. Status quo sounds great for right now. Thanks!