phetsims / utterance-queue

Alerting library powered by aria-live
MIT License
0 stars 2 forks source link

Prioritization needs to be supported by UtteranceQueue.announceImmediately #42

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

From https://github.com/phetsims/joist/issues/752, We need to add this feature to announceImmediately. We have only gotten to addToBack thus far.

zepumph commented 2 years ago

There are TODOs pointing to this issue

jessegreenberg commented 2 years ago

In the above commit I added an initial test for announceImmediately and it is broken. The test:

    // Add all 3 to back
    testVoicingUtteranceQueue.addToBack( firstUtterance );
    testVoicingUtteranceQueue.addToBack( secondUtterance );
    testVoicingUtteranceQueue.addToBack( thirdUtterance );

    assert.ok( testVoicingUtteranceQueue.queue.length === 3, 'All three utterances in the queue' );

    // now speak the first utterance immediately
    testVoicingUtteranceQueue.announceImmediately( firstUtterance );

    await timeout( timeForFirstUtterance / 2 );

    // this should have no impact on the queue (should not remove the duplicate firstUtterance that is already in queue)
    assert.ok( testVoicingUtteranceQueue.queue.length >= 3, 'announcing firstUtterance immediately has no impact on existing queue' );

The problem is in announceImmediately, we use unshift and let stepQueue announce it as soon as possible. In step, we attemptToAnnounce, which will use removeUtterance if announce is successful. removeUtterance will remove ALL UtteranceWrappers with the same Utterance, and so of the firstUtterance get removed from the queue.

jessegreenberg commented 2 years ago

I currently see zero usages of announceImmediately in the project, is this worth supporting? In general I see speakIgnoringEnabled used instead.

Another problem is that announceImmediately uses attemptToAnnounce which uses the Announcer.readyToSpeak. The current implementation puts the utterance at the front of the queue if it isn't ready to speak, but that means that the utterance is not announced immediately. I remembered announceImmediately was useful in cases where speech needed to be made synchronously with zero delay. But since there are no usages of it, that could be wrong.

jessegreenberg commented 2 years ago

Discussed today with @zepumph -

We need announceImmediately and want it to to be usable instead of speakIgnoringEnabled (where that function is used for synchronous speech).

We discussed replacing announceImmediately with announceSynchronous which simply no-op if the announcer wasn't ready to speak intead of adding the utterance to the front of the queue. But we convinced ourselves that if the announcer is not ready to speak, there has already been some successful speech and so synchronous speech is not necessary because synchronous speech is just a requirement for the first time SpeechSynthesis is used.

So we would like priority to work with the current behavior of announceImmediately.

jessegreenberg commented 2 years ago

I am running into a problem again where UtteranceQueue assumed that there would only be one reference per Utterance in the queue at a time. The current behavior of announceImmediately breaks this by always putting the Utterance at the front of the queue. It seems very reasonable to me that announceImmediately would also remove duplicate Utterances in the queue, and I don't see any reason not to do that. I think this would simplify quite a bit and let us use existing utterance-queue implementations for announceImmediately.

zepumph commented 2 years ago

Let's go with that then! But please make sure to document well all the excentricities of announceImmediately. I don't want people using it for any sort of normal occurence.

jessegreenberg commented 2 years ago

I was able to make some good progress on this issue today. https://github.com/phetsims/utterance-queue/commit/2291f6831431562cae28cd734df0981c7e627a9b is the heart of the work and https://github.com/phetsims/utterance-queue/commit/02bfe5c32d99cd68ff1a29eec0737be7434cafd8 updates the documentation for announceImmediately.

I added many unit tests verifying the behavior and timing of announceImmediately and was able to get them passing on both Chrome and Firefox.

I think this is ready for review. @zepumph would you mind reviewing the changes? It may be best to review together line by line at the next a11y dev meeting since we worked on this collection of issues together so much, but assigning to you in case you prefer to review before then.

zepumph commented 2 years ago

This is really really nice, by deciding that announceImmediately is basically a glorified addToFront, but I think it is better to have a different name because of its mostly-sync usage needs. Good documentation too! I used this review as an opportunity to clear up our terminology in UtteranceQueue a bit, focusing around announce instead of speak/alert/spoken. This involved the rename from readyToSpeak to readyToAnnounce. It felt to me like lots of vestigial words were around from voicing-specific thought-process.

Anything else here?

jessegreenberg commented 2 years ago

OK, thanks for reviewing. Consistent use of "announce"/"announcer" is good in your commits. I think that is all, ready to close.