It is possible to add a priorityProperty listener twice in Firefox

[jessegreenberg]

While working on #42 I found another case where the async end event in Firefox is causing a problem. I made a simple unit test to exemplify:

    testVoicingUtteranceQueue.addToBack( firstUtterance );

    // cancel the utterance while it is speaking - the cancel is synchronous, but the `end` event will be deferred
    await timeout( timeForFirstUtterance / 2 );
    voicingManager.cancelUtterance( firstUtterance );

    // Add the firstUtterance back to the queue - in Firefox this will happen BEFORE we get the `end` event, so 
    // this is also before we removed the priorityProperty listener from the Map in UtteranceQueue. Adding the 
    // utterance back will try to add another listener to the priorityProperty.
    testVoicingUtteranceQueue.addToBack( firstUtterance );
    assert.ok( true, 'did we get here??' );

The result is hitting assertion

Assertion failed: About to add the priority listener twice and only one should exist on the Utterance. The...

[jessegreenberg]

I considered eagerly removing the listener on cancelUtterance so that we can add it back right away when we next call addToBack. But that has a behavioral problem. Firefox will still give the end event from the cancelUtterance, but it will hapen after the call to addToBack. From the end event we will try to unlink the priorityProperty listener so we are no longer observing it even though that firstUtterance is still in the queue or being spoken by the voicingManager.

[jessegreenberg]

Other things I thought of that didn't seem like good solutions or even possible: 1) On cancel, set a flag that will not remove the listener on the subsequent end event. I didn't think about this too hard, but it seems likely to cause confusion. 2) Only remove the priorityProperty listener if the utterance is out of the queue and not being spoken by the synth. Only add listeners if we do not already have a priorityProperty listener in the map. I am not sure why I thought this would help, the behavioral problem will still be there. It will be possible to remove the priorityProperty listener from an Utterance before we are done speaking it. 3) Change the utteranceToPriorityListenerMap to map from UtteranceWrapper to listener so we can remove the listener for a particular UtteranceWrapper in the queue instead of whenever we get an end event for an Utterance. But all functions and Emitters from the Announcer use Utterance. UtteranceWrapper is private to UtteranceQueue.

One thought that I like more is to remove the end listener from the SpeechSynthesisUtterance when we cancel so that we don't have a delayed emission of the end event.

[jessegreenberg]

This is working well, and I have this case and other unit tests passing on both Chrome and Firefox. Ready to commit.

[jessegreenberg]

This is also ready for review, either separately or at an upcoming a11y dev meeting along with #42.

[zepumph]

• [x] Could we rename VoicingUtteranceWrapper to SpeechSynthesisUtteranceWrapper? Is that better name?
• [x] Could we compine VoicingUtteranceWrapper and UtterancePair, those seem redundant.
• [x] I would like to have a conversation about clearEmitter, I wasn't totally sure that it was necessary for how this code is working, and I would like to understand it better (a bit off topic here, sorry).
• [ ] Why does the safari work around list not need to be cleared as part of handleSpeechSynthesisEnd, and only in the endListener?
• [x] Shouldn't utteranceToVoicingUtteranceWrapperMap only ever have one or zero items in it? Can you convince me otherwise or refactor it to just a single pointer to an Wrapper|null?

[jessegreenberg]

Thank you for reviewing!

Could we rename VoicingUtteranceWrapper to SpeechSynthesisUtteranceWrapper? Is that better name? Could we compine VoicingUtteranceWrapper and UtterancePair, those seem redundant.

Yes, makes sense. Done in https://github.com/phetsims/scenery/commit/1841e9e5298b651a0e7ca314300e7d4fbba22cf2 and https://github.com/phetsims/scenery/commit/1841e9e5298b651a0e7ca314300e7d4fbba22cf2

I would like to have a conversation about clearEmitter, I wasn't totally sure that it was necessary for how this code is working, and I would like to understand it better (a bit off topic here, sorry).

Yes I think it can be removed. It already has a TODO in Announcer to remove it pointing to https://github.com/phetsims/joist/issues/752, so I think it will soon be gone.

Why does the safari work around list not need to be cleared as part of handleSpeechSynthesisEnd, and only in the endListener?

I remember explicitly thinking that it should not be in handeSpeechSynthesisEnd. . . but I don't know what I was thinking. It was moved in https://github.com/phetsims/scenery/commit/7470c93a8f432d9e4f0b26275bfa8dd70cf13544.

Shouldn't utteranceToVoicingUtteranceWrapperMap only ever have one or zero items in it? Can you convince me otherwise or refactor it to just a single pointer to an Wrapper|null?

I think you are right, good point. We assume that a start event will never come before an end or cancel event, so this seems legitimate. Done in https://github.com/phetsims/scenery/commit/f1cf55aa634634cf161fedfd4c9c88f483395017.

[jessegreenberg]

Reassigning to @zepumph to review comments above.

[zepumph]

Yes, makes sense. Done in phetsims/scenery@1841e9e and phetsims/scenery@1841e9e

That is the same commit, is there a second one to look at?

This is probably more harm that it is worth, but I feel like safariWorkaroundUtteranceWrappers is redundant at this point. Either we don't need it, because the SpeechSynthesisUtterance is being stored in memory by speakingSpeechSynthesisUtteranceWrapper, or we are going to hit this assertion anyways because we never got the end event. https://github.com/phetsims/scenery/blob/19c605df426735a678df69e8f6f18ea23c42ed93/js/accessibility/voicing/voicingManager.js#L370.

I hate to ask you to tear things like that up when we don't necessarily have a reproducible, buggy case, but it is a bit of a mess to have unnecessarily, so I guess I would just ask you what you think? I know you have a much better grasp on why we need to store this in memory anyways. Do we still need it?

And, as always, thanks for all this great work!

[jessegreenberg]

That is the same commit, is there a second one to look at?

Sorry, one is supposed to be https://github.com/phetsims/scenery/commit/05ad9257f26b63227a6aa32426cdc8d08b063c12., where VoicingUtteranceWrapper and UtterancePair were combined.

but I feel like safariWorkaroundUtteranceWrappers is redundant at this point.

I think you may be right! I started a new issue for this, here: https://github.com/phetsims/utterance-queue/issues/52

[zepumph]

All looks great. Thanks!

[jessegreenberg]

OK great, thanks.