Reusable singleton for UtteranceQueue

[jessegreenberg]

In https://github.com/phetsims/utterance-queue/issues/7 the UtteranceQueue was changed from a singleton because of https://github.com/phetsims/utterance-queue/issues/7#issuecomment-555671762.

An instance of an UtteranceQueue for PhET's usage is now on the Sim. Currently usages look like phet.joist.sim.utteranceQueue...

In https://github.com/phetsims/balloons-and-static-electricity/issues/454 @zepumph said

What if in utterance-queue there was an utteranceQueueSingleton getter that could get phet.joist.sim.utteranceQueue || new UtteranceQueue() or something like that. Is it gross to put that in utterance-queue? Called utteranceQueueSingleton. Then we could change all global usages of utteranceQueue to use that module. Could it live in joist? No I don't think so, because it would need to be used in lower level repos like sun/scenery-phet.

I like this idea, I think it is worth exploring more to avoid going through namespace.

[zepumph]

I agree and understand your concerns, but I worry that having a singleton would be too limiting, and actually just orthogonal to the problem. Over in https://github.com/phetsims/joist/issues/686 there is discussion of removing the dependency on joist in common code by using an options pattern. This would be (to me) much more flexible than a singleton that would be hard coded to a single instance of Utterance. With the new implementation of decoupling the announcer from UtteranceQueue (to support aria-live and webAudio), it seems like this is best long term solution to allow each component that uses utteranceQueue to support an option to pick what utteranceQueue is to be used for its functionality.

If you agree, feel free to close this and we will do the conversion over in that other issue.

[zepumph]

I actually think I'd love to touch base on this at an a11y dev meeting. Since working more on https://github.com/phetsims/joist/issues/686 I'm sorta split. I can see positives both ways. It would certainly be easiest from usage's standpoint to just have a singleton. But I could also live with setting default options to the global in common code. Either way, let's decide and implement HERE, not in that issue.

[samreid]

Reading through https://github.com/phetsims/utterance-queue/issues/7#issuecomment-555671762, it seems you could have one singleton per display, with our primary PhET utterance queue defined in joist. It doesn't have to be only one utterance queue for the entire application.

[zepumph]

with our primary PhET utterance queue defined in joist

Does that mean we allow importing joist from sun/scenery-phet/scenery?

[zepumph]

It doesn't have to be only one utterance queue for the entire application.

I'm not completely sure what you mean by this, but I will say that it could get quite buggy if there are utterances going to different queues, because of the nature of how we prioritize elements in the queue. We really need one master list that all utterances get put into.

[samreid]

Usage sites could be getGlobal( 'phet.joist.utteranceQueue' ) instead of getGlobal( 'phet.joist.sim.utteranceQueue' ) so they don't have to go through sim. This would help us prune the number of phet.joist.sim occurrences, but I'm not sure if it's worth it.

[zepumph]

@jessegreenberg and I decided that we like using getGlobal for this in common code. In sims, where it is fine to access joist globals, we can call upon the global directly. Assigning to both of us, whoever gets to it first.

[zepumph]

Today @jessegreenberg, @jonathanolson and I found that we can build this into the scene graph. I want to experiment with adding a ParallelDOM.getUtterances() that can look up a connected tree to find the utteranceQueues for each Display this Node is connected to.

[jessegreenberg]

