phetsims / utterance-queue

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

Assertion with announcingUtteranceWrapper when canSpeakProperty.value is false #58

Closed jessegreenberg closed 2 years ago

jessegreenberg commented 2 years ago

From https://github.com/phetsims/number-play/issues/134#issuecomment-1031750987 -

When SpeechSynthesisAnnouncer.canSpeakProperty is false, we never call requestSpeech at

    if ( this.initialized && this._canSpeakProperty.value ) {
      this.requestSpeech( utterance );
    }

And so we never emit the announcementCompleteEmitter to clear the announcingUtteranceWrapper of the UtteranceQueue. So we hit the assertion

announcingUtterancePriorityListener should be set on this.announcingUtteranceWrapper

To reproduce in a sim, load number-play. Disable all audio with the navigation bar button. Then press the yellow speaker button a few times.

jessegreenberg commented 2 years ago

Should be fixed in the above commit. Unit tests are passing, and I can't produce the original case in https://github.com/phetsims/number-play/issues/134#issuecomment-1031750987.

I considered putting a check in attemptToAnnounce around here: https://github.com/phetsims/utterance-queue/blob/37fd8f82b0a55427ecd8100eebeac241b9fd27d5/js/UtteranceQueue.js#L608-L610 so that if the Announcer is not ready and its canSpeakProperty is false, we do not try to set a new announcingUtteranceWrapper. But it seemed more correct that for every call to announcer.announce() we should get an emit from the announcementCompleteEmitter, and the Announcer should be responsible for emitting that.

jessegreenberg commented 2 years ago

I think this should receive a review, @zepumph are you OK with this?

zepumph commented 2 years ago

Yes, I think in general this makes a bunch of sense. I feel as as a whole, you and I go really back and forth about who should hold ze power: UtteranceQueue or Announcer. It has led to some inconsistencies. Most often we say UtteranceQueue is running the show, but we also ask the Announcer for stuff. With this change, we send stuff to the Announcer fully expecting that it may just not be spoken.

Basically here are the control Properties that we have to keep track of: UtteranceQueue.muted UtteranceQueue.enabledProperty SpeechSynthesisAnnouncer.enabledProperty SpeechSynthesisAnnouncer.canSpeakProperty

With joist (thus our sims) kinda taking its pick as to what to set during different things, it is really hard for me to see the full picture, and thus what could be the best and simplest implementation. It is a work in progress, and one that relates to our conversation from a Friday past about creating a preferences model. Do you feel this tension, or am I overreacting? (those are your only two options. . . . )

jessegreenberg commented 2 years ago

I did not feel the tension but see it now that you mention it. I don't think this relates to to canSpeakProperty (A private DerivedProperty) but I see it as confusing that we have an enabledProperty on both the UtteranceQueue and the Announcer. I could see us making the enabledProperty of the Announcer utterance-queueu internal. Lets open a new issue for this.