UtteranceQueue should ask if announcer is ready for next utterance

[zepumph]

From phetsims/scenery#1288, @jessegreenberg and I don't like that there is duplicated code with a queue in utterancequeue and also in voicingManager. These can be combined. The way to do this is just to ask the announcer before announcing. AriaLiveAnnouncer will always say yes, but voicingManager will want to wait until the current element in the synth is done.

We came about this from phetsims/scenery#1300, because that bug will be substantially easier to manage after we have done this refactor.

Scrap notes:

Index: scenery/js/accessibility/voicing/voicingManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/voicing/voicingManager.js b/scenery/js/accessibility/voicing/voicingManager.js
--- a/scenery/js/accessibility/voicing/voicingManager.js	(revision d6e1062e5d866b9463a8510aeaa93e79b76be9a6)
+++ b/scenery/js/accessibility/voicing/voicingManager.js	(date 1634745067246)
@@ -269,7 +269,11 @@
    * @private
    */
   onSpeechSynthesisUtteranceEnd() {
-    this.alertNow();
+    this.flag = true // don't say NOW, but let utterance-queue know you're ready
+  }
+
+  amIReady(){
+    return this.flag;
   }
 
   /**
Index: utterance-queue/js/UtteranceQueue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/utterance-queue/js/UtteranceQueue.js b/utterance-queue/js/UtteranceQueue.js
--- a/utterance-queue/js/UtteranceQueue.js	(revision e4376c1507a2d12406cd61b3d702bff53cc99fdf)
+++ b/utterance-queue/js/UtteranceQueue.js	(date 1634744825474)
@@ -245,7 +245,7 @@
       if ( utteranceWrapper.stableTime > utteranceWrapper.utterance.alertStableDelay ||
            utteranceWrapper.timeInQueue > utteranceWrapper.utterance.alertMaximumDelay ) {
         nextUtterance = utteranceWrapper.utterance;
-        this.queue.splice( i, 1 );
+        // this.queue.splice( i, 1 );
 
         break;
       }
@@ -388,10 +388,17 @@
    */
   attemptToAnnounce( utterance ) {
 
-    // only speak the utterance if not muted and the Utterance predicate returns true
-    if ( !this._muted && utterance.predicate() && utterance.getAlertText( this.announcer.respectResponseCollectorProperties ) !== '' ) {
-      this.announcer.announce( utterance, utterance.announcerOptions );
+    if ( this.announcer.amIReady() ) {
+
+      // only speak the utterance if not muted and the Utterance predicate returns true
+      if ( !this._muted && utterance.predicate() && utterance.getAlertText( this.announcer.respectResponseCollectorProperties ) !== '' ) {
+        this.announcer.announce( utterance, utterance.announcerOptions );
+      }
+
+      this.queue.splice( this.queue.indexOf( utterance ), 1 );
+
     }
+
   }
 
   /**

[zepumph]

We found that there is a sarafi workaround built into the async queue in voicingManager at this time, bug report in phetsims/john-travoltage#435. @jessegreenberg and I think we can fix this by setting the announcer.isReady flag to false for some time after a cancel() call.

[zepumph]

@jessegreenberg, please let me know if I can assist here more, and bring this up at our Friday meeting if you'd like.

[jessegreenberg]

This was done in the above commits and voicingManager no longer has its own queue. The original plan in #37 (comment) worked very well.

The hardest part of this change was giving voicingManager (an Announcer) full control over the UtteranceQueue queue, which was necessary because voicingManager had full control over its queue to add/remove/prioritize/clear.

Summarizing the changes while they are fresh:
UtteranceQueue/Announcer:

• readyToSpeak was added to Announcer. utteranceQueue checks this before moving to the next item in the queue.
• clearEmitter was added to Announcer. This is emitted from the Announcer and the utteranceQueue clears itself in response to this. This replaces the voicingManager clearing its internal queue.
• prioritizeUtterances is an abstract method that was added to Announcer. This is called by utteranceQueue on addToBack. It takes the added utterance and the utteranceQueue's queue. It uses the original priority system of voicingManager to organize the utterances and modify the queue. This is probably the weirdest part of this change, but it allows the voicingManager to prioritize the utteranceQueue instead of its internal queue.
• step is an abstract method that was added to Announcer. Similar to the above point, it lets us do the timing portions that are specific to the Announcer but depend on the queue of the UtteranceQueue.

voicingManager:

• VoicingQueueElement inner class timing variables are replaced by timeSinceUtteranceEnd in step.
• removeFromQueue now modifies the actual queue of the UtteranceQueue. As mentioned above we have access to the queue from prioritizeUtterances.
• alertNow now uses speech synthesis synchronously, without waiting for minTimeInQueue.
• onSpeechSynthesisUtteranceEnd was removed. Instead of using the SpeechSynthesisUtterance end event to trigger the next speech, we use the step function to set poll and set the new readyToSpeak property when it is time. I am OK with this because the end event has historically been inconsistent on some platforms anyway.

I tested several sims with Voicing, including Friction which uses the voicingManager priority system a lot, comparing them to versions before the change and they sounded the same. I also did some dedicated testing queing up Utterances with the console using combinations of cancelSelf, cancelOther, and priority and everything was as I expect. FInally, unit tests in utterance-queue and scenery are passing.

@zepumph can we review this list of changes together tomorrow during meeting?

[zepumph]

• readyToSpeak was added to Announcer. utteranceQueue checks this before moving to the next item in the queue.

announceImmediately will completely lose an utterance if the Announcer is not ready to speak. Do we want different behavior for that function instead of a Noop?

Looking forward to more conversation.

[jessegreenberg]

One thing we talked about together today was that if we are using polling more anyway to decide when readyToSpeak can be set to true, why not use the step function entirely to replace start and end event listeners on the SpeechSynthesisUtterance. These events are buggy on multiple platforms and we have workarounds in place to catch when they fail. We could go all in on the step function and have a more consistent and simple implementation.

If we do this we could get rid of VOICING_UTTERANCE_INTERVAL, we can get rid of safariWorkaroundUtterancePairs, because those are only needed to get the start and end events working better.

[jessegreenberg]

As for #37 (comment), we decided to implement a behavior where attemptToAnnounce will add an Utterance to the back of the queue if the Announcer is not ready. attemptToAnnounce does not prioritize the Utterances with the Announcer or collapse the same Utterances, but Utterances added in this way are read in last-in-first-out order. We did some testing of this function by adding Utterances to the queue in varying combinations and everything came out as we expect.

  const utterance1 = new phet.utteranceQueue.Utterance( {
    alert: 'This is the first utterance to speak',
    announcerOptions: {
      cancelOther: false
    }
  } );
  const utterance2 = new phet.utteranceQueue.Utterance( {
    alert: 'This is the second utterance to speak',
    announcerOptions: {
      cancelOther: false
    }
  } );

  phet.scenery.voicingUtteranceQueue.addToBack( utterance1 );
  phet.scenery.voicingUtteranceQueue.announceImmediately( utterance2 );

[jessegreenberg]

Some more things we discussed:

• The improvement discussed in UtteranceQueue should ask if announcer is ready for next utterance #37 (comment)
• Another thing @zepumph noted was that it is strange that the cancelEmitter allows bi-directional communication between Announcer and UtteranceQueue. Can we replace the cancelEmitter with calls to cancel the UtteranceQueue instead? It seems very possible if the UtteranceQueue can also cancel anything being spoken by the Announcer. Ill give this a try.
• alertNow can be inlined into requestSpeech.
• Do we still need cancelSelf and cancelOther? Not necessarily an action item here, but something to keep in mind.

[jessegreenberg]

I investigated what it would look like to replace the cancelEmitter with cancelling the UtteranceQueue instead, and the following patch is what I came up with. I added an abstract cancel to Announcer which cancels the synth in voicingManager. This way utteranceQueue.clear() also cancels any ongoing speech. The hardest thing to replace is the boundHandleCanSpeakChange in voicingManager.initialize, and the best I could come up with are the changes shown in audioManager.

Index: joist/js/preferences/PreferencesManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/preferences/PreferencesManager.js b/joist/js/preferences/PreferencesManager.js
--- a/joist/js/preferences/PreferencesManager.js	(revision 737f4633156b955b6c4313f203e9ecf34ced6b1d)
+++ b/joist/js/preferences/PreferencesManager.js	(date 1636756218796)
@@ -6,10 +6,12 @@
  * @author Jesse Greenberg
  */
 
+import globalKeyStateTracker from '../../../scenery/js/accessibility/globalKeyStateTracker.js';
+import KeyboardUtils from '../../../scenery/js/accessibility/KeyboardUtils.js';
 import voicingManager from '../../../scenery/js/accessibility/voicing/voicingManager.js';
 import voicingUtteranceQueue from '../../../scenery/js/accessibility/voicing/voicingUtteranceQueue.js';
 import responseCollector from '../../../utterance-queue/js/responseCollector.js';
-import joistVoicingUtteranceQueue from '../../../utterance-queue/js/UtteranceQueue.js';
+import joistVoicingUtteranceQueue from '../joistVoicingUtteranceQueue.js';
 import joist from '../joist.js';
 import PreferencesProperties from './PreferencesProperties.js';
 import PreferencesStorage from './PreferencesStorage.js';
@@ -43,6 +45,15 @@
         joistVoicingUtteranceQueue.enabled = enabled;
       } );
 
+      // The control key will stop Utterances in the UtteranceQueues from speaking if there is an active utterance.
+      // This key was decided because most major screen readers will stop speech when this key is pressed.
+      globalKeyStateTracker.keyupEmitter.addListener( domEvent => {
+        if ( KeyboardUtils.isControlKey( domEvent ) ) {
+          voicingUtteranceQueue.clear();
+          joistVoicingUtteranceQueue.clear();
+        }
+      } );
+
       // Register these to be stored when PreferencesStorage is enabled. TODO: likely to be moved to a better spot, see https://github.com/phetsims/joist/issues/737
       PreferencesStorage.register( responseCollector.objectResponsesEnabledProperty, 'objectResponsesEnabledProperty' );
       PreferencesStorage.register( responseCollector.contextResponsesEnabledProperty, 'contextResponsesEnabledProperty' );
Index: utterance-queue/js/UtteranceQueue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/utterance-queue/js/UtteranceQueue.js b/utterance-queue/js/UtteranceQueue.js
--- a/utterance-queue/js/UtteranceQueue.js	(revision 08dcf1886afe6a7137d26bf412bc480abb8840a1)
+++ b/utterance-queue/js/UtteranceQueue.js	(date 1636755566055)
@@ -87,12 +87,6 @@
 
       // begin stepping the queue
       stepTimer.addListener( this.stepQueueListener );
