How to handle enabled in Node

[zepumph]

Over in #1100, enabledProperty was moved to Node. Currently, default logic is hard coded in Node.onEnabledPropertyChange to do one thing, but it isn't flexible:

  /**
   * Called when enabledProperty changes values.
   * @protected - override this to change the behavior of enabled
   *
   * @param {boolean} enabled
   */
  onEnabledPropertyChange: function( enabled ) {
    !enabled && this.interruptSubtreeInput();
    this.pickable = enabled;
    this.opacity = enabled ? 1.0 : this._disabledOpacity;
  },

The only way to change this behavior currently is to override this method.

In a discussion today with @jonathanolson, multiple problems were pointed out with this pattern.

1. this.pickable = enabled; is not the way we want to handle disabling input generally. This is because certain components, like buttons, may have particular input listeners that may still want to fire on a disabled component. We cannot make this assumption in Node.js that would effect all Nodes. @jonathanolson mentioned potentially having PressListener generally respect enabled, but I'm not sure we actually felt like that was a reasonable approach.
2. This hard coded logic doesn't appropriately reset the view when going from disabled->enabled. By hard-cording an enabled opacity of 1, and assuming that pickable should be true (see Should Node.enabledProperty respect pickable:null? #1105). This is not a reasonable assumption.
3. We went through many usages in the project of enabledProperty.link, which found lots of spots that were using logic different from this standard, hard-coded logic we centralized sun components around. We need a way to be flexible to all enabled needs of components.
4. Expanding on (3), we found some differing cases that made us question if the discrepancies were purposeful, or just due to the fact that we have never standardized on what the look and feel of PhET enabled/disabled should be. Likely this should be a separate issue to discuss looking at existing usages. For example, shouldn't enabled set cursor in some standard way? (UPDATE: oh, I see that was actually removed as part of phetsims/sun@add7c94)
5. From this, there was much discussion over whether or not enabledProperty should live in Node. @jonathanolson tended towards no from a design standpoint, because it is clients of Node that decide how enabled logic should work. I feel like mixed and torn, but edge toward the fact enabledProperty in Node makes a lot of things more convenient, at the cost of adding to the Node API. We can most likely decide to keep it in Node and solve the above problems.

I think we should discuss at dev meeting. @jonathanolson has brought up many good points. I also think it deserves a high priority to make sure that unstable code doesn't sit in Node for too long.

[samreid]

Additionally, we decided sims should not be published until buttons have "disabled/grayscale". I'm not sure if that is tracked elsewhere, but I'll mark this as blocks-publication for now.

[zepumph]

Additionally, we decided sims should not be published until buttons have "disabled/grayscale". I'm not sure if that is tracked elsewhere, but I'll mark this as blocks-publication for now.

Perhaps you are thinking of phetsims/sun#658?

[zepumph]

During developer meeting we all agreed that having default enabled behavior in Node was problematic. Today I hoped to make improvements to Node, by removing the default and moving a to centralized sun component listener for most of the cases that were covered by EnabledNode prior to #1100.

@jonathanolson, I don't think we are done yet, but I feel like this was the most important blocking work to be done.

Please review , especially SunConstants.getComponentEnabledListener() and the changes to Node. Where should we go from here? For now I think that we should keep this marked for dev meeting to make sure consistent progress is made on this.

[jonathanolson]

Looks like it's on the right path, but I will want to discuss how we can override this behavior on a subtype or specific component instance level.

[zepumph]

In phetsims/sun@7d5cc3e, I converted ButtonNode to an overridable enabled strategy. I'll do the rest of them now.

[zepumph]

Above, we now have an option allowing overriding of any enabled appearance strategy.

@jonathanolson, I feel like all is done here, or separated out in to other issues. Will you please review this (and note the work also done over in phetsims/sun#658. Anything else here?

[zepumph]

I don't think this needs to be labelled for meeting anymore.

[jonathanolson]

Looks great here, thanks!