search

phetsims / utterance-queue

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

Alerting an utteranceQueue generates the text twice #83

Closed zepumph closed 1 year ago

zepumph commented 2 years ago

Over in https://github.com/phetsims/ratio-and-proportion/issues/473, I found that alerting an utterance will formulate the text twice while alerting, once in utterancequeue, and the second time in the announcer. This is problematic for me because generating the context response creates side effects of state (checking against a "previousValue" for distance that is then updated after determining the context response). I think we need to make sure that this doesn't happen. Marking for a11y dev meeting with @jessegreenberg.

jessegreenberg commented 2 years ago

Is there a way that we can remove previousAlertText? The documentation for getAlertText says "Get the string to alert, with no side effects".

Alternatively, maybe most usages of getAlertText should be replaced with Utterance.alertableToText and getAlertText should be private to utterance-queue?

zepumph commented 2 years ago

This is the line that is causing the side effect, it is the nature of allowing functions as values in the utterance, both as top level alerts and in the ResponsePacket:

https://github.com/phetsims/utterance-queue/blob/ab47d818cda7c0463efe86276a0c945a88732462/js/ResponsePacket.ts#L163

and

https://github.com/phetsims/utterance-queue/blob/ab47d818cda7c0463efe86276a0c945a88732462/js/Utterance.ts#L342

Are we on the same page?

jessegreenberg commented 2 years ago

OK I understand the area of code better, thanks. Supporting side effects in this function sounds difficult because of how much it is used in the Voicing implementation. For example it is used by all Voicing.getVoicing*Response getters. These functions could be called more than once in a sim or in a scenery-internal implementation.

zepumph commented 2 years ago

@jessegreenberg and I worked on this today, We made it so that the utterancequeue gets the alert text, and then passes it to the announcer. This way we don't regenerate it twice for each utterance. We also updated other cases, and tested over in https://github.com/phetsims/ratio-and-proportion/issues/481 and things are working very well!

samreid commented 1 year ago

As discovered in https://github.com/phetsims/chipper/issues/946, there are TODOs in the code referring to this issue. Reopening.