Should we prevent announce from being used when setting phet-io state?

[jessegreenberg]

This came from phetsims/balloons-and-static-electricity#521, where it was suggested that we should prevent alerts from coming out of the sim while the sim changes during a phet-io state set.

This is in the "phet-io + a11y" overlap category and likely needs a decision before greenhouse-effect is published with PhET-iO + Interactive Description.

[zepumph]

Yes very nice thought! We do the same thing for sound:

https://github.com/phetsims/tambo/blob/98effa9c6a488ec3520c7884e61f493b7615d9fd/js/soundManager.js#L188

I think my ideal strategy for this is on hold by #25. If that comes through, we could use UtteranceQueue.canAlertUtterance as the spot to check this.

[zepumph]

#25 has been implemented. I'll add this now.

[zepumph]

I wasn't able to complete this right now. Here is my current patch:

Index: js/UtteranceQueue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/UtteranceQueue.js b/js/UtteranceQueue.js
--- a/js/UtteranceQueue.js	(revision a6176e8ffd4a4ba4486d9af6b24a5ea8c50a48ed)
+++ b/js/UtteranceQueue.js	(date 1628817987427)
@@ -79,6 +79,10 @@
     // whether the UtterancesQueue is alerting, and if you can add/remove utterances
     this._enabled = true;
 
