Move enabledProperty into Node

[zepumph]

Related to work done in #1047 and #1082 and phetsims/sun#585 and phetsims/sun#257. And from discussion with @samreid

The last time we discussed this was part of phetsims/sun#257 (comment), and there was no support for Property in Node at that time. We now have that, and also have precedent for adding PhET-iO instrumented Properties into Node (visible/text/pickable), see above issues.

With that in mind, I think we should take the awesome work that @pixelzoom did in phetsims/sun#585 in simplifying and organizing our Node enabledProperty usages and factor that out into Node.

As for the question of performance, @samreid and I dove into this in #1096, and from that feel like <10 new fields initialized to null in Node may not be that big of deal. Likely the best way forward for enabledProperty would be to initialize it to null.

[zepumph]

We should mark this for developer meeting to see if we want to move EnabledNode into Node. @samreid made the argument that EnabledComponent is nice, and factored out, but is actually pretty simple, and may be more effort than it's worth to keep totally factored out into a single place. We could keep EnabledComponent for the three usages that currently use it (search for EnabledComponent.mixInto), which will likely be more if we keep it around, and then duplicate the simple implementation over in Node too, but add the extra logic that EnabledNode adds.

Marking for dev meeting, and marking high priority because of the overhead of doing timely work in Node before we have to relearn the current implementation.

[zepumph]

@samreid also recommended that we could factor out a Property called EnabledProperty, which is the main logic in EnabledComponent:

 new BooleanProperty( options.enabled, merge( {
          tandem: options.tandem.createTandem( ENABLED_PROPERTY_TANDEM_NAME ),
          phetioDocumentation: 'When disabled, the component is grayed out and cannot be interacted with.',
          phetioFeatured: true
        }, options.enabledPropertyOptions )

To be created if one isn't provided. The type name "EnabledProperty" wouldn't be instanceof checkable though, because you can still pass in any Property for it.

[pixelzoom]

EnabledProperty sounds like a good idea. But let's work on that proposed phetioDocumentation, keeping in mind that EnabledProperty can also be used with model elements.

phetioDocumentation: 'When disabled, the component is grayed out and cannot be interacted with.'

"component" - PhET-iO doesn't have "components", it has "elements".

"grayed out" - incorrect (we change opacity, not color palette), and specific to UI components

"cannot be interacted with" - specific to UI components

"with." - We can do better than ending with a preposition.

So maybe something like:

phetioDocumentation: 'Determines whether the element is enabled (true) or disabled (false).'

[samreid]

Moving my comment from: phetsims/phet-info#143 (comment)

Now that Node supports TinyProperty, and has established support for passing in properties, PhET-iO support (linked elements & instrumentation), perhaps we should move enabledProperty directly to Node.

UPDATE: Upon reflection, I realized that is the premise of this entire issue.

[zepumph]

From developer meeting discussion today, the best outcome from a coding and API standpoint would be to move enabledProperty into Node, and no longer have EnabledNode. The main concern here is about performance and memory. @samreid did a single test adding 10 TinyProperties per Node in Natural selection (all set to true), and found that each additional tinyProperty in Node seems to add ~.6% memory to NS. In this case it was a few hundred KBs of memory. Not nothing, but also not immediately prohibitive.

From this we decided that we will prusue adding enabledProperty to Node, in the same pattern as we do for visible/pickable, and then I will create a new issue about trying to slim down Node.js, as it has become a total beast (especially adding PhetioObject as its base type and ParallelDOM mixed in.

I still feel like we are in the investigation phase, and not blinding implementing the pattern. I am still uncertain about a couple of pieces:

• What to do about the mixin EnabledComponent, do we factor some stuff out and duplicate some logic in Node and EnabledComponent like we talked about above?
• For PhET-iO instrumentation, are we going to always instrument every instrumented Node's enabledProperty? Like visibleProperty? Or will it be more of an opt-in approach like pickable? If the latter, than isn't that exactly what we were trying to get rid of by moving from the EnabledNode trait (opt-in) to the feature built into Node (ubiquitous).
• We need to do better performance checks before implementing (as I mention above in this same comment).

@samreid anything else to comment? want to investigate this tomorrow?

[zepumph]

I think we should work on #1097 first, and then try to apply what ever composition pattern over there to enabled directly in Node here.

[zepumph]

I want to get this in for the GAO release for completeness.

[samreid]

Should we mark this as blocks-publication? Would you like to take the lead?

[zepumph]

I have commits locally, it shouldn't be long before a push comes.

[zepumph]

Is this code in TwoSceneSelectionNode extraneous?

This caused me to search for other declarations of setEnabled, and I created phetsims/balancing-chemical-equations#145.

[zepumph]

I went through review comments, and looked at all issues linked above. Things are looking really good here! I think we are ready to close this.

[samreid]

Reopening based on a TODO in the code, not sure if it can be deleted or needs attention.

[pixelzoom]

Reopening.

I'm curious about these 2 confusing (and possibly buggy) lines:

1885 assert.ok( pdomNode.pdomInstances[ 0 ].peer.primarySibling.getAttribute( 'aria-disabled' ) !== 'true', 'should be not enabled' );

1889 assert.ok( pdomNode.pdomInstances[ 0 ].peer.primarySibling.getAttribute( 'aria-disabled' ) === 'false', 'should be not enabled' );

Why are you checking !== 'true' in one case, === 'false' in the other?

In both cases, you're confirming that 'aria-disabled' is 'false'. Besides the fact that the current message doesn't parse well.... Shouldn't the message be the opposite of what you have, something like 'should be disabled' or 'aria-disabled should be false'?

[pixelzoom]

OK, now I understand the 'should be not enabled' message. But it sure is confusing. 'aria-disabled should be false' would be so much clearer and straightforward.

[zepumph]

I agree, the messages left much to be desired. Updated above. As for the actual tests, I was trying to figure out the balance between testing a feature vs its implementation. I landed on testing the actual implementation, in which the aria-disabled attribute will not be added until enabled is toggled, because although behavior could be unchanged when these tests fail, I want to know and rethink things if the implementation ever changed out from under us.

Ready to close?

[pixelzoom]

I still don't understand the purpose of using !== true vs === false, but maybe I don't need to. So closing.