"Read me" buttons should never be interrupted by sim voicing alerts

[zepumph]

@terracoda reported that it is possible to have alerts from the sim interrupt read-me buttons. @jessegreenberg and I thought that we could use priority to accomplish this since it just needs to be done for voicing.

[zepumph]

This should be done for friction.

[zepumph]

This patch works, but during the meeting @terracoda was not happy that many of the stateful alerts while interacting with the sim wouldn't interrupt the buttons. It seemed like there needed to be more conversation about that.

Index: js/toolbar/VoicingToolbarItem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/toolbar/VoicingToolbarItem.js b/js/toolbar/VoicingToolbarItem.js
--- a/js/toolbar/VoicingToolbarItem.js	(revision 96116cf7524bbec6e24e073efb0175560e570aea)
+++ b/js/toolbar/VoicingToolbarItem.js	(date 1633542883814)
@@ -147,7 +147,11 @@
     this.lookAndFeel = lookAndFeel;
 
     // @private
-    this.utterance = new Utterance();
+    this.utterance = new Utterance( {
+      announcerOptions: {
+        priority: voicingManager.TOP_PRIORITY
+      }
+    } );
 
     this.createAlert = createAlert;
 

@terracoda do you want me to commit the patch that you saw today where the read-me buttons are uninterruptable, and interrupt everything else?

[jessegreenberg]

We discussed this during meeting today. Instead of using Priority we considered having different UtteranceQueues for different contexts so that you could prioritize sections of content and when one is made inactive it would never have any Utterances even added to the back of the queue. For example, we could have one for ScreenView content, one for the PreferencesDialog and another for the Toolbar. When the PreferenseDialog is open, nothing new would be added to the back of the ScreenView UtteranceQueue. This would also prevent stale alerts from being read after the Dialog is closed.

For the toolbar we would have logic like

• If one of the "Read me" buttons are pressed, the toolbar content is spoken and cannot be interrupted by anything in the ScreenView queue. Once the button content is finished the ScreenView queue can become active again.
• If one of the "Read me" buttons are pressed, the toolbar content is spoken and cannot be interrupted by anything in the ScreenView queue. Once a sim component receives some kind of input (focus, down, other), the ScreenView queue becomes active again and can interrupt the "Read me" button content.

[jessegreenberg]

During the meeting we discussed that Friction would benefit from this a lot because it has frequent alerts that could interrupt Preferences or the Toolbar and it would be great to have this improvement in the sim for its publication with Voicing. It is currently in a dev test in phetsims/qa#717.

[zepumph]

Let's discuss and potentially prototype this during out next meeting.

[terracoda]

@jessegreenberg and @zepumph, apologies for the delay, but I want to emphasize that the user's actions should be able to interrupt their previous actions. The goal is that the sim voices where the user's attention is. I think this was clear in last week's meeting. Just in case we are not saying the same thing, I'd like to write down how I see it.

If we take the "queue-solution route" which seems like a good one, there seems to be at least 2 types of queues, a Sim queue, and a UI-like queue.

1. The sim queue is the primary queue and is tracked by the model. It often needs customization for the design of each sim. The sim queue can be interrupted (or temporarily silenced) by user action with a UI queue. But if something is happening in the model, responses would be allowed to enter the queue and be voiced once the sim queue is not being blocked by user action with a UI queue.
2. The potential UI queues I see are the Toolbar (read me buttons) and Navbar (screen buttons, dialogs, PhET menu). A UI queue is a secondary queue. It is not persistent like the sim queue. Interaction with another UI element that feeds a UI queue or interaction with the sim would interrupt and clear that UI queue. So, if a user hits the hint button before the details button content is finished, the hint button content is voiced and the details content won't voice unless the user hits the details button again.

Example:

• The user heats up friction, so responses are firing for a little while...
• The user quickly hits the details read-me button. The details button content interrupts the sim queue until the details are done, or until the user takes another action.
• If the user takes no action, the details content finishes and if the sim model is not yet in its steady cooled state, the final cooling responses would enter the sim queue and be voiced without user action. If the model is cool, there is no more voicing until the user interacts again.
• If the user presses the hint button before the details button content is finished, and the hint content replaces the details button. When the hint content is done, no details would play, but if the sim model was not yet cooled, cooling responses could enter the sim queue and be voiced (only if they are current).

I hope that is a clear example.

[jessegreenberg]

We discussed this during voicing meeting today and decided that we really want interacting with a component after a Toolbar button is pressed to interrupt the "Read me" button content. Interrupting after interaction with a sim component will be very difficult to implement. So we discussed and agreed to try an alternative where clicking anywhere in the Display or changing focus will reduce the priority of the button's Voicing content so that Voicing from the sim can resume.

[jessegreenberg]

