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

assertSpeakingPropertiesInSync breaks outside of Chrome #1317

Closed
jessegreenberg opened this issue Oct 29, 2021 · 5 comments
Closed

assertSpeakingPropertiesInSync breaks outside of Chrome #1317

jessegreenberg opened this issue Oct 29, 2021 · 5 comments

Comments

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Oct 29, 2021

The assertSpeakingPropertiesInSync works for Chrome, but not in Firefox (and possibly others).

In #1288 (comment) I said

I discovered that synth.speaking and the start/end events just happen at different times (at least for Chrome). Once synth.speak is called, the synth.speaking flag is set to true synchronously.

But that is not true for Firefox. After calling synth.speak synth.speaking returns false in Firefox.

When I remove the assertion after the speak call (which clearly makes the assumption that synth.speaking is set synchronously) I still hit the assertion sometimes just tabbing around. In these cases where it is breaking speakingProperty.value is true while synth.speaking is false.

THis was found in phetsims/friction#268

@jessegreenberg
Copy link
Contributor Author

Can we convince ourselves that the assertion is not necessary and that speakingProperty does not need to exactly reflect synth.speaking? speakingProperty is kind of like a model Property for how we describe the state of voicingManager. But synth.speaking is just a flag that is internal to the implementation of the browser's SpeechSynthesis.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Oct 29, 2021

Also, the speakingProperty is not used at all in or out of voicingManager. Can we just remove it?

@jessegreenberg
Copy link
Contributor Author

I removed assertSpeakingPropertiesInSync and voicingManager.speakingProperty. The only usage of speakingProperty was replaced by synth.speaking. This means that getSynth().speaking is the only way to check for whether the synth is speaking in voicingManager, and that makes sense to me. The only bummer is that this removed the way to query whether the voicingManager is currently speaking. But that hasn't been needed at all yet.

@zepumph since we discussed assertSpeakingPropertiesInSync and voicingManager.speakingProperty a lot in #1288 would you mind reviewing this?

@zepumph
Copy link
Member

zepumph commented Oct 29, 2021

Sounds excellent to me! I think we can get really really far improving the behavior of start/end emitters across all browsers, and that will provide enough of a hook to accomplish pretty much everything I can think of this Property providing. Thanks for the ingenious problem solving here.

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Oct 29, 2021
@jessegreenberg
Copy link
Contributor Author

Sounds good, 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