Remove the mutate from Voicing and similar traits

[jessegreenberg]

It results in mutating everything twice and shouldn't be the general pattern. Instead, we should rely on mutate to be called after initializing the voicing trait in the constructor of whatever type is composing. For example, see this in ComboBox

Index: js/ComboBoxListItemNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/ComboBoxListItemNode.js b/js/ComboBoxListItemNode.js
--- a/js/ComboBoxListItemNode.js	(revision 6b6f489064a8981b42bc601078f20b440d94ff9f)
+++ b/js/ComboBoxListItemNode.js	(date 1635534217744)
@@ -10,6 +10,7 @@
 
 import Shape from '../../kite/js/Shape.js';
 import merge from '../../phet-core/js/merge.js';
+import Voicing from '../../scenery/js/accessibility/voicing/Voicing.js';
 import IndexedNodeIO from '../../scenery/js/nodes/IndexedNodeIO.js';
 import Node from '../../scenery/js/nodes/Node.js';
 import Rectangle from '../../scenery/js/nodes/Rectangle.js';
@@ -24,6 +25,8 @@
    * @param {number} highlightWidth
    * @param {number} highlightHeight
    * @param {Object} [options]
+   *
+   * @mixes {Voicing}
    */
   constructor( item, highlightWidth, highlightHeight, options ) {
 
@@ -59,6 +62,7 @@
     // pdom: get innerContent from the item
     assert && assert( options.innerContent === undefined, 'ComboBoxListItemNode sets innerContent' );
     options.innerContent = item.a11yLabel;
+    options.voicingNameResponse = item.a11yLabel;
 
     // Highlight that is shown when the pointer is over this item. This is not the a11y focus rectangle.
     const highlightRectangle = new Rectangle( 0, 0, highlightWidth, highlightHeight, {
@@ -92,7 +96,10 @@
     assert && assert( !options.children, 'ComboBoxListItemNode sets children' );
     options.children = [ highlightRectangle, itemNodeWrapper ];
 
-    super( options );
+    super();
+
+    // voicing - initialize the Voicing trait
+    this.initializeVoicing();
 
     // @public (read-only)
     this.item = item;
@@ -106,8 +113,13 @@
       enter() { highlightRectangle.fill = options.highlightFill; },
       exit() { highlightRectangle.fill = null; }
     } );
+
+    // mutate after initializing the Voicing trait
+    this.mutate( options );
   }
 }
+
+Voicing.compose( ComboBoxListItemNode );
 
 sun.register( 'ComboBoxListItemNode', ComboBoxListItemNode );
 export default ComboBoxListItemNode;
\ No newline at end of file

[jessegreenberg]

I found the issue where we added support for options to initializeVoicing #1206. Looking again I don't see how this "mutates everything twice" like I said originally in #1316 (comment). initializeVoicing only mutates with VOICING_OPTION_KEYS.

I think we discussed a concern that calling super( options ) before calling initializeVoicing would call Voicing.js setters before Voicing.js was initialized. But that is possible even if we remove the ability to set options in initializeVoicing.

I think what we really need to do is say that anything that mixes Voicing.js cannot call super with an options arg and that it must use mutate( options ) instead. I am not sure how to enforce that. The alternative is to make sure that Voicing.js setters gracefully support being called before Voicing.initializeVoicing which is currently the case.

Lets discuss again before proceeding with changes.

[jessegreenberg]

Discussed with @zepumph - The description in #1316 (comment) is correct. But we also want to avoid calling mutate more than we need to as well.

We will continue with this issue and add documentation to Voicing.js to describe that options cannot be passed to the super call if you intend to call initializeVoicing. super and mutate must be called after mixing everything, that is just our pattern. Trying to call functions that have not been declared yet would never fly in another language, and for good reason.

[zepumph]

I just ran into this while working on #1283, so I'm going to go ahead and handle it as part of that work.

[zepumph]

@jessegreenberg, I'm pretty sure that I converted to this pattern and refactored correctly, but will you make sure. I think the most important file to review is Voicing.js, and to make sure there isn't any more documentation to add about how to use the type. Again noting here that Paintable, and other mixins that add mutator keys have the same constraint, so this is an established pattern in scenery.

[jessegreenberg]

These commits make sense to me. I scanned the project for other places where voicing options were used in a super call but could not find any that did not follow the pattern described here.

Again noting here that Paintable, and other mixins that add mutator keys have the same constraint, so this is an established pattern in scenery.

This is my understanding as well.

Thanks for doing this!