-
-      // @private {function}
-      this.clearListener = this.clear.bind( this );
-
-      // if our announcer indicates that it is cancelling, clear the queue
-      this.announcer.clearEmitter.addListener( this.clearListener );
     }
   }
 
@@ -284,12 +278,14 @@
 
   /**
    * Clear the utteranceQueue of all Utterances, any Utterances remaining in the queue will
-   * not be announced by the screen reader.
+   * not be announced by the screen reader. Also calls on the Announcer to clear as well, in case
+   * it has any internal work to do (such as interrupting speech).
    *
    * @public
    */
   clear() {
     this.queue = [];
+    this.announcer.clear();
   }
 
   /**
@@ -448,7 +444,6 @@
     // only remove listeners if they were added in initialize
     if ( this._initialized ) {
       stepTimer.removeListener( this.stepQueueListener );
-      this.announcer.clearEmitter.removeListener( this.clearListener );
     }
 
     super.dispose();
Index: joist/js/audioManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/audioManager.js b/joist/js/audioManager.js
--- a/joist/js/audioManager.js	(revision 737f4633156b955b6c4313f203e9ecf34ced6b1d)
+++ b/joist/js/audioManager.js	(date 1636757068211)
@@ -21,11 +21,14 @@
 
 import BooleanProperty from '../../axon/js/BooleanProperty.js';
 import DerivedProperty from '../../axon/js/DerivedProperty.js';
+import Property from '../../axon/js/Property.js';
 import voicingManager from '../../scenery/js/accessibility/voicing/voicingManager.js';
+import voicingUtteranceQueue from '../../scenery/js/accessibility/voicing/voicingUtteranceQueue.js';
 import soundManager from '../../tambo/js/soundManager.js';
 import PhetioObject from '../../tandem/js/PhetioObject.js';
 import Tandem from '../../tandem/js/Tandem.js';
 import joist from './joist.js';
+import joistVoicingUtteranceQueue from './joistVoicingUtteranceQueue.js';
 
 class AudioManager extends PhetioObject {
 
@@ -118,19 +121,31 @@
     }
 
     if ( this.supportsVoicing ) {
-      voicingManager.initialize( {
-
-        // specify the Properties that control whether or not output is allowed from voicingManager
-        speechAllowedProperty: new DerivedProperty( [
-          sim.isConstructionCompleteProperty,
-          sim.browserTabVisibleProperty,
-          sim.activeProperty,
-          sim.isSettingPhetioStateProperty,
-          this.audioEnabledProperty
-        ], ( simConstructionComplete, simVisible, simActive, simSettingPhetioState, audioEnabled ) => {
-          return simConstructionComplete && simVisible && simActive && !simSettingPhetioState && audioEnabled;
-        } )
+      const speechAllowedProperty = new DerivedProperty( [
+        sim.isConstructionCompleteProperty,
+        sim.browserTabVisibleProperty,
+        sim.activeProperty,
+        sim.isSettingPhetioStateProperty,
+        this.audioEnabledProperty
+      ], ( simConstructionComplete, simVisible, simActive, simSettingPhetioState, audioEnabled ) => {
+        return simConstructionComplete && simVisible && simActive && !simSettingPhetioState && audioEnabled;
       } );
+
+      voicingManager.initialize( {
+
+        // specify the Properties that control whether or not output is allowed from voicingManager
+        speechAllowedProperty: speechAllowedProperty
+      } );
+
+      // whenever speech is no longer allowed, clear all UtteranceQueues,which will also immediately
+      // cancel any speech coming out of the voicingManager
+      Property.multilink( [ speechAllowedProperty, voicingManager.enabledProperty ],
+        ( speechAllowed, voicingEnabled ) => {
+          if ( !speechAllowed && !voicingEnabled ) {
+            voicingUtteranceQueue.clear();
+            joistVoicingUtteranceQueue.clear();
+          }
+        } );
     }
   }
 }
Index: scenery/js/accessibility/voicing/voicingManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/voicing/voicingManager.js b/scenery/js/accessibility/voicing/voicingManager.js
--- a/scenery/js/accessibility/voicing/voicingManager.js	(revision 39493853e568ce02ced217107b946757f5e51281)
+++ b/scenery/js/accessibility/voicing/voicingManager.js	(date 1636757209549)
@@ -21,8 +21,6 @@
 import Announcer from '../../../../utterance-queue/js/Announcer.js';
 import Utterance from '../../../../utterance-queue/js/Utterance.js';
 import scenery from '../../scenery.js';
-import globalKeyStateTracker from '../globalKeyStateTracker.js';
-import KeyboardUtils from '../KeyboardUtils.js';
 
 const DEFAULT_PRIORITY = 1;
 
@@ -141,10 +139,6 @@
     // Null until initialized, and can be set by options to initialize().
     this._canSpeakProperty = null;
 
-    // @private {function} - bound so we can link and unlink to this.canSpeakProperty when the voicingManager becomes
-    // initialized.
-    this.boundHandleCanSpeakChange = this.handleCanSpeakChange.bind( this );
-
     // @private {Utterance|null} - A reference to the utterance currently in the synth being spoken by the browser, so
     // we can determine cancelling behavior when it is time to speak the next utterance. See voicing's supported
     // announcerOptions for details.
@@ -181,10 +175,6 @@
 
     this._synth = window.speechSynthesis;
 
-    // whether the optional Property indicating speech is allowed and the voicingManager is enabled
-    this._canSpeakProperty = DerivedProperty.and( [ options.speechAllowedProperty, this.enabledProperty ] );
-    this._canSpeakProperty.link( this.boundHandleCanSpeakChange );
-
     // Set the speechAllowedAndFullyEnabledProperty when dependency Properties update
     Property.multilink(
       [ options.speechAllowedProperty, this.voicingFullyEnabledProperty ],
@@ -202,14 +192,6 @@
     // onvoiceschanged event
     this.populateVoices();
 
-    // The control key will stop the synth from speaking if there is an active utterance. This key was decided because
-    // most major screen readers will stop speech when this key is pressed
-    globalKeyStateTracker.keyupEmitter.addListener( domEvent => {
-      if ( KeyboardUtils.isControlKey( domEvent ) ) {
-        this.cancel();
-      }
-    } );
-
     // To get Voicing to happen quickly on Chromebooks we set the counter to a value that will trigger the "engine
     // wake" interval on the next animation frame the first time we get a user gesture. See ENGINE_WAKE_INTERVAL
     // for more information about this workaround.
@@ -459,15 +441,20 @@
       // Cancel anything that is being spoken currently.
       this.cancelSynth();
 
-      // indicate to utteranceQueues that we expect everything queued for voicing to be removed
-      this.clearEmitter.emit();
-
-      // cancel clears all utterances from the utteranceQueue, so we should clear all of the safari workaround
-      // references as well
+      // We expect the utteranceQueue to be cleared as well, so we should clear all of the safari workaround
+      // references as they are no longer needed.
       this.safariWorkaroundUtterancePairs = [];
     }
   }
 
+  /**
+   * @public
+   * @override
+   */
+  clear() {
+    this.cancel();
+  }
+
   /**
    * Given one utterance, should it cancel another provided utterance?
    * @param {Utterance} utterance
Index: joist/js/toolbar/VoicingToolbarItem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/toolbar/VoicingToolbarItem.js b/joist/js/toolbar/VoicingToolbarItem.js
--- a/joist/js/toolbar/VoicingToolbarItem.js	(revision 737f4633156b955b6c4313f203e9ecf34ced6b1d)
+++ b/joist/js/toolbar/VoicingToolbarItem.js	(date 1636756394562)
@@ -208,7 +208,7 @@
       joistVoicingUtteranceQueue.addToBack( this.utterance );
     }
     else {
-      voicingManager.cancel();
+      joistVoicingUtteranceQueue.clear();
     }
   }
 }
Index: utterance-queue/js/Announcer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/utterance-queue/js/Announcer.js b/utterance-queue/js/Announcer.js
--- a/utterance-queue/js/Announcer.js	(revision 08dcf1886afe6a7137d26bf412bc480abb8840a1)
+++ b/utterance-queue/js/Announcer.js	(date 1636755579235)
@@ -6,7 +6,6 @@
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
 
-import Emitter from '../../axon/js/Emitter.js';
 import merge from '../../phet-core/js/merge.js';
 import utteranceQueueNamespace from './utteranceQueueNamespace.js';
 
@@ -24,9 +23,6 @@
     // @public {boolean} - A flag that indicates to an UtteranceQueue that this announcer is ready to speak the next
     // utterance.
     this.readyToSpeak = true;
-
-    // @public {Emitter} - Signify that this announcer expects UtteranceQueues to clear.
-    this.clearEmitter = new Emitter();
   }
 
   /**
@@ -59,6 +55,13 @@
    * @param {UtteranceWrapper[]} queue
    */
   step( dt, queue ) {}
