phetsims / ratio-and-proportion

"Ratio and Proportion" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 4 forks source link

Voicing will sometimes read descriptions for previous item before current item #533

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air and Chromebook

Operating System macOS 13.0.1

Browser chrome

Problem description For https://github.com/phetsims/qa/issues/852, Voicing alerts aren't always interrupted when I tab through objects on chrome. This doesn't happen with mac + safari.

Steps to reproduce

  1. Enable Voicing and check all Sim Voicing Options
  2. Tab through the controls and nav bar (but do not press and hold)

Visuals

https://user-images.githubusercontent.com/87318828/203129156-e7b9e935-ef50-46b3-b565-a5d5f4490690.mov

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Ratio and Proportion‬ URL: https://phet-dev.colorado.edu/html/ratio-and-proportion/1.2.0-rc.1/phet/ratio-and-proportion_all_phet.html Version: 1.2.0-rc.1 2022-11-11 22:40:36 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36 Language: en-US Window: 1544x712 Pixel Ratio: 1.7999999523162842/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
zepumph commented 1 year ago

After some investigation here . . . .

This is another great bug!!!

We will talk about the example of playing around in the nav bar going from create screen -> discover screen button and then waiting. You hear the complete create screen response and then the complete discover screen response after.

Basically what I found is that in this case, you can get the a race condition between when the browser speech API starts speaking the first response (about the create screen) and when the discover screen utterance is added to the queue. Upon adding an utterance, we check to see if any ahead should be interrupted. Most of the time it is true, but in this case, we don't know to interrupt it because the currentlySpeakingUtterance isn't set until we get the start speaking listener. I believe there could be an easy solution here to just set the currentlySpeakingUtterance eagerly, before getting the start listener. I don't know if @jessegreenberg will have other thoughts about that, but I believe that it will work well.

My main worry was if speechSynthesis.cancel() wouldn't work because we hadn't received the start listener yet, but this script shows that not to be true. Please note here though that the end listener is still async.

```html ```

So what this tells us is that we can most likely fix the bug by setting the currentlySpeakingUtterance synchronously. I'll give it a test drive!

zepumph commented 1 year ago

Ok, so I was a bit wrong above, we have a currentlySpeakingUtterance AND a pending one, which holds the space in between the synchronous request and the async start listener. It is even being cancelled correctly. The bug is that it isn't taken into consideration when the utteranceQueue asks if the announcer should cancel from a new Utterance. I confirmed that this commit works to fix it, but I have some questions about it.

@jessegreenberg:

Thanks!

jessegreenberg commented 1 year ago

Ah, thank you for diving into this and fixing. The change makes sense.

Is there any way that we would have a pending AND currently speaking utterance,

I am not too worried about this, each Utterance would be cancelled according to shouldUtteranceCancelOther and I think that is correct.

Are there other spots where we should also case of the pending utterance.

I am looking at handleCanAnnounceChange. What if the canAnnounceProperty changes while the utterance is "pending", I don't see how the Utterance would be cancelled in that case.

EDIT: I think I confirmed this is a problem by setting (what is presumably the voicingVisibleProperty)

this.pendingSpeechSynthesisUtteranceWrapper.utterance.canAnnounceProperty.implementationProperty.value.dependencies[ 0 ].value = false

just above the

this.getSynth()!.speak( speechSynthUtterance );

in SpeechSynthesisAnnouncer

jessegreenberg commented 1 year ago

Tried to fix in the above commit. I verified that it is working by adding this line right after the this.getSynth()!.speak call.

this.pendingSpeechSynthesisUtteranceWrapper.utterance.canAnnounceProperty.implementationProperty.value.dependencies[ 0 ].value = false

Unit tests with ?manualInput are passing. @zepumph are you OK with this?

zepumph commented 1 year ago

Everything you made sounds good, but I think 10 minutes of sync conversation would be best before continuing. Shouldn't be much to make sure the bug is fixed completely.

zepumph commented 1 year ago

This was signed off on in https://github.com/phetsims/utterance-queue/issues/97. I'm feeling good about this issue.

TO cherry pick: https://github.com/phetsims/utterance-queue/commit/20567c5fd46cbb988205632df0e6240c3d87084f

zepumph commented 1 year ago

Done and ready for confirmation in next version.

Nancy-Salpepi commented 1 year ago

This looks good in 1.2.0-rc.2 with mac + chrome. Will reopen if something comes up on another platform.