review of mixins, remove TODOs mostly, #1340

js/accessibility/voicing/InteractiveHighlighting.ts

@@ -35,6 +35,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
   assert && assert( _.includes( inheritance( Type ), Node ), 'Only Node subtypes should compose InteractiveHighlighting' );
 
   const InteractiveHighlightingClass = class extends Type {
+
     // Input listener to activate the HighlightOverlay upon pointer input. Uses exit and enter instead of over and out
     // because we do not want this to fire from bubbling. The highlight should be around this Node when it receives
     // input.
@@ -45,13 +46,12 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
     // at one time.
     _pointer: null | Pointer;
 
-    // protected - A map that collects all of the Displays that this InteractiveHighlighting Node is
+    // @protected - A map that collects all of the Displays that this InteractiveHighlighting Node is
     // attached to, mapping the unique ID of the Instance Trail to the Display. We need a reference to the
     // Displays to activate the Focus Property associated with highlighting, and to add/remove listeners when
     // features that require highlighting are enabled/disabled. Note that this is updated asynchronously
     // (with updateDisplay) since Instances are added asynchronously.
-    // TODO: this should be protected, how to conventionize this?. https://github.com/phetsims/scenery/issues/1340
-    _displays: { [ key: string ]: Display };
+    displays: { [ key: string ]: Display };
 
     // The highlight that will surround this Node when it is activated and a Pointer is currently over it. When
     // null, the focus highlight will be used (as defined in ParallelDOM.js).
@@ -63,8 +63,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
     // this.interactiveHighlightActivated is true.
     _interactiveHighlightLayerable: boolean;
 
-    // Emits an event when the interactive highlight changes for this Node
-    // TODO: this should be protected, how to conventionize this?. https://github.com/phetsims/scenery/issues/1340
+    // @protected - Emits an event when the interactive highlight changes for this Node
     interactiveHighlightChangedEmitter: TinyEmitter;
 
     // When new instances of this Node are created, adds an entry to the map of Displays.
@@ -96,7 +95,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
       };
 
       this._pointer = null;
-      this._displays = {};
+      this.displays = {};
       this._interactiveHighlight = null;
       this._interactiveHighlightLayerable = false;
       this.interactiveHighlightChangedEmitter = new TinyEmitter();
