Voicing reads out wrong voice as you switch

[KatieWoe]

Test device
Dell
Operating System
Win 11
Browser
Firefox
Problem description
For phetsims/qa#717.
When switching between the voicing options, the voice that is read out seems to be the one you were on previously, not the new one you switched to.

Visuals

Image.from.iOS.MOV

Troubleshooting information:

!!!!! DO NOT EDIT !!!!!
Name: ‪Friction‬
URL: https://phet-dev.colorado.edu/html/friction/1.6.0-dev.19/phet/friction_all_phet.html
Version: 1.6.0-dev.19 2021-10-08 22:59:15 UTC
Features missing: touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:93.0) Gecko/20100101 Firefox/93.0
Language: en-US
Window: 1280x667
Pixel Ratio: 1.5/1
WebGL: WebGL 1.0
GLSL: WebGL GLSL ES 1.0
Vendor: Mozilla (ANGLE (Intel(R) HD Graphics Direct3D11 vs_5_0 ps_5_0))
Vertex: attribs: 16 varying: 30 uniform: 4096
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

[zepumph]

This bug is a result of listener order trouble and because the focus event comes before the Property is changed:

Note that the focus event will come first (thus the object response for the button)
https://github.com/phetsims/sun/blob/8b4861fd80122b2026b92815763b65222a74c96b/js/ComboBoxListBox.js#L70-L75

Then the Property is changed, and allows the listener to fire to change the object response to the new value:
https://github.com/phetsims/joist/blob/66006a5c4b497c19bd1180bb79082664a4b010d8/js/preferences/VoicingPanelSection.js#L444-L455

I feel a bit like this has to do with phetsims/sun#701, where we need a hook that the item has just changed, but also the timing is all off, so we may have to spin up our own internal. I'll prototype this, but tagging @jessegreenberg so he is aware.

[zepumph]

This patch is a potential solution build into ComboBox, but it is a one-off just for voicing.

Basically the main issue is the the focus changes before the Property changes. If we can get rid of that need, and focus after the Property has changed, then we can still handle this from outside of ComboBox, but otherwise, I don't see a large value in creating a general hook that is added before the Property has changed.

Here is the doc that briefly discussed why we focus before the Property change. @jessegreenberg, do we have to do this? Do you remember why it is like this?

https://github.com/phetsims/sun/blob/8b4861fd80122b2026b92815763b65222a74c96b/js/ComboBoxListBox.js#L71-L72

Index: sun/js/ComboBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/ComboBox.js b/sun/js/ComboBox.js
--- a/sun/js/ComboBox.js	(revision 6b6f489064a8981b42bc601078f20b440d94ff9f)
+++ b/sun/js/ComboBox.js	(date 1635371808257)
@@ -106,6 +106,9 @@
       openedSoundPlayer: generalOpenSoundPlayer,
       closedNoChangeSoundPlayer: generalCloseSoundPlayer,
 
+      // voicing
+      getOnSelectionObjectResponse: null,
+
       // pdom
       tagName: 'div', // must have accessible content to support behavior functions
       accessibleNameBehavior: ACCESSIBLE_NAME_BEHAVIOR,
@@ -131,6 +134,7 @@
 
     this.items = items; // @private
     this.listPosition = options.listPosition; // @private