I worked on this for a little bit this morning and it seems pretty simple, but two challenges I am running into are 1) We need to make announcerOptions (and Priority in particular) mutable. Adding a listener to the Display doesn't seem to work because Display listeners are called at the very end of Input.dispatchEvent. At this time other Utterances from input have been added to the queue and the Priority hasn't changed yet because we haven't hit the Display listener so they are just lost. We need to set the new priority before anything can be added to the back of the UtteranceQueue.

[jessegreenberg]

I was able to hack this together for demonstration purposes today, and in the Voicing meeting it was agreed that #752 (comment) was acceptable behavior.

Regarding #752 (comment) It seems like the only way to add an event listener that will be guaranteed to be called before listeners on Node targets is to add to the Pointers. I don't know how that will work well for touch. display._input (to get to input.pointers or input.pointerAddedEmitter) is private and there isn't a valid way to access it.

However, all of that may not be important. The following patch is working really well so far and works by eagerly calling prioritizeUtterances whenever we need to change the priority of an Utterance that is already in the queue.

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 592fc67551e5a6de67f67eecc7fe1768fb3343ef)
+++ b/scenery/js/accessibility/voicing/voicingManager.js	(date 1637796969626)
@@ -468,6 +468,18 @@
     }
   }
 
+  /**
+   * Cancel the provided Utterance, if it is currently being spoken by this Announcer.
+   * @public
+   *
+   * @param {Utterance} utterance
+   */
+  cancelUtterance( utterance ) {
+    if ( this.currentlySpeakingUtterance === utterance ) {
+      this.cancelSynth();
+    }
+  }
+
   /**
    * Given one utterance, should it cancel another provided utterance?
    * @param {Utterance} utterance
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 8619882985352f7ed4991167884407ec8e1e7209)
+++ b/utterance-queue/js/UtteranceQueue.js	(date 1637798357182)
@@ -151,6 +151,16 @@
     this.queue.unshift( utteranceWrapper );
   }
 
+  /**
+   * @public
+   * @param utterance
+   * @param priority
+   */
+  setUtterancePriority( utterance, priority ) {
+    utterance.announcerOptions.priority = priority;
+    this.announcer.prioritizeUtterances( utterance, this.queue );
+  }
+
   /**
    * Create an Utterance for the queue in case of string and clears the queue of duplicate utterances. This will also
    * remove duplicates in the queue, and update to the most recent timeInQueue variable.
@@ -355,6 +365,9 @@
 
     dt *= 1000; // convert to ms
 
+
+    console.log( this.queue.length );
+
     if ( this.queue.length > 0 ) {
       for ( let i = 0; i < this.queue.length; i++ ) {
         const utteranceWrapper = this.queue[ i ];
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 23e3cba86402989bac6bdf0e2b06df6228ed7964)
+++ b/joist/js/toolbar/VoicingToolbarItem.js	(date 1637799079244)
@@ -13,6 +13,7 @@
 import VoicingText from '../../../scenery/js/accessibility/voicing/nodes/VoicingText.js';
 import ReadingBlockHighlight from '../../../scenery/js/accessibility/voicing/ReadingBlockHighlight.js';
 import voicingManager from '../../../scenery/js/accessibility/voicing/voicingManager.js';
+import Display from '../../../scenery/js/display/Display.js';
 import AlignGroup from '../../../scenery/js/nodes/AlignGroup.js';
 import HBox from '../../../scenery/js/nodes/HBox.js';
 import Node from '../../../scenery/js/nodes/Node.js';
@@ -185,6 +186,21 @@
         this.playingProperty.set( false );
       }
     } );
+
+    const handlePriority = () => {
+      joistVoicingUtteranceQueue.setUtterancePriority( this.utterance, 0 );
+      Display.removeInputListener( displayListener );
+    };
+    const displayListener = {
+      down: event => { handlePriority(); },
+      focus: event => { handlePriority(); }
+    };
+
+    voicingManager.startSpeakingEmitter.addListener( ( text, endedUtterance ) => {
+      if ( endedUtterance === this.utterance ) {
+        Display.addInputListener( displayListener );
+      }
+    } );
   }
 
   /**
@@ -198,6 +214,9 @@
 
     if ( this.playingProperty.value ) {
 
+      // Immediately silence and clear the queue to speak this button's content immediately.
+      voicingManager.cancel();
+
       // when one button is pressed, immediately stop any other buttons, only one should be playing at a time
       const otherProperties = _.without( playingProperties, this.playingProperty );
       otherProperties.forEach( property => {
@@ -205,10 +224,14 @@
       } );
 
       this.utterance.alert = this.createAlert();
+      this.utterance.announcerOptions.priority = voicingManager.TOP_PRIORITY;
       joistVoicingUtteranceQueue.addToBack( this.utterance );
     }
     else {
-      voicingManager.cancel();
+
+      // Cancel this utterance if it is still speaking, don't cancel all utterances that may have been added to the
+      // queue already before this was cancelled.
+      voicingManager.cancelUtterance( this.utterance );
     }
   }
 }

A couple of things:

1. Since utteranceQueue manages the queue, we need to call prioritizeUtterances on UtteranceQueue. "Priority" is then no longer specific to the voicingManager announcer. How should we reconcile this? Should priority be moved right into all Utterances?
2. Here are some notes I wrote up trying to understand why calling prioritizeUtterances when setting Utterance priority gets things to work. I think it demonstrates why we do not need to worry about the Display listeners happening last.

//********************************************************************************
Order of events when just setting announcerOptions directly:

speak toolbar button content on click event
on voicingManager.startSpeakingEmitter, add a listener to Display that will set Priority of speaking Utterance to zero when focus event happens
receive focus event:
    dispatched to targets.
        focus event lands on "details" button
            voicing content for button is added to the back of the utteranceQueue.
                call prioritizeUtterances
                    In this case it is just added to the back of the queue since it is a lower priority
    dispatched to display
        announcer priority is set to zero in listener

utteranceQueue continues to step through utterances in the queue, speaking button content in full,
then name of focused item


//********************************************************************************
Order of events when setting priority and eagerly calling announcer.prioritize()

speak button content in display listener on click event
on start, add a listener to Display that will set Priority to zero when focus event happens
focus event:
    dispatched to targets.
        focus event lands on "details" button
            voicing content for button is added to the back of the utteranceQueue.
                call prioritizeUtterances (new utterance lower than voicing button)
                    In this case it is just added to the back of the queue since it is a lower priority
    dispatch to display
        announcer priority is set to zero in listener but we also re-prioritize the queue.

utteranceQueue will step through the utterances in the queue, but the second call to prioritizeUtterances
has removed the toolbar content since the sim content has been made higher priority. I think this is exactly what is 
intended by the priority feature but I needed to write this out to understand it for some reason.

Looking forward to coming back to this I think we are getting close to a solution.

EDIT: An updated patch

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 2ebe2b5a253778eb1653779be6e0a90f2fc51070)
+++ b/scenery/js/accessibility/voicing/voicingManager.js	(date 1638372496921)
@@ -466,6 +466,19 @@
     }
   }
 
+  /**
+   * Cancel the provided Utterance, if it is currently being spoken by this Announcer. Does not cancel
+   * any other utterances that may be in the UtteranceQueue.
+   * @public
+   *
+   * @param {Utterance} utterance
+   */
+  cancelUtterance( utterance ) {
+    if ( this.currentlySpeakingUtterance === utterance ) {
+      this.cancelSynth();
+    }
+  }
+
   /**
    * Given one utterance, should it cancel another provided utterance?
    * @param {Utterance} utterance
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 3d0f92f0f6047f4527d22cb574c3cac178f9aa98)
+++ b/utterance-queue/js/UtteranceQueue.js	(date 1638372409062)
@@ -151,6 +151,23 @@
     this.queue.unshift( utteranceWrapper );
   }
 
+  /**
+   * Set the Utterance priority. Assuming the utterance priority has changed, we need to re-order the entire
+   * queue if the priority has changed.
+   *
+   * TODO: See https://github.com/phetsims/joist/issues/752 - Priority is specific to Voicing, but we need this
+   * function in UtteranceQueue so we have access to the queue. Should we make priority a feature of all
+   * Utterances?
+   * @public
+   *
+   * @param {Utterance} utterance
+   * @param {number} priority
+   */
+  setUtterancePriority( utterance, priority ) {
+    utterance.announcerOptions.priority = priority;
+    this.announcer.prioritizeUtterances( utterance, this.queue );
+  }
+
   /**
    * Create an Utterance for the queue in case of string and clears the queue of duplicate utterances. This will also
    * remove duplicates in the queue, and update to the most recent timeInQueue variable.
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 498b3de2d485a91ca8c907249740572915f5709b)
+++ b/joist/js/toolbar/VoicingToolbarItem.js	(date 1638373429070)
@@ -10,7 +10,7 @@
 import BooleanProperty from '../../../axon/js/BooleanProperty.js';
 import PlayStopButton from '../../../scenery-phet/js/buttons/PlayStopButton.js';
 import PhetFont from '../../../scenery-phet/js/PhetFont.js';
-import { VoicingText } from '../../../scenery/js/imports.js';
+import { Display, VoicingText } from '../../../scenery/js/imports.js';
 import { ReadingBlockHighlight } from '../../../scenery/js/imports.js';
 import { voicingManager } from '../../../scenery/js/imports.js';
 import { AlignGroup } from '../../../scenery/js/imports.js';
@@ -185,6 +185,22 @@
         this.playingProperty.set( false );
       }
     } );
+
+    // handle priority by
+    const handlePriority = () => {
+      joistVoicingUtteranceQueue.setUtterancePriority( this.utterance, 0 );
+      Display.removeInputListener( displayListener );
+    };
+    const displayListener = {
+      down: event => handlePriority(),
+      focus: event => handlePriority()
+    };
+
+    voicingManager.startSpeakingEmitter.addListener( ( text, endedUtterance ) => {
+      if ( endedUtterance === this.utterance ) {
+        Display.addInputListener( displayListener );
+      }
+    } );
   }
 
   /**
@@ -198,6 +214,9 @@
 
     if ( this.playingProperty.value ) {
 
+      // Immediately silence and clear the queue to speak this button's content immediately.
+      voicingManager.cancel();
+
       // when one button is pressed, immediately stop any other buttons, only one should be playing at a time
       const otherProperties = _.without( playingProperties, this.playingProperty );
       otherProperties.forEach( property => {
@@ -205,10 +224,14 @@
       } );
 
       this.utterance.alert = this.createAlert();
+      this.utterance.announcerOptions.priority = voicingManager.TOP_PRIORITY;
       joistVoicingUtteranceQueue.addToBack( this.utterance );
     }
     else {
-      voicingManager.cancel();
+
+      // Cancel this utterance if it is still speaking, don't cancel all utterances that may have been added to the
+      // queue already before this was cancelled.
+      voicingManager.cancelUtterance( this.utterance );
     }
   }
 }

EDIT: Here is a snippet to paste into dev tools so add many alerts to the queue to watch them get deferred

window.setInterval( () => {
  phet.scenery.voicingUtteranceQueue.addToBack(
    new phet.utteranceQueue.Utterance( {
      alert: 'This is a new alert',
    } )
  )
}, 3000 )

[jessegreenberg]

@zepumph and I discussed today - We want to try making priority a feature of Utterances instead of the voicingManager Announcer. That will potentially resolve this TODO in the patch

TODO: See #752 - Priority is specific to Voicing, but we need this function in UtteranceQueue so we have access to the queue. Should we make priority a feature of all Utterances?

We also thought about making the priority be a Property of the Utterances so that it could be more easily set. It wasn't clear yet how to support that in the implementation, but it seemed like the first step mentioned above was required to make this happen. Lets start with that, then try to make priority a Property on the Utterance.

[jessegreenberg]

@zepumph found a bug with the toolbar items that should be fixed by this patch #752 (comment) - the call to voicingManager.cancel() in VoicingToolbarItem.playContent() needs to be replaced with voicingManager.cancelUtterance( utterance ), so that Utterances that are in the queue will still be heard in this case.

[jessegreenberg]

The above commits move priority into Utterance/UtteranceQueue and out of voicingManager. @zepumph and I spent a long time today working on the second part in #752 (comment), making Priority observable on Utterance and we made really good progress:

Index: utterance-queue/js/AriaLiveAnnouncer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/utterance-queue/js/AriaLiveAnnouncer.js b/utterance-queue/js/AriaLiveAnnouncer.js
--- a/utterance-queue/js/AriaLiveAnnouncer.js	(revision 228711ef351cf188ba1c8fa6b656eb75c874742f)
+++ b/utterance-queue/js/AriaLiveAnnouncer.js	(date 1639769289340)
@@ -146,6 +146,10 @@
     // Note that getTextToAlert will have side effects on the Utterance as the Utterance
     // may have have logic that changes its alert content each time it is used
     this.announcingEmitter.emit( utterance.getTextToAlert( this.respectResponseCollectorProperties ), options.ariaLivePriority, utterance );
+
+    // With aria-live we don't have information about when the screen reader is done speaking
+    // the content, so we have to emit this right away
+    this.announcementCompleteEmitter.emit( utterance );
   }
 
   /**
Index: utterance-queue/js/Utterance.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/utterance-queue/js/Utterance.js b/utterance-queue/js/Utterance.js
--- a/utterance-queue/js/Utterance.js	(revision 228711ef351cf188ba1c8fa6b656eb75c874742f)
+++ b/utterance-queue/js/Utterance.js	(date 1639761105911)
@@ -20,6 +20,7 @@
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
 
+import NumberProperty from '../../axon/js/NumberProperty.js';
 import validate from '../../axon/js/validate.js';
 import merge from '../../phet-core/js/merge.js';
 import AlertableDef from './AlertableDef.js';
@@ -121,9 +122,8 @@
     // @public (utterance-queue-internal) {Object} - Options to be passed to the announcer for this Utterance
     this.announcerOptions = options.announcerOptions;
 
-    // @public (read-only) - Temporarily read-only, we want this to be mutable.
-    // See https://github.com/phetsims/joist/issues/752
-    this.priority = options.priority;
+    // @public - observable for the Property, can be set to modify behavior in the utteranceQueue
+    this.priorityProperty = new NumberProperty( options.priority );
   }
 
   /**
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 f51b106e1402238fad0a27cb631578273928a1d8)
+++ b/scenery/js/accessibility/voicing/voicingManager.js	(date 1639775075005)
@@ -212,21 +212,6 @@
     this.initialized = true;
   }
 
-  /**
-   * Remove an element from the utterance queue.
-   * @private
-   *
-   * @param {UtteranceWrapper} utteranceWrapper
-   * @param {UtteranceWrapper[]} queue - modified by this function!
-   */
-  removeFromQueue( utteranceWrapper, queue ) {
-
-    // remove from voicingManager list after speaking
-    const index = queue.indexOf( utteranceWrapper );
-    assert && assert( index >= 0, 'trying to remove a utteranceWrapper that doesn\'t exist' );
-    queue.splice( index, 1 );
-  }
-
   /**
    * @override
    * @private
@@ -391,6 +376,8 @@
         this.safariWorkaroundUtterancePairs.splice( indexOfPair, 1 );
       }
 
+      this.announcementCompleteEmitter.emit( utterance );
+
       this.currentlySpeakingUtterance = null;
     };
 
@@ -470,8 +457,8 @@
     const utteranceOptions = merge( {}, UTTERANCE_OPTION_DEFAULTS, utterance.announcerOptions );
 
     let shouldCancel;
-    if ( utteranceToCancel.priority !== utterance.priority ) {
-      shouldCancel = utteranceToCancel.priority < utterance.priority;
+    if ( utteranceToCancel.priorityProperty.value !== utterance.priorityProperty.value ) {
+      shouldCancel = utteranceToCancel.priorityProperty.value < utterance.priorityProperty.value;
     }
     else {
       shouldCancel = utteranceOptions.cancelOther;
@@ -488,12 +475,12 @@
    * we should cancel the synth immediately.
    * @public
    *
-   * @param {Utterance} newUtterance
+   * @param {Utterance} nextAvailableUtterance
    */
