Closed jessegreenberg closed 2 years ago
OK I identified the case, it is actually pretty simple. We are not handling what should happen when you add an utterance to the queue while the Announcer is announcing that Utterance.
const testUtterance = new phet.utteranceQueue.Utterance( { alert: 'This is a test utterance.', announcerOptions: { cancelSelf: false } } );
phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );
window.setTimeout( () => {
phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );
}, 500 );
For behavior, it seems important that if you add the Utterance to the queue while the Announcer is still speaking it it should not interrupt. alertMaximumDelay
requires that an Announcer can be speaking an Utterance while that Utterance still sits in the queue.
We cannot simply remove the assertion. If this ran without error, the listener would be removed when the Announcer was done speaking the Utterance the first time. So changing the priorityProperty would have no effect while it was speaking the Utterance the second time.
Brainstorming fixes:
Map<Utterance,function>
or change it so that we can have more than one listener per Utterance.
currentlySpeakingUtterance
. Announcer shouldn't be aware of UtteranceQueue (ideally). So maybe instead of having the Announcer manage this listener, it should still be on the UtteranceQueue, but be separate from the listeners in utteranceToPriorityListenerMap
.Change utteranceToPriorityListenerMap so that it is not Map<Utterance,function> or change it so that we can have more than one listener per Utterance.
I feel like this is the way to go, but I would make it a list of functions, so you can remove the one that applies to you. Sorta like the "count" features of scenery node structures. We just said that a constraint of the system (Announcer + UtteranceQueue) is that one Utterance can be in multiple stages of the system at the same time. If duplicates are allowed, then changing the Map to be more tolerant feels great to me.
Thoughts?
I like that, but I am struggling with how to find the right listener to remove from the list.
EDIT: I suppose that the listeners are simple enough that it doesn't matter. They all do the same work on the utterance. so the map could look like Map<Utterance,function[]>
and whenever it is time to remove a listener from the list, we just remove the first one?
I found another bug that may mean further changes to Determined to be a non-issue.utteranceToPriorityListenerMap
: https://github.com/phetsims/utterance-queue/issues/47
Sorta like the "count" features of scenery node structures.
I liked this idea, this morning I tried an approach where we use counting variables to decide whether to add or remove a priorityProperty listener. It is basically a way to implement
Only add the priorityProperty listener to the Utterance if the Announcer is not currently speaking that Utterance. Only remove the priorityProperty listener on end if the Utterance is not still in the queue.
without looking at the Announcer.
UtteranceWrapper has a counting variable usageCount
that counts "usages"
announcer.announce()
increment counting varaible.announcementCompleteEmitter
, decrement counting variable.If counting variable is greater than zero when it is time to add or remove a priorityProperty listener we do nothing. In that case, the Utterance exists in the queue or is still being spoken by the Announcer.
So far it is working well. I am not seeing this problem anymore and unit tests are passing but it feels pretty complicated and I suspect there is an easier way. Here is the patch with this solution though:
FYI, with this patch, I can test voicing code that doesn't use priority:
Here is another solution that is more simple. It is also passing unit tests. It works by saving a reference to the UtteranceWrapper and priorityPropertyListener when we call announcer.announce
. Then when we receive the announcementCompleteEmitter
event, we unlink This way, the utteranceToPriorityListenerMap
is only for Utterances that are in the UtteranceQueue.queue
. It is fragile because it does not work under this condition (TODO in the patch):
this.announcer.announcementCompleteEmitter.addListener( utterance => {
// It is possible that this.announcer is used by a different UtteranceQueue. When the announcementCompleteEmitter
// announces, it may not be for this queue. Would love the following assertions though.
// TODO: This would break if both UtteranceQueues that share the same announcer both have the same Utterance at this.announcingUtteranceWrapper.utterance, https://github.com/phetsims/utterance-queue/issues/46
// assert && assert( this.announcingUtteranceWrapper, 'no announcingUtteranceWrapper' );
// assert && assert( this.announcingUtterancePriorityListener, 'no announcingUtterancePriorityListener' );
if ( this.announcingUtteranceWrapper && utterance === this.announcingUtteranceWrapper.utterance ) {
assert && assert( this.announcingUtteranceWrapper.utterance.priorityProperty.hasListener( this.announcingUtterancePriorityListener ) );
this.announcingUtteranceWrapper.utterance.priorityProperty.unlink( this.announcingUtterancePriorityListener );
this.announcingUtteranceWrapper = null;
this.announcingUtterancePriorityListener = null;
}
} );
Full patch:
EDIT: THe problem mentioned here is actually a problem for the patch in https://github.com/phetsims/utterance-queue/issues/46#issuecomment-1018717820 as well. Confirmed that this breaks with patch in that comment. This is an issue with master as well.
const testUtterance = new phet.utteranceQueue.Utterance( {
alert: 'This is a test utterance.',
alertStableDelay: 0,
announcerOptions: { cancelSelf: false }
} );
phet.scenery.voicingUtteranceQueue.addToBack( testUtterance )
phet.joist.joistVoicingUtteranceQueue.addToBack( testUtterance )
@zepumph and I discussed this during a11y dev meeting today and decided that the issue identified in https://github.com/phetsims/utterance-queue/issues/46#issuecomment-1018747398 is a separate issue and also low priority (see https://github.com/phetsims/utterance-queue/issues/48).
That freed up both solutions in mentioned above. The counting one seemed unnecessarily complicated so we went with https://github.com/phetsims/utterance-queue/issues/46#issuecomment-1018747398. While pairing, we refined some of the checks in the announcementCompleteEmitter
listener to make sure that the correct listener was getting removed.
I just committed the refined patch and verified that unit tests are passing. Next I would like to turn the case that identified this into a unit test. Then I think this issue can be closed.
const testUtterance = new phet.utteranceQueue.Utterance( { alert: 'This is a test utterance.', announcerOptions: { cancelSelf: false } } );
phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );
window.setTimeout( () => {
phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );
}, 500 );
Unit test added above, it is passing in Chrome and Firefox.
Options were removed from removeUtterance
, that was the final TODO for this issue. Since the strategy and bulk of the patch was reviewed together I think this is ready to close.
Id love to review the commits. I think there is one more assertion to add reopening.
Please double check to make sure my changes are correct in your mind.
This looks really really excellent. I want to note an important point of this work, just to make sure I understand it correctly. Let me know if you feel like this needs more explanation in the code. . . . this.announcingUtteranceWrapper
is storing a reference to the Utterance while it is being announced, but it is never used to cancel/interrupt the utterance. Instead, the code path is through the listener, which will generally compare the utterance to all items in the queue, and then forward to Announcer.onUtterancePriorityChange
.
This is a good pattern because it provides rigitity in how we ensure proper management of the priority listener add/remove calls, but flexibility as to whether or not the announcer actually cares about/handles priority (i.e. a lack of AriaLiveAnnouncer.onUtterancePriorityChange
).
Feel free to close.
I like that assertion a lot, and things are working well with it.
I want to note an important point of this work, just to make sure I understand it correctly
Yes, your description here is correct.
eel free to close.
Sounds good, thank you for reviewing this.
While working on review for https://github.com/phetsims/utterance-queue/issues/43 I hit this assertion
I think it is consistently happening in Chrome when pressing the voicing toolbar buttons rapidly.