@@ -194,9 +193,9 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
     isInteractiveHighlightActivated(): boolean {
       let activated = false;
 
-      const trailIds = Object.keys( this._displays );
+      const trailIds = Object.keys( this.displays );
       for ( let i = 0; i < trailIds.length; i++ ) {
-        const pointerFocus = this._displays[ trailIds[ i ] ].focusManager.pointerFocusProperty.value;
+        const pointerFocus = this.displays[ trailIds[ i ] ].focusManager.pointerFocusProperty.value;
 
         // @ts-ignore // TODO: fixed once FocusManager is converted to typescript https://github.com/phetsims/scenery/issues/1340
         if ( pointerFocus && pointerFocus.trail.lastNode() === this ) {
@@ -219,13 +218,13 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
       }
 
       // remove listeners on displays and remove Displays from the map
-      const trailIds = Object.keys( this._displays );
+      const trailIds = Object.keys( this.displays );
       for ( let i = 0; i < trailIds.length; i++ ) {
-        const display = this._displays[ trailIds[ i ] ];
+        const display = this.displays[ trailIds[ i ] ];
 
         // @ts-ignore // TODO: fixed once FocusManager is converted to typescript https://github.com/phetsims/scenery/issues/1340
         display.focusManager.pointerHighlightsVisibleProperty.unlink( this._interactiveHighlightingEnabledListener );
-        delete this._displays[ trailIds[ i ] ];
+        delete this.displays[ trailIds[ i ] ];
       }
 
       // @ts-ignore
@@ -238,7 +237,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
      */
     _onPointerEntered( event: SceneryEvent ) {
 
-      const displays = Object.values( this._displays );
+      const displays = Object.values( this.displays );
       for ( let i = 0; i < displays.length; i++ ) {
         const display = displays[ i ];
 
@@ -253,7 +252,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
 
     _onPointerMove( event: SceneryEvent ) {
 
-      const displays = Object.values( this._displays );
+      const displays = Object.values( this.displays );
       for ( let i = 0; i < displays.length; i++ ) {
         const display = displays[ i ];
 
@@ -279,7 +278,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
      */
     _onPointerExited( event: SceneryEvent ) {
 
-      const displays = Object.values( this._displays );
+      const displays = Object.values( this.displays );
       for ( let i = 0; i < displays.length; i++ ) {
         const display = displays[ i ];
         display.focusManager.pointerFocusProperty.set( null );
@@ -292,7 +291,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
     _onPointerDown( event: SceneryEvent ) {
 
       if ( this._pointer === null ) {
-        const displays = Object.values( this._displays );
+        const displays = Object.values( this.displays );
         for ( let i = 0; i < displays.length; i++ ) {
           const display = displays[ i ];
           const focus = display.focusManager.pointerFocusProperty.value;
@@ -321,7 +320,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
      */
     _onPointerRelease( event?: SceneryEvent ) {
 
-      const displays = Object.values( this._displays );
+      const displays = Object.values( this.displays );
       for ( let i = 0; i < displays.length; i++ ) {
         const display = displays[ i ];
         display.focusManager.lockedPointerFocusProperty.value = null;
@@ -340,7 +339,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
      */
     _onPointerCancel( event?: SceneryEvent ) {
 
-      const displays = Object.values( this._displays );
+      const displays = Object.values( this.displays );
       for ( let i = 0; i < displays.length; i++ ) {
         const display = displays[ i ];
         display.focusManager.pointerFocusProperty.set( null );
@@ -374,7 +373,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
       assert && assert( instance.trail, 'should have a trail' );
 
       if ( added ) {
-        this._displays[ instance.trail!.uniqueId ] = instance.display;
+        this.displays[ instance.trail!.uniqueId ] = instance.display;
 
         // Listener may already by on the display in cases of DAG, only add if this is the first instance of this Node
         if ( !instance.display.focusManager.pointerHighlightsVisibleProperty.hasListener( this._interactiveHighlightingEnabledListener ) ) {
@@ -383,7 +382,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
       }
       else {
         assert && assert( instance.node, 'should have a node' );
-        const display = this._displays[ instance.trail!.uniqueId ];
+        const display = this.displays[ instance.trail!.uniqueId ];
 
         // If the node was disposed, this display reference has already been cleaned up, but instances are updated
         // (disposed) on the next frame after the node was disposed. Only unlink if there are no more instances of
@@ -394,7 +393,7 @@ const InteractiveHighlighting = <SuperType extends Constructor>( Type: SuperType
           display.focusManager.pointerHighlightsVisibleProperty.unlink( this._interactiveHighlightingEnabledListener );
         }
 
-        delete this._displays[ instance.trail!.uniqueId ];
+        delete this.displays[ instance.trail!.uniqueId ];
       }
     }
 

js/accessibility/voicing/ReadingBlock.ts

@@ -53,9 +53,8 @@ const ReadingBlock = <SuperType extends Constructor>( Type: SuperType, optionsAr
 
   assert && assert( _.includes( inheritance( Type ), Node ), 'Only Node subtypes should compose Voicing' );
 
-  const VoicingClass = Voicing( Type, optionsArgPosition );
+  const ReadingBlockClass = class extends Voicing( Type, optionsArgPosition ) {
 
-  const ReadingBlockClass = class extends VoicingClass {
     // The tagName used for the ReadingBlock when "Voicing" is enabled, default
     // of button so that it is added to the focus order and can receive 'click' events. You may wish to set this
     // to some other tagName or set to null to remove the ReadingBlock from the focus order. If this is changed,
@@ -223,10 +222,10 @@ const ReadingBlock = <SuperType extends Constructor>( Type: SuperType, optionsAr
     isReadingBlockActivated(): boolean {
       let activated = false;
 
-      const trailIds = Object.keys( this._displays );
+      const trailIds = Object.keys( this.displays );
       for ( let i = 0; i < trailIds.length; i++ ) {
 
-        const pointerFocus = this._displays[ trailIds[ i ] ].focusManager.readingBlockFocusProperty.value;
+        const pointerFocus = this.displays[ trailIds[ i ] ].focusManager.readingBlockFocusProperty.value;
 
         // @ts-ignore // TODO: fixed once FocusManager is converted to typescript https://github.com/phetsims/scenery/issues/1340
         if ( pointerFocus && pointerFocus.trail.lastNode() === this ) {

js/accessibility/voicing/Voicing.ts

@@ -20,10 +20,8 @@
  * exception is on the 'focus' event. Every Node that composes Voicing will speak its responses by when it
  * receives focus.
  *
- * NOTE: At this time, you cannot use Voicing options and pass options through super(), instead you must call mutate
- * as a second statement. TODO: can we get rid of this stipulation? https://github.com/phetsims/scenery/issues/1340
- *
  * @author Jesse Greenberg (PhET Interactive Simulations)
+ * @author Michael Kauzmann (PhET Interactive Simulations)
  */
 
 import inheritance from '../../../../phet-core/js/inheritance.js';
@@ -74,10 +72,8 @@ const Voicing = <SuperType extends Constructor>( Type: SuperType, optionsArgPosi
 
   assert && assert( _.includes( inheritance( Type ), Node ), 'Only Node subtypes should compose Voicing' );
 
-  const InteractiveHighlightingClass = InteractiveHighlighting( Type, optionsArgPosition );
-
   // Unfortunately, nothing can be private or protected in this class, see https://github.com/phetsims/scenery/issues/1340#issuecomment-1020692592
-  const VoicingClass = class extends InteractiveHighlightingClass {
+  const VoicingClass = class extends InteractiveHighlighting( Type, optionsArgPosition ) {
 
     // ResponsePacket that holds all the supported responses to be Voiced
     _voicingResponsePacket!: ResponsePacket;
@@ -242,7 +238,7 @@ const Voicing = <SuperType extends Constructor>( Type: SuperType, optionsArgPosi
         utterance: null
       }, providedOptions );
 
-      // TODO: remove eslint-diable-line when AlertableDef is in a proper typescript file, https://github.com/phetsims/scenery/issues/1340
+      // TODO: Why is this causing a lint error? AlertableDef is in phet-types. https://github.com/phetsims/scenery/issues/1340
       let response: AlertableDef = responseCollector.collectResponses( options ); // eslint-disable-line no-undef
 
       if ( options.utterance ) {
@@ -256,7 +252,7 @@ const Voicing = <SuperType extends Constructor>( Type: SuperType, optionsArgPosi
      * Use the provided function to create content to speak in response to input. The content is then added to the
      * back of the voicing UtteranceQueue.
      * @protected
-     * TODO: remove eslint-diable-line when AlertableDef is in a proper typescript file, https://github.com/phetsims/scenery/issues/1340
+     * TODO: Why is this causing a lint error? AlertableDef is in phet-types. https://github.com/phetsims/scenery/issues/1340
      */
     speakContent( content: AlertableDef | null ): void { // eslint-disable-line no-undef
 
@@ -480,3 +476,4 @@ Voicing.VOICING_OPTION_KEYS = VOICING_OPTION_KEYS;
 
 scenery.register( 'Voicing', Voicing );
 export default Voicing;
+export type { VoicingOptions };

js/imports.ts

@@ -155,6 +155,7 @@ export { default as InteractiveHighlighting } from './accessibility/voicing/Inte
 export { default as voicingManager } from './accessibility/voicing/voicingManager.js';
 export { default as voicingUtteranceQueue } from './accessibility/voicing/voicingUtteranceQueue.js';
 export { default as Voicing } from './accessibility/voicing/Voicing.js';
+export type { VoicingOptions } from './accessibility/voicing/Voicing.js';
 export { default as ReadingBlockUtterance } from './accessibility/voicing/ReadingBlockUtterance.js';
 export { default as FocusDisplayedController } from './accessibility/FocusDisplayedController.js';
 export { default as FocusManager } from './accessibility/FocusManager.js';

js/nodes/Node.ts

@@ -6147,7 +6147,6 @@ interface Node { // eslint-disable-line
   drawableMarkFlags: string[]
 }
 
-// TODO: I think tandem needs to go last, how do we account for mixins/suptypes that add mutator keys, should they go at the end but then move tandem to the end again? https://github.com/phetsims/scenery/issues/1340
 Node.prototype._mutatorKeys = ACCESSIBILITY_OPTION_KEYS.concat( NODE_OPTION_KEYS );
 
 /**