+
+  /**
+   * Intended to be overridden by subtypes if necessary as a way to implement Announcer specific
+   * behavior when the UtteranceQueue clears.
+   * @public
+   */
+  clear() {}
 }
 
 utteranceQueueNamespace.register( 'Announcer', Announcer );

I think clearEmitter is more consolidated personally, but I also don't feel too strongly, we can review this together next we discuss.

[zepumph]

Up to you. I appreciate the investigation and totally trust whatever direction you would like to take this.

[jessegreenberg]

The improvement discussed in UtteranceQueue should ask if announcer is ready for next utterance #37 (comment)

Made progress on this that I will need to return to. It is working well but I am hitting an assertion in requestSpeech for "We should never request speech while we are already speaking" and need to figure out why.

Index: js/accessibility/voicing/voicingManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/voicing/voicingManager.js b/js/accessibility/voicing/voicingManager.js
--- a/js/accessibility/voicing/voicingManager.js	(revision b6eaefd906d141868799f57b4098938b1cb41436)
+++ b/js/accessibility/voicing/voicingManager.js	(date 1636762653334)
@@ -141,14 +141,20 @@
     // Null until initialized, and can be set by options to initialize().
     this._canSpeakProperty = null;
 
+    // @private {boolean} - Whether or not the synth is currently speaking, updated in step. SpeechSynthesisUtterances
+    // have `start` and `end` event listeners when speech starts/stops. But they do not fire consistently on all
+    // platforms. To improve consistency we determine ourselves when speech has started/stopped by checking
+    // for changes in SpeechSynthesis.speaking every time step.
+    this.synthCurrentlySpeaking = false;
+
     // @private {function} - bound so we can link and unlink to this.canSpeakProperty when the voicingManager becomes
     // initialized.
     this.boundHandleCanSpeakChange = this.handleCanSpeakChange.bind( this );
 
-    // @private {Utterance|null} - A reference to the utterance currently in the synth being spoken by the browser, so
-    // we can determine cancelling behavior when it is time to speak the next utterance. See voicing's supported
-    // announcerOptions for details.
-    this.currentlySpeakingUtterance = null;
+    // @private {VoicingUtteranceWrapper|null} - A reference to the utterance currently in the synth being spoken by
+    // the browser, so we can determine cancelling behavior when it is time to speak the next utterance. See
+    // voicing's supported announcerOptions for details.
+    this.currentlySpeakingUtteranceWrapper = null;
 
     // fixes a bug on Safari where the `start` and `end` Utterances don't fire! The
     // issue is (apparently) that Safari internally clears the reference to the
@@ -159,7 +165,7 @@
     // clearing this, though it is a bit tricky since we don't have a way to know
     // when we are done with an utterance - see #215
     // Blown away regularly, don't keep a reference to it.
-    this.safariWorkaroundUtterancePairs = [];
+    this.safariWorkaroundUtteranceWrappers = [];
   }
 
   /**
@@ -248,10 +254,38 @@
   step( dt, queue ) {
 
     if ( this.initialized ) {
+      const synthSpeaking = this.getSynth().speaking;
+
+      // We may not have a reference to the utterance if synth.speak was called outside of request speech. This
+      // is true in the case of platform specific workarounds, see ENGINE_WAKE_INTERVAL for example.
+      if ( this.currentlySpeakingUtteranceWrapper ) {
+        if ( synthSpeaking && !this.synthCurrentlySpeaking ) {
+          console.log( 'speaking' );
+
+          // We just transitioned to speaking
+          this.startSpeakingEmitter.emit( this.currentlySpeakingUtteranceWrapper.stringToSpeak, this.currentlySpeakingUtteranceWrapper.utterance );
+        }
+        else if ( !synthSpeaking && this.synthCurrentlySpeaking ) {
+          console.log( 'not speaking' );
+
+          // we just finished speaking, broadcast and do any necessary cleanup
+          this.endSpeakingEmitter.emit( this.currentlySpeakingUtteranceWrapper.stringToSpeak, this.currentlySpeakingUtteranceWrapper.utterance );
+
+          // remove the reference to the SpeechSynthesisUtterance so we don't leak memory
+          const indexOfPair = this.safariWorkaroundUtteranceWrappers.indexOf( this.currentlySpeakingUtteranceWrapper );
+          if ( indexOfPair > -1 ) {
+            this.safariWorkaroundUtteranceWrappers.splice( indexOfPair, 1 );
+          }
+
+          this.currentlySpeakingUtteranceWrapper = null;
+        }
+      }
+
+      this.synthCurrentlySpeaking = synthSpeaking;
 
       // Increment the amount of time since the synth has stopped speaking the previous utterance, but don't
       // start counting up until the synth has finished speaking its current utterance.
-      this.timeSinceUtteranceEnd = this.getSynth().speaking ? 0 : this.timeSinceUtteranceEnd + dt;
+      this.timeSinceUtteranceEnd = synthSpeaking ? 0 : this.timeSinceUtteranceEnd + dt;
 
       // Wait until VOICING_UTTERANCE_INTERVAL to speak again for more consistent behavior on certain platforms,
       // see documentation for the constant for more information. By setting readyToSpeak in the step function
@@ -261,10 +295,10 @@
         this.readyToSpeak = true;
       }
 
-      // If our queue is empty and the synth isn't speaking, then clear safariWorkaroundUtterancePairs to prevent memory leak.
+      // If our queue is empty and the synth isn't speaking, then clear safariWorkaroundUtteranceWrappers to prevent memory leak.
       // This handles any uncertain cases where the "end" callback on SpeechSynthUtterance isn't called.
-      if ( !this.getSynth().speaking && queue.length === 0 && this.safariWorkaroundUtterancePairs.length > 0 ) {
-        this.safariWorkaroundUtterancePairs = [];
+      if ( !synthSpeaking && queue.length === 0 && this.safariWorkaroundUtteranceWrappers.length > 0 ) {
+        this.safariWorkaroundUtteranceWrappers = [];
       }
 
       // A workaround to keep SpeechSynthesis responsive on Chromebooks. If there is a long enough interval between
@@ -273,7 +307,7 @@
       // there is nothing to speak in the queue, requesting speech with empty content keeps the engine active.
       // See https://github.com/phetsims/gravity-force-lab-basics/issues/303.
       this.timeSinceWakingEngine += dt;
-      if ( !this.getSynth().speaking && queue.length === 0 && this.timeSinceWakingEngine > ENGINE_WAKE_INTERVAL ) {
+      if ( !synthSpeaking && queue.length === 0 && this.timeSinceWakingEngine > ENGINE_WAKE_INTERVAL ) {
         this.timeSinceWakingEngine = 0;
         this.getSynth().speak( new SpeechSynthesisUtterance( '' ) );
       }
@@ -363,6 +397,8 @@
    */
   speakIgnoringEnabled( utterance ) {
     if ( this.initialized ) {
+
+      // FOR NEXT TIME: I think this is causing problems
       this.requestSpeech( utterance );
     }
   }