We discussed adding to Node a function that would let us get the connected Displays for a Node, so that we can get all the utteranceQueues for each Display. It would look something like this:

  /**
   * @private
   * / 
  getRecursiveConnectedDisplays( displays ) {
    displays.push( ...this.rootedDisplays );

    for ( let i = 0; i < this._parents.length; i++ ) {
      displays.push( ...this._parents[ i ].getRecursiveConnectedDisplays( displays ) );
    }

    return displays;
  }

  getConnectedDisplays() {
    return _.unique( this.getRecursiveConnectedDisplays( [] ) );
  }

[zepumph]

I felt really good about the above plan. Enough so that I just committed the change to ParallelDOM and Node. I converted Ohms Law and RIAW (just 5 calls to utteranceQueue in total) as a proof of concept.

Next steps here would be for @jonathanolson to take a review of the above commits, especially for performance within Node.getConnectedDisplays().

Then next we can convert other usages.

One note was that I hard coded addToBack. There is only one usage of the interactive description utteranceQueue being used with addToFront. We may have to look at usages of toggling enabled etc, but this seemed like a good place to start. Like we talked about during our meeting last week, we are alerting to all connected Display.utteranceQueues.

[jonathanolson]

It looks good to me, just requires a @param in getRecursiveConnectedDisplays doc.

[jessegreenberg]

I just hit a "maximum call stack size exceeded" from getConnectedDisplays because each visit adds all the Displays back to the list so it grows like 2^n.

In https://github.com/phetsims/scenery/commit/30c7368642b61c8a5f8e971a46319112483fd08f I a call to _.uniq to each visit in getRecursiveConnectedDisplays, but now the list can only grow to n + number of displays used.

[zepumph]

Sorry this fell off of my radar completely. Thanks @jessegreenberg, that is all really nice. I also just came back to the usages in OL and RIAW, and tested on NVDA. Things are sounding really nice.

Besides supporting addToBack, I see 11 usages of other public features of the UtteranceQueue:

clear() enabled addToFront

We could do a couple of things to handle this:

1. create an abstraction on PDOM for each method
2. make a general function call PDOM.forEachUtterance( function(utterance) ), which you can do all these operations for. If any of these 11 usages look at first glance like they would work with this, and it would be future proof, but it is a bit clunky.

@jessegreenberg what do you think?

[jessegreenberg]

I am nervous about option 1 because it will add a number of functions to ParallelDOM.js, and every time we add a public function to UtteranceQueue we would presumably need to add it to ParallelDOM.

I like option 2, though would that be something like PDOM.forEachUtteranceQueue( function( utteranceQueue ) )? Or just a getter for the utteranceQueues to be iterated on ParallelDOM.getUtteranceQueues()?

[zepumph]

Seems ok either way, but I kinda like passing in the function right now. Would it be better to have a list to do with what you wish?

const myNode = new Node();

myNode.getUtteranceQueues().forEach( queue => queue.addToFront( 'blargety' ) );

// or 

myNode.forEachUtteranceQueue( queue => queue.addToFront( 'blargety' ) );

[jessegreenberg]

but I kinda like passing in the function right now

OK, that sounds good to me!

[zepumph]

@jessegreenberg and I confirmed, we will go with:

myNode.forEachUtteranceQueue( queue => queue.addToFront( 'blargety' ) );

[zepumph]

• [x] I encountered that this is a bit of a weird pattern to try to conform our Describer and AlertManager patterns to. No we need a reference to a Node in order to alert to the description utterance queue. I will convert as many usages as I can, but this is certainly not easy in some cases, like pretty much every usage of utteranceQueue in Friction at the moment. I'll continue with my conversion, but I think there will be some refactoring to make sure we pass in the Node that we want to alert for. That doesn't seem horrible to me in general. But perhaps we will hate it and reevaluate once we start implementing more. For non-addToBack usages, this effects Friction and BASE.

[zepumph]

• [x] I also found that this line will get quite a bit more annoying, to support an arbitrary number of utteranceQueues being connected to the resetAllButton:

https://github.com/phetsims/scenery-phet/blob/e8f9f44b3272f48d014138ae560284485dff11da/js/buttons/ResetAllButton.js#L86-L107

[zepumph]

For the a11y view, there is this line:

https://github.com/phetsims/chipper/blob/5a7e7770163f9b524dc262f8ccb2a2f9d7426f97/templates/sim-a11y-view.html#L519

this felt like an alright use of the global, but I am grabbing it from the display directly now, because hopefully we can get rid of sim.utteranceQueue as a global

[zepumph]

• [x] AccessibleValueHandler is weird too, I should look into that one.

[zepumph]

Ok. I felt good enough to commit these changes, and the snapshot comparison task didn't reveal any differences in how the utterances were being emitted (so excited for that test btw!!). From here, the main concern is to figure out how we want to support the current Describer and AlertManager patterns, which are heavily used in some sims. Where those patterns control alerting, there seems to be no easy way to get a reference to a Node.

From here I see two paths forward.

1. We call it good, and in the future only use PDOM.alertDescriptionUtterance instead of the global from the sim. I would most prefer not doing this, but it may be more cost that it's worth to change the rest of the usages (~25 usages)
2. @jessegreenberg and I discuss how the best pattern is for these alerting types is, and we adopt it. My first thought is passing in a Node to methods that alert. Another way is to refactor AlertManagers to the spots where alerts happen in the view.

Marking for a11y meeting. @jessegreenberg feel free to respond async if you want to get to it sooner than next Friday.

[zepumph]

High priority at this point because it is hopefully close to being complete, and so we should sprint on it to finish it off.

[zepumph]

• [x] Don't forget about the usage of addToBackIfDefined, which I left out of above searches on accident.

[zepumph]

I explained the issue to @jessegreenberg, and we didn't have anything off the top of our tired little heads, but we will meet next week to discuss further.

[zepumph]

I talked with @jessegreenberg about this today, and we would like to pursue the following strategy

• Create a base class AlertManager that takes a node, and creates forwarding methods to alertDescriptionUtterance and forEachUtteranceQueue in the PDOM.
• Go through current usages of phet.joist.sim.utteranceQueue and refactor those types into AlertManagers. There are cases where types called "*Describers" are alerting, and we feel like that is not correct. Any alerting should be done in an AlertManager subtype. We may run into trouble in the ISLCDescriber Hierarchy, but we can potentially use composition to achieve alerting for that one case (though not ideal).

I will take care of this.

[zepumph]

I created AlertManager and tested things out in Friction (in https://github.com/phetsims/friction/issues/217). We are now down to 43 usages of phet.joist.sim.utteranceQueue.

[zepumph]

Down to 36 now after Molarity.

[zepumph]

Fixed ResetAllButton above.

[zepumph]

BalloonDescriber fixed above.

[zepumph]

From Meeting with @jessegreenberg today:

• [x] Let's rename AlertManager.js to Alerter.js (see https://github.com/phetsims/scenery-phet/issues/697)
• [x] Let's make Alerter.js support options like I just added in MovementAlerter.js (making that type extend Alerter.js instead of implement its own)

Here is a patch to get us started:

Index: js/accessibility/describers/AlertManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/describers/AlertManager.js b/js/accessibility/describers/AlertManager.js
--- a/js/accessibility/describers/AlertManager.js   (revision 5673417504f4230d09cc0fbdb3d0bf0383f92a76)
+++ b/js/accessibility/describers/AlertManager.js   (date 1630087289257)
@@ -12,10 +12,23 @@
   /**
    * @param {Node} node
    */