-  onUtterancePriorityChange( newUtterance ) {
+  onUtterancePriorityChange( nextAvailableUtterance ) {
 
     // test against what is currently being spoken by the synth (currentlySpeakingUtterance)
-    if ( this.currentlySpeakingUtterance && this.shouldUtteranceCancelOther( newUtterance, this.currentlySpeakingUtterance ) ) {
+    if ( this.currentlySpeakingUtterance && this.shouldUtteranceCancelOther( nextAvailableUtterance, this.currentlySpeakingUtterance ) ) {
       this.cancelSynth();
     }
   }
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 228711ef351cf188ba1c8fa6b656eb75c874742f)
+++ b/utterance-queue/js/Announcer.js	(date 1639775046981)
@@ -8,6 +8,7 @@
 
 import Emitter from '../../axon/js/Emitter.js';
 import merge from '../../phet-core/js/merge.js';
+import Utterance from './Utterance.js';
 import utteranceQueueNamespace from './utteranceQueueNamespace.js';
 
 class Announcer {
@@ -25,7 +26,12 @@
     // utterance.
     this.readyToSpeak = true;
 
+    this.announcementCompleteEmitter = new Emitter( {
+      parameters: [ { valueType: Utterance } ]
+    } );
+
     // @public {Emitter} - Signify that this announcer expects UtteranceQueues to clear.
+    // TODO: Do we still need this? The announcer doesn't mutate the queue anymore, https://github.com/phetsims/joist/issues/752
     this.clearEmitter = new Emitter();
   }
 
@@ -52,7 +58,7 @@
    * @returns {boolean}
    */
   shouldUtteranceCancelOther( utterance, utteranceToCancel ) {
-    return utteranceToCancel.priority < utterance.priority;
+    return utteranceToCancel.priorityProperty.value < utterance.priorityProperty.value;
   }
 
   /**
@@ -61,7 +67,6 @@
    * @public
    *
    * @param {Utterance} utterance
-   * @param {UtteranceWrapper[]} queue - The UtteranceQueue list - can be modified by this function!
    */
   onUtterancePriorityChange( utterance ) {}
 
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 228711ef351cf188ba1c8fa6b656eb75c874742f)
+++ b/utterance-queue/js/UtteranceQueue.js	(date 1639778164037)
@@ -80,6 +80,17 @@
     // whether the UtterancesQueue is alerting, and if you can add/remove utterances
     this._enabled = true;
 
