phetsims / utterance-queue

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

Can we remove safariWorkaroundUtteranceWrappers from voicingManager? #52

Closed jessegreenberg closed 2 years ago

jessegreenberg commented 2 years ago

voicingManager currently lives in scenery but I am putting this issue here because it came from https://github.com/phetsims/utterance-queue/issues/43 and because this issue is part of a collection of other utterance-queue work. And SpeechSynth code may soon move to this repo anyway see #34.

@zepumph said

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.

Earlier today we tried to remove them by removing the end event listeners entirely in https://github.com/phetsims/scenery/issues/1344 and couldn't get it to work. But we may still be able to get rid of the safari workaround list because the reference to the speaking utterance is in the speakingSpeechSynthesisUtteranceWrapper.

Here is the bug that made the workaround necessary - https://github.com/phetsims/john-travoltage/issues/435. if this is not reproducible and the sims behave well I think we can consider it safe to remove.

jessegreenberg commented 2 years ago

From https://github.com/phetsims/utterance-queue/issues/43#issuecomment-1020391917

jessegreenberg commented 2 years ago

I removed the workaround array. Unit tests are passing in Chrome, Firefox and Safari. I was not able to produce the bug in https://github.com/phetsims/john-travoltage/issues/435 in Safari after this change. So I think we are in the clear. @zepumph since this came from https://github.com/phetsims/utterance-queue/issues/43#issuecomment-1020391917, would you recommend anything else?

zepumph commented 2 years ago

Looks so great. Thank you!

jessegreenberg commented 2 years ago

Great, thanks!