@@ -375,6 +411,7 @@
    */
   requestSpeech( utterance ) {
     assert && assert( this.isSpeechSynthesisSupported(), 'trying to speak with speechSynthesis, but it is not supported on this platform' );
+    assert && assert( this.currentlySpeakingUtteranceWrapper === null, 'We should never request speech while we are already speaking!' );
 
     // embedding marks (for i18n) impact the output, strip before speaking
     const stringToSpeak = removeBrTags( stripEmbeddingMarks( utterance.getTextToAlert( this.respectResponseCollectorProperties ) ) );
@@ -384,38 +421,12 @@
     speechSynthUtterance.rate = this.voiceRateProperty.value;
 
     // keep a reference to WebSpeechUtterances in Safari, so the browser doesn't dispose of it before firing, see #215
-    const utterancePair = new UtterancePair( utterance, speechSynthUtterance );
-    this.safariWorkaroundUtterancePairs.push( utterancePair );
-
-    const startListener = () => {
-      this.startSpeakingEmitter.emit( stringToSpeak, utterance );
-      this.currentlySpeakingUtterance = utterance;
-      speechSynthUtterance.removeEventListener( 'start', startListener );
-    };
-
-    const endListener = () => {
-      this.endSpeakingEmitter.emit( stringToSpeak, utterance );
-      speechSynthUtterance.removeEventListener( 'end', endListener );
-
-      // remove the reference to the SpeechSynthesisUtterance so we don't leak memory
-      const indexOfPair = this.safariWorkaroundUtterancePairs.indexOf( utterancePair );
-      if ( indexOfPair > -1 ) {
-        this.safariWorkaroundUtterancePairs.splice( indexOfPair, 1 );
-      }
-
-      this.currentlySpeakingUtterance = null;
-    };
-
-    speechSynthUtterance.addEventListener( 'start', startListener );
-    speechSynthUtterance.addEventListener( 'end', endListener );
-
-    // In Safari the `end` listener does not fire consistently, (especially after cancel)
-    // but the error event does. In this case signify that speaking has ended.
-    speechSynthUtterance.addEventListener( 'error', endListener );
+    const utteranceWrapper = new VoicingUtteranceWrapper( utterance, speechSynthUtterance, stringToSpeak );
+    this.safariWorkaroundUtteranceWrappers.push( utteranceWrapper );
 
     // Signify to the utterance-queue that we cannot speak yet until this utterance has finished
     this.readyToSpeak = false;
-
+    this.currentlySpeakingUtteranceWrapper = utteranceWrapper;
     this.getSynth().speak( speechSynthUtterance );
 
     if ( !this.hasSpoken ) {
@@ -464,7 +475,7 @@
 
       // cancel clears all utterances from the utteranceQueue, so we should clear all of the safari workaround
       // references as well
-      this.safariWorkaroundUtterancePairs = [];
+      this.safariWorkaroundUtteranceWrappers = [];
     }
   }
 