+    // @private {Map<Utterance,function>} - Maps the Utterance to a listener on its priorityProperty that will
+    // udpdate the queue when its priority changes. The map lets us remove the listener when it the Utterance gets
+    // removed from the queue.
+    this.utteranceToPriorityListenerMap = new Map();
+
+    this.announcer.announcementCompleteEmitter.addListener( utterance => {
+      if ( this.utteranceToPriorityListenerMap.has( utterance ) ) {
+        this.removePriorityListener( utterance );
+      }
+    } );
+
     if ( this._initialized ) {
 
       // @private {function}
@@ -122,8 +133,16 @@
     // Remove identical Utterances from the queue and wrap with a class that will manage timing variables.
     const utteranceWrapper = this.prepareUtterance( utterance );
 
+    assert && assert( !this.utteranceToPriorityListenerMap.has( utteranceWrapper.utterance ),
+      'About to add the priority listener twice, are only one should exist on the Utterance. The listener should have been removed by removeOthersAndUpdateUtteranceWrapper.' );
+    const priorityListener = () => {
+      this.prioritizeUtterances( utteranceWrapper );
+    };
+    utteranceWrapper.utterance.priorityProperty.lazyLink( priorityListener );
+    this.utteranceToPriorityListenerMap.set( utteranceWrapper.utterance, priorityListener );
+
     // Prioritize utterances based on Utterance `priority` (or perhaps other announcer logic)
-    this.prioritizeUtterances( utteranceWrapper.utterance );
+    this.prioritizeUtterances( utteranceWrapper, true );
 
     this.queue.push( utteranceWrapper );
   }