+    // @private - Upon construction, phet-io is not ready to hook into, so set up this flag, and wire this up lazily upon
+    // announcing.
+    this.wiredUpPhetioCheck = false;
+
     if ( this._initialized ) {
 
       // @private {function}
@@ -391,8 +395,18 @@
    */
   attemptToAnnounce( utterance ) {
 
+    // Upon construction, there is no sim yet, so do this once here.
+    if ( !this.wiredUpPhetioCheck ) {
+      Tandem.PHET_IO_ENABLED && phet.joist.sim.isSettingPhetioStateProperty.lazyLink( settingState => {
+        !settingState && this.clear();
+        this.wiredUpPhetioCheck = true;
+      } );
+    }
+
+    const notSettingPhetioState = !( Tandem.PHET_IO_ENABLED && phet.joist.sim.isSettingPhetioStateProperty.value );
+
     // only speak the utterance if not muted and the Utterance predicate returns true
-    if ( !this._muted && utterance.predicate() ) {
+    if ( !this._muted && utterance.predicate() && notSettingPhetioState ) {
       this.announcer.announce( utterance, utterance.announcerOptions );
     }
   }

[zepumph]

I see two paths forward, and I would like to ask @samreid's opinion about it before moving forward. The first is to embed iO state checking into UtteranceQueue, but this seems a bit heavy-handed:

Index: js/UtteranceQueue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/UtteranceQueue.js b/js/UtteranceQueue.js
--- a/js/UtteranceQueue.js	(revision a6176e8ffd4a4ba4486d9af6b24a5ea8c50a48ed)
+++ b/js/UtteranceQueue.js	(date 1628867085529)
@@ -79,6 +79,10 @@
     // whether the UtterancesQueue is alerting, and if you can add/remove utterances
     this._enabled = true;
 
+    // @private - Upon construction, phet-io is not ready to hook into, so set up this flag, and wire this up lazily upon
+    // announcing.
+    this.wiredUpPhetioCheck = false;
+
     if ( this._initialized ) {
 
       // @private {function}
@@ -391,8 +395,18 @@
    */
   attemptToAnnounce( utterance ) {
 
+    // Upon construction, there is no sim yet, so do this once here.
+    if ( !this.wiredUpPhetioCheck ) {
+      Tandem.PHET_IO_ENABLED && phet.joist.sim.isSettingPhetioStateProperty.lazyLink( settingState => {
+        !settingState && this.clear();
+        this.wiredUpPhetioCheck = true;
+      } );
+    }
+
+    const notSettingPhetioState = !( Tandem.PHET_IO_ENABLED && phet.joist.sim.isSettingPhetioStateProperty.value );
+
     // only speak the utterance if not muted and the Utterance predicate returns true
-    if ( !this._muted && utterance.predicate() ) {
+    if ( !this._muted && utterance.predicate() && notSettingPhetioState ) {
       this.announcer.announce( utterance, utterance.announcerOptions );
     }
   }

The next is to have Sim.js do the work. That way we are keeping iO specific code out of the library (since it isn't needed anyways).

Index: phet-io/js/phetioEngine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phet-io/js/phetioEngine.js b/phet-io/js/phetioEngine.js
--- a/phet-io/js/phetioEngine.js	(revision adf00829e54e2d49c9255b12dbb6bc4ff9ba12cf)
+++ b/phet-io/js/phetioEngine.js	(date 1628871091733)
@@ -216,8 +216,10 @@
    * @param {SimInfo} simInfo
    * @param {Property.<boolean>}isConstructionCompleteProperty
    * @param {Emitter} frameEndedEmitter
+   * @param {UtteranceQueue} descriptionUtteranceQueue
+   * @param {UtteranceQueue} voicingUtteranceQueue
    */
-  onSimConstructionStarted( simInfo, isConstructionCompleteProperty, frameEndedEmitter ) {
+  onSimConstructionStarted( simInfo, isConstructionCompleteProperty, frameEndedEmitter, descriptionUtteranceQueue, voicingUtteranceQueue ) {
     assert && assert( !this.simConstructionBegan, 'sim cannot be constructed twice.' );
 
     // The simStarted event is guaranteed to be a top-level event, not nested under other events.
@@ -339,6 +341,11 @@
 
         // This should be set after phetioObject callbacks for construction completed.
         this.simConstructed = true;
+
+        this.phetioStateEngine.stateSetEmitter.addListener( () => {
+          descriptionUtteranceQueue.clear();
+          voicingUtteranceQueue.clear();
+        } );
       }
     } );
     this.simConstructionBegan = true;
Index: joist/js/Sim.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/Sim.js b/joist/js/Sim.js
--- a/joist/js/Sim.js	(revision 77d575b375977f9e0a9d633d42e74ce85f5a5662)
+++ b/joist/js/Sim.js	(date 1628871091733)
@@ -32,6 +32,7 @@
 import globalKeyStateTracker from '../../scenery/js/accessibility/globalKeyStateTracker.js';
 import KeyboardFuzzer from '../../scenery/js/accessibility/KeyboardFuzzer.js';
 import KeyboardUtils from '../../scenery/js/accessibility/KeyboardUtils.js';
+import voicingUtteranceQueue from '../../scenery/js/accessibility/voicing/voicingUtteranceQueue.js';
 import Display from '../../scenery/js/display/Display.js';
 import InputFuzzer from '../../scenery/js/input/InputFuzzer.js';
 import animatedPanZoomSingleton from '../../scenery/js/listeners/animatedPanZoomSingleton.js';
@@ -749,7 +750,13 @@
     this.simInfo = new SimInfo( this );
 
     // Set up PhET-iO, must be done after phet.joist.sim is assigned
-    Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.onSimConstructionStarted( this.simInfo, this.isConstructionCompleteProperty, this.frameEndedEmitter );
+    Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.onSimConstructionStarted(
+      this.simInfo,
+      this.isConstructionCompleteProperty,
+      this.frameEndedEmitter,
+      this.display.utteranceQueue,
+      voicingUtteranceQueue
+    );
 
     // Third party support
     phet.chipper.queryParameters.legendsOfLearning && new LegendsOfLearningSupport( this ).start();

@samreid, two questions for you:

1. Which of the two general placements seems best to you. Does keeping iO out of utterance-queue appeal to you like it does to me? (I prefer patch 2 I think).
2. On a more micro scale, I think I prefer patch 2 also, as I think we only need to clear the queue after state is set. Do we need to toggle anything on/off during state setting otherwise?

[zepumph]

After discussing with @samreid on a call, we decided that the above commit made sense. There is already precedent for this with how sounds are handled. There was still an important design decision to discuss though, about what the behavior should be for these announced features. If a PhET-iO client sets a state on a sim, should the description or voicing detail that? Should it say what has happened in that case? Something like "state has been set." Or maybe we actually want it to restate the new state of the sim, however that may look.

Marking for design discussion to see if we want any response announcements to come through while setting state.

[zepumph]

Now is the time!

[zepumph]

We discussed this during PhET-iO meeting today.

We decided to treat this like a reset all button description. We want to add an option to opt out of this so that we can have silent state setting, like on startup.

Here were discussed what the utterance text should be:

Sim changed, everything reset
sim updated, the configuration has changed
sim updated, 
Sim coniguration changed
The state of the sim has changed
Sim state changed, explore to discover more
Sim State change detected, the configuration may have changed
Sim state has been changed
Sim state may have changed

"We will go with Sim state changed, explore to discover more." for now.

[zepumph]

We don't want to include the option of sound in this because we fear that it will be too intrusive in most cases. That said, we discussed super granular options like:

setState( state, {
  voicingText: 'reset',
  descriptionText: 'other reset',
  includeSound: true