@@ -518,15 +529,15 @@
         this.removeFromQueue( utteranceWrapper, queue );
 
         // remove from safari workaround list to avoid memory leaks, if available
-        const index = _.findIndex( this.safariWorkaroundUtterancePairs, utterancePair => utterancePair.utterance === utteranceWrapper.utterance );
+        const index = _.findIndex( this.safariWorkaroundUtteranceWrappers, utterancePair => utterancePair.utterance === utteranceWrapper.utterance );
         if ( index > -1 ) {
-          this.safariWorkaroundUtterancePairs.splice( index, 1 );
+          this.safariWorkaroundUtteranceWrappers.splice( index, 1 );
         }
       }
     }
 
-    // test against what is currently being spoken by the synth (currentlySpeakingUtterance)
-    if ( this.currentlySpeakingUtterance && this.shouldCancel( newUtterance, this.currentlySpeakingUtterance ) ) {
+    // test against what is currently being spoken by the synth (currentlySpeakingUtteranceWrapper)
+    if ( this.currentlySpeakingUtteranceWrapper && this.shouldCancel( newUtterance, this.currentlySpeakingUtteranceWrapper.utterance ) ) {
       this.cancelSynth();
     }
   }
@@ -542,19 +553,21 @@
 }
 
 /**
- * An inner class that pairs a SpeechSynthesisUtterance with an Utterance. Useful for the Safari workaround
+ * An inner class that pairs a SpeechSynthesisUtterance with an Utterance, and keeps references to other useful data.
  */
-class UtterancePair {
+class VoicingUtteranceWrapper {
 
   /**
    * @param {Utterance} utterance
    * @param {SpeechSynthesisUtterance} speechSynthesisUtterance
+   * @param {string} stringToSpeak
    */
-  constructor( utterance, speechSynthesisUtterance ) {
+  constructor( utterance, speechSynthesisUtterance, stringToSpeak ) {
 
     // @public (read-only)
     this.utterance = utterance;
     this.speechSynthesisUtterance = speechSynthesisUtterance;
+    this.stringToSpeak = stringToSpeak;
   }
 }
 

[zepumph]

I am unsure if this issue is on the list, or just out of date at this point. @jessegreenberg, can you reflect on the progress of this issue? Haven't we completed it in other issues?

For example from #37 (comment), we don't have the cancelEmitter anymore.

[jessegreenberg]

I read through comments in this issue looking for loose ends.

#37 (comment)

We tried this today in phetsims/scenery#1344 but it ended up being difficult and we decided it was not worth the effort/destabilizing at this time.

For example from #37 (comment), we don't have the cancelEmitter anymore.

Indeed!

You are right, I don't think there is anything else. The remaining work for this issue happened while we worked on making priorityProperty observable (among other things). Im going to close.