@@ -189,7 +208,9 @@
     options = merge( {
 
       // If true, then an assert will make sure that the utterance is expected to be in the queue.
-      assertExists: true
+      assertExists: true,
+
+      removePriorityListener: true
     }, options );
 
     const utteranceWrapperToUtteranceMapper = utteranceWrapper => utteranceWrapper.utterance === utterance;
@@ -198,7 +219,12 @@
       'utterance to be removed not found in queue' );
 
     // remove all occurrences, if applicable
-    _.remove( this.queue, utteranceWrapperToUtteranceMapper );
+    const removedUtteranceWrappers = _.remove( this.queue, utteranceWrapperToUtteranceMapper );
+
+
+    if ( options.removePriorityListener ) {
+      this.removePriorityListeners( removedUtteranceWrappers );
+    }
   }
 
   /**
@@ -207,38 +233,58 @@
    * @public
    * @override
    *
-   * @param newUtterance {Utterance}
-   * @param {UtteranceWrapper[]} queue - The queue of the utteranceQueue. Will be modified as we prioritize!
+   * @param utteranceWrapper {UtteranceWrapper}
+   * @param justAddedToQueue {boolean}
    */
-  prioritizeUtterances( newUtterance ) {
+  prioritizeUtterances( utteranceWrapper, justAddedToQueue = false ) {
+
+    const utteranceWrapperIndex = this.queue.indexOf( utteranceWrapper );
+    const utteranceWrapperInQueue = utteranceWrapperIndex >= 0;
+
+    let traverseToFrontStartIndex;
+    if ( utteranceWrapperInQueue ) {
+
+      // The utterance is in the queue already, we need to walk back to the front of the queue to remove
+      // Utterances that have a lower priority
+      traverseToFrontStartIndex = utteranceWrapperIndex - 1;
+    }
+    else if ( !justAddedToQueue ) {
+
+      // If not in the queue and it wasn't just added to the back of the queue, priority will be managed
+      // by the announcer
+      traverseToFrontStartIndex = -1;
+    }
+    else {
+
+      // We are being added to the back of the queue for the first time, walk from the end to the front removing
+      // Utterances of lower priority
+      traverseToFrontStartIndex = this.queue.length - 1;
+    }
 
     // Update the queue before canceling the browser queue, since that will most likely trigger the end
     // callback (and therefore the next utterance to be spoken).
-    for ( let i = this.queue.length - 1; i >= 0; i-- ) {
+    for ( let i = traverseToFrontStartIndex; i >= 0; i-- ) {
 
       // {UtteranceWrapper} of UtteranceQueue
-      const utteranceWrapper = this.queue[ i ];
-
-      if ( this.shouldUtteranceCancelOther( newUtterance, utteranceWrapper.utterance ) ) {
-        this.removeFromQueue( utteranceWrapper );
+      const otherUtteranceWrapper = this.queue[ i ];
+      if ( this.shouldUtteranceCancelOther( utteranceWrapper.utterance, otherUtteranceWrapper.utterance ) ) {
+        this.removeUtterance( otherUtteranceWrapper.utterance );
       }
     }
 
-    this.announcer.onUtterancePriorityChange( newUtterance );
-  }
+    // Now look backwards to determine if the utteranceWrapper should be removed because an utterance behind it
+    // has a higher priority. The only utterance that we have to check is the next one in the queue because
+    // any utterance further back MUST be of lower priority. The next Utterance after utteranceWrapper.utterance would
+    // have been removed when the higher priority utterances further back were added.
+    if ( utteranceWrapperInQueue ) {
+      const otherUtteranceWrapper = this.queue[ utteranceWrapperIndex + 1 ];
+      if ( otherUtteranceWrapper && this.shouldUtteranceCancelOther( otherUtteranceWrapper.utterance, utteranceWrapper.utterance ) ) {
+        this.removeUtterance( utteranceWrapper.utterance );
+      }
+    }
 
-  /**
-   * Remove an UtteranceWrapper from the queue.
-   * @public
-   *
-   * @param utteranceWrapper
-   */
-  removeFromQueue( utteranceWrapper ) {
-
-    // remove the wrapped Utterance if it is queued - it may not be in case we are using announceImmediately
-    const indexOfWrapper = this.queue.indexOf( utteranceWrapper );
-    if ( indexOfWrapper > -1 ) {
-      this.queue.splice( indexOfWrapper, 1 );
+    if ( this.queue.length > 0 ) {
+      this.announcer.onUtterancePriorityChange( this.queue[ 0 ].utterance );
     }
   }
 
@@ -280,7 +326,8 @@
     utteranceWrapper.timeInQueue = Math.max( times );
 
     // remove all occurrences, if applicable. This side effect is to make sure that the timeInQueue is transferred between adding the same Utterance.
-    _.remove( this.queue, currentUtteranceWrapper => currentUtteranceWrapper.utterance === utteranceWrapper.utterance );
+    const removedWrappers = _.remove( this.queue, currentUtteranceWrapper => currentUtteranceWrapper.utterance === utteranceWrapper.utterance );
+    this.removePriorityListeners( removedWrappers );
   }
 
   /**
@@ -346,9 +393,37 @@
    * @public
    */
   clear() {
+
+    // Removes all priority listeners from the queue.
+    this.removePriorityListeners( this.queue );
+
     this.queue = [];
   }
 
+  /**
+   * Removes the listeners on Utterance Priority for all provided UtteranceWrappers.
+   * @private
+   * @param utteranceWrappers
+   */
+  removePriorityListeners( utteranceWrappers ) {
+    utteranceWrappers.forEach( utteranceWrapper => this.removePriorityListener( utteranceWrapper.utterance ) );
+  }
+
+  /**
+   * @private
+   * @param utterance
+   */
+  removePriorityListener( utterance ) {
+    const listener = this.utteranceToPriorityListenerMap.get( utterance );
+
+    // The same Utterance may exist multiple times in the queue if we are removing duplicates from the array,
+    // so the listener may have already been removed.
+    if ( listener ) {
+      utterance.priorityProperty.unlink( listener );
+      this.utteranceToPriorityListenerMap.delete( utterance );
+    }
+  }
+
   /**
    * Set whether or not the utterance queue is muted.  When muted, Utterances will still
    * move through the queue, but nothing will be sent to assistive technology.
@@ -485,7 +560,10 @@
       }
 
       // remove the wrapped Utterance if it is queued - it may not be in case we are using announceImmediately
-      this.removeFromQueue( utteranceWrapper );
+      this.removeUtterance( utteranceWrapper.utterance, {
+        assertExists: false,
+        removePriorityListener: false
+      } );
 
       announceSuccessful = true;
     }

The crux of the changes involved:

• Add priorityProperty to Utterance.
• When an Utterances is added to the queue, a listener is added to priorityProperty that will call prioritizeUtterances when the value changes.
• prioritizeUtterances needed to change behavior depending on if the Utterance was added to the back of the queue, not in the queue anymore (because it is removed from the queue and being spoken by the announcer), or if the utterance is sitting in the queue already.
• Removing priorityProperty listeners when an Utterance is removed/cleared from the queue.
• Removing priorityProperty listeners when the Utterance has finished being spoken by the announcer. This information is available in the Voicing case, and we do our best in the aria-live case.
• See prioritizeUtterances in UtteranceQueue for most of the work and logic in this issue.

We created the following tests and saw all of these working as we expected before ending for the day:

// because we just want to test priority
const noCancelOptions = {
  cancelSelf: false,
  cancelOther: false
}
const firstUtterance = new phet.utteranceQueue.Utterance( {
  alert: 'This is a first utterance, it was put in the queue first.',
  announcerOptions: noCancelOptions
} );
const secondUtterance = new phet.utteranceQueue.Utterance( {
  alert: 'This is a second utterance, it was put in the queue second.',
  announcerOptions: noCancelOptions
} );
const thirdUtterance = new phet.utteranceQueue.Utterance( {
  alert: 'This is a third utterance, it was put in the queue third.',
  announcerOptions: noCancelOptions
} );


//-----------------------------------------------------------

// Add all 3 to back
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( secondUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

// if we do this, it would interrupt the first one and then we would hear the third one
secondUtterance.priorityProperty.value = 2;

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );

// after these, we should hear thirdUtterance right away
firstUtterance.priorityProperty.value = 0;
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );

// after these, we should hear thirdUtterance right away
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );
firstUtterance.priorityProperty.value = 0;

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );


// we should hear firstUtterance for 2 seconds, then get interrupted by third utterance
window.setTimeout( () => {
  firstUtterance.priorityProperty.value = 0;
}, 1000 );

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 0;
thirdUtterance.priorityProperty.value = 0;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

// we should hear firstUtterance for 2 seconds, then get interrupted by third utterance
window.setTimeout( () => {
  thirdUtterance.priorityProperty.value = 3;
}, 2000 );

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

// we should hear both Utterances in full, the new priority for firstUtterance is higher
// than the second one in the queue
window.setTimeout( () => {
  firstUtterance.priorityProperty.value = 5;
}, 2000 );

//-----------------------------------------------------------

I am unfortunately out of time, but hope to finish this change set on Tuesday 12/21/21.

[jessegreenberg]

The above commit uses the Utterance.priorityProperty feature in a listener on the Display to make it so that these Voicing toolbar buttons cannot be interrupted unless there is some interaction with the display.

@zepumph can you please review ff9c360?

[jessegreenberg]

Discussed together, one improvement is to replace setting priority to 0 to Utterance.LOW_PRIORITY.

[zepumph]

From a discussion with JG today, to fully implement these readme buttons generally, we may need to clear the queue when the read-me buttons are finished, because they will collect in the queue until the overview button has completed speaking.

[zepumph]

• Is this case buggy, or a feature?

1. Enable voicing
2. turn sim voicing off
3. press a readme button
4. While it is speaking, interact with something in the screen view.
5. The Read me button is interrupted, but no voicing comes from the sim.

[terracoda]

I did not think of this particular case, when I opened this issue.

That said, when Sim Voicing is off, a read me button is pressed and interaction starts in the sim, I think it seems good for Voicing of the Read Me button to be stopped because the learner has changed their focus to something new - the actual sim.

We may not have thought of this particular case, but I think all the work on utterance priority has paid off. A learner's actions (this what they are attending to) seems to be the playing out as the priority. There are very few (if any at all) discrepant events - events where learner action/attention and voiced information are out of synch.

I tested Friction and RaP, and I like how the voicing is being managed.

There is one issue in Friction that I am not interested in addressing at this time. The pressing of the Read Me button describes the temperature at the time of the pressing of the button, which seems reasonable to me. Due to the length of the response and speed at which cooling happens, the sim is cooler by the time the response is done. I learner can get a new description by pressing the button again or grabbing the book.

So to answer your question @zepumph ...
This case, "Sim Voicing Off, read-me button pressed, and interaction with the sim silencing the read-me button content mid-way because the learner's focus has switched to interacting with the sim seems like a nice feature to me.

I think the priorities are working well.

[terracoda]

@jessegreenberg, when the code is good, I think we can close this issue.

[terracoda]

And maybe we should change the title of the issue to: Read me button responses, sim voicing responses, & utterance priorities

[jessegreenberg]

to fully implement these readme buttons generally, we may need to clear the queue when the read-me buttons are finished, because they will collect in the queue until the overview button has completed speaking.

Done in 8a82246.

...one improvement is to replace setting priority to 0 to Utterance.LOW_PRIORITY.

Done in 7b717a4.

Is this case buggy, or a feature?

@terracoda comment #752 (comment) indicates this is OK. Ill leave it as is for now.

So we are back to ready for review in this issue. Here is the commit to review: ff9c360

[zepumph]

Awesome thanks.

Last thing. Do you feel like we should use Enumeration instead of number? I was just thinking we can't enforce having arbitrary priorities right now, which we currently don't have (but I had to go and check all usages of priority:. Do you like this better? If not feel free to close.

class Priority extends EnumerationValue {
  // Priority levels that can be used by Utterances providing the `announcerOptions.priority` option.
  public static TOP_PRIORITY = new Priority( 10 );
  public static HIGH_PRIORITY = new Priority( 5 );
  public static MEDIUM_PRIORITY = new Priority( 2 );
  public static DEFAULT_PRIORITY = new Priority( DEFAULT_PRIORITY );
  public static LOW_PRIORITY = new Priority( 0 );

  public constructor( public readonly priorityNumber: number ) {super();}

  public static enumeration = new Enumeration( Priority );
}

[jessegreenberg]

I do like that a little better, it was proposed in phetsims/utterance-queue#78 as well. I think it is a dev enhancement we can get to separate from this issue.