-  constructor( node ) {
+  constructor( options ) {
+
+    options = merge( {
+
+      // When true, movement alerts will be sent to the voicingUtteranceQueue. This shutoff valve is similar to
+      // descriptionAlertNode, but for voicing.
+      alertToVoicing: true,
+
+      // {Node|null} If provided, use this Node to send description alerts to the Display's UtteranceQueue. Unlike for
+      // Voicing, description alerts must occur through a Node connected to a Display through the scene graph. If null,
+      // do not alert for description (same as alertToVoicing:false). NOTE: No description will alert without this option!
+      descriptionAlertNode: null
+    }, options );

     // @private - this node is only to be used to access the UtteranceQueues needed for alerting
-    this.node = node;
+    this.descriptionAlertNode = options.descriptionAlertNode;
+    this.alertToVoicing = options.alertToVoicing;
   }

   /**

[zepumph]

• [x] Apply this patch to bug fix resetAllButton:

Index: js/buttons/ResetAllButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/ResetAllButton.js b/js/buttons/ResetAllButton.js
--- a/js/buttons/ResetAllButton.js  (revision 5673417504f4230d09cc0fbdb3d0bf0383f92a76)
+++ b/js/buttons/ResetAllButton.js  (date 1630087610966)
@@ -106,12 +106,12 @@

           // mute and clear the utteranceQueue
           ariaEnabledOnFirePerUtteranceQueueMap.set( utteranceQueue, utteranceQueue.enabled );
-          phet.joist.sim.utteranceQueue.enabled = false;
-          phet.joist.sim.utteranceQueue.clear();
+          utteranceQueue.enabled = false;
+          utteranceQueue.clear();
         }
         else {
           utteranceQueue.enabled = ariaEnabledOnFirePerUtteranceQueueMap.get( utteranceQueue ) || utteranceQueue.enabled;
-          phet.joist.sim.utteranceQueue.addToBack( resetUtterance );
+          utteranceQueue.addToBack( resetUtterance );
         }
       } );
     } );

[zepumph]

• [x] Create an issue for GFL suite and also Greenhouse effect to convert those usages, assigning them to @jessegreenberg to implement once the above is taken care of.

[zepumph]

• [x] I also have to handle AccessibleValueHandler

[zepumph]

• [x] There are still some usages of phet.joist.sim.utteranceQueue commented in Faradays Law

[zepumph]

• [x] I need to fix documentation in https://github.com/phetsims/phet-info/blob/f1ae8f2f1316a9fc21ea0fc79d09bb011a4b1455/doc/interactive-description-technical-guide.md#L121

[zepumph]

OK. I can't see anything more to do in this issue. I thought about keeping it open to remove the Sim.utteranceQueue getter, but we should just do that as part of the last usage over in https://github.com/phetsims/greenhouse-effect/issues/70.

@jessegreenberg, will you please review all commits in this issue, even though you have been part of this as we went. Can you think of any loose ends from this giant batch of work?

[jessegreenberg]

Thanks for doing all of this @zepumph, nice work. Especially in AccessibleValueHandler, that looked tricky. I spot checked commits since https://github.com/phetsims/utterance-queue/issues/13#issuecomment-894457016 and everything looks reasonable. The Sim.js utteranceQueue getter has been removed and I think this is ready to close.