+    this.getOnSelectionObjectResponse = options.getOnSelectionObjectResponse // @private
 
     // optional label
     if ( options.labelNode !== null ) {
@@ -169,6 +173,7 @@
     this.listBox = new ComboBoxListBox( property, items,
       this.hideListBox.bind( this ), // callback to hide the list box
       this.button.focus.bind( this.button ), // callback to transfer focus to button
+      this.updateVoicingCallback.bind( this ),
       options.tandem.createTandem( 'listBox' ), {
         align: options.align,
         highlightFill: options.highlightFill,
@@ -384,6 +389,16 @@
   isItemVisible( value ) {
     return this.listBox.isItemVisible( value );
   }
+
+  updateVoicingCallback( newItem ) {
+
+    // the voice can be null
+    if ( this.getOnSelectionObjectResponse ) {
+      this.button.voicingObjectResponse = this.getOnSelectionObjectResponse( _.find(
+        this.items, item => item.value === newItem.value
+      ) );
+    }
+  }
 }
 
 ComboBox.ComboBoxIO = new IOType( 'ComboBoxIO', {
Index: joist/js/preferences/VoicingPanelSection.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/preferences/VoicingPanelSection.js b/joist/js/preferences/VoicingPanelSection.js
--- a/joist/js/preferences/VoicingPanelSection.js	(revision e15457bffc9c5ed8f8a5d8380e9e4f65ebd3ec82)
+++ b/joist/js/preferences/VoicingPanelSection.js	(date 1635371808252)
@@ -434,6 +434,14 @@
       listPosition: 'above',
       accessibleName: voiceLabelString,
 
+      // voicing
+      getOnSelectionObjectResponse: newItem => {
+        if ( newItem && newItem.value ) {
+          return newItem.value.name;
+        }
+        return null;
+      },
+
       // phet-io
       tandem: Tandem.OPT_OUT
     } );
@@ -441,30 +449,6 @@
     // voicing -  responses for the button should always come through, regardless of user selection of
     // responses
     this.button.voicingIgnoreVoicingManagerProperties = true;
-
-    // NOTE: this kind of thing should be moved to ComboBox.js
-    const voicePropertyListener = voice => {
-
-      // the voice can be null
-      if ( voice ) {
-        this.button.voicingObjectResponse = _.find(
-          items, item => item.value === voicingManager.voiceProperty.value
-        ).value.name;
-      }
-    };
-    voicingManager.voiceProperty.link( voicePropertyListener );
-
-    this.disposeVoiceComboBox = () => {
-      voicingManager.voiceProperty.unlink( voicePropertyListener );
-    };
-  }
-
-  /**
-   * @public
-   */
-  dispose() {
-    this.disposeVoiceComboBox();
-    super.dispose();
   }
 }
 
Index: sun/js/ComboBoxListBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/ComboBoxListBox.js b/sun/js/ComboBoxListBox.js
--- a/sun/js/ComboBoxListBox.js	(revision 6b6f489064a8981b42bc601078f20b440d94ff9f)
+++ b/sun/js/ComboBoxListBox.js	(date 1635371909729)
@@ -30,7 +30,7 @@
    * @param {Tandem} tandem
    * @param {Object} [options]
    */
-  constructor( property, items, hideListBoxCallback, focusButtonCallback, tandem, options ) {
+  constructor( property, items, hideListBoxCallback, focusButtonCallback, updateVoicingCallback, tandem, options ) {
 
     options = merge( {
 
@@ -68,6 +68,9 @@
       const listItemNode = event.currentTarget;
       assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' );
 
+      // Needs to go before the focus callback, so we can't listen to the Propety to update voicing
+      updateVoicingCallback( listItemNode.item );
+
       // So that something related to the ComboBox has focus before changing Property value.
       focusButtonCallback();
 

[zepumph]

@jessegreenberg and I worked on this, and fixed it in master. @KatieWoe can you please test on master.

[KatieWoe]

On Firefox, the whole sim freezes when you turn on voicing

[jessegreenberg]

Thanks @KatieWoe, looks like an unrelated issue with Voicing in Firefox after phetsims/scenery#1288

[jessegreenberg]

This should be fixed now. @KatieWoe can you please try again to see if this is fixed?

[KatieWoe]

Looks fixed on master

[zepumph]

Thanks!

[Nancy-Salpepi]

I am seeing this issue again in phetsims/qa#791 with iOS 15.4 + Safari.

Ignore my big hand in this video!

voices.mov

[jessegreenberg]

Thanks @Nancy-Salpepi - what you are showing looks a bit different than the original post. The original issue was related to the wrong voice right after making a ComboBox selection but this appears to be the entirely wrong voice for all subsequent interactions. Moving this comment to utterance-queue.