Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we remove safariWorkaroundUtteranceWrappers from voicingManager? #52

Closed
jessegreenberg opened this issue Jan 28, 2022 · 4 comments
Closed

Comments

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jan 28, 2022

voicingManager currently lives in scenery but I am putting this issue here because it came from #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 phetsims/scenery#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 - phetsims/john-travoltage#435. if this is not reproducible and the sims behave well I think we can consider it safe to remove.

@jessegreenberg jessegreenberg self-assigned this Jan 28, 2022
@jessegreenberg
Copy link
Contributor Author

From #43 (comment)

@jessegreenberg
Copy link
Contributor Author

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

@zepumph
Copy link
Member

zepumph commented Feb 1, 2022

Looks so great. Thank you!

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Feb 1, 2022
@jessegreenberg
Copy link
Contributor Author

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants