phetsims / friction

"Friction" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/friction
GNU General Public License v3.0
4 stars 6 forks source link

Voicing doesn't describe action that interrupts Quick Info statements #326

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air (m1 chip)

Operating System 13.1

Browser safari and chrome

Problem description For https://github.com/phetsims/qa/issues/886, after pressing a Quick Info button, if I press another button or grab the book (without moving it), the statement being uttered is interrupted but I don't hear anything about the action I just took. For example, if I pressed Reset All, I don't hear "Reset All. Everything Reset."

I looked at other sims with Voicing and older versions of Friction and found: This issue was not present in : Friction dev.23, published GFLB, published JT This issue is present in: published RaP, in all sims with Voicing in master.

Was this an intentional change that was made at some point?

Steps to reproduce

  1. In Preferences, enable Voicing
  2. Press Overview button
  3. While Voicing is speaking, press the Reset All button (or grab book without moving it, or press Hide Toolbar button)

Visuals

https://user-images.githubusercontent.com/87318828/214645865-8041cfb5-ebd5-41b4-aa7a-28afa76edf70.mov

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Friction‬ URL: https://phet-dev.colorado.edu/html/friction/1.6.0-rc.1/phet/friction_all_phet.html?printVoicingResponses Version: 1.6.0-rc.1 2023-01-17 16:32:22 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.2 Safari/605.1.15 Language: en-US Window: 1424x710 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
jessegreenberg commented 1 year ago

I thought we got to the bottom of this, maybe in https://github.com/phetsims/joist/issues/846 or https://github.com/phetsims/joist/issues/752. Ill try to find some history.

jessegreenberg commented 1 year ago

Yes, this issue is exactly https://github.com/phetsims/joist/issues/846. At the end of that issue we added a fix for the "Sim Voicing" toggle switch, but it was noted as a general problem in https://github.com/phetsims/joist/issues/846#issuecomment-1230334441.

In https://github.com/phetsims/joist/issues/846#issuecomment-1268833383 it was noted

Please note this issue is blocking the publication of Friction. I don't think @jessegreenberg and I have a direction forward just yet.

In https://github.com/phetsims/joist/issues/846#issuecomment-1233539545 I brainstormed some ways forward. None of the ideas are good and all would take significant time.

I would recommend accepting this behavior for the publication of Friction and noting it as a known issue, but lets review next week.

jessegreenberg commented 1 year ago

We identified that we do not need the call to voicingUtteranceQueue.cancel in VoicingToolbarItem. This can all be removed.

https://github.com/phetsims/joist/blob/7b833a663f0c735e5e4a254ace9a1a693a5835ef/js/toolbar/VoicingToolbarItem.ts#L222-L234

This is safe to remove because priorityProperty will handle managing the UtteranceQueue for us. We reviewed https://github.com/phetsims/joist/issues/752 and https://github.com/phetsims/joist/issues/846 (particularly comment https://github.com/phetsims/joist/issues/752#issuecomment-1192966444) and believe this change is very safe.

jessegreenberg commented 1 year ago

Next steps are to remove that whole block of code and revert any changes related to the muteSwitchUtterance from https://github.com/phetsims/joist/issues/846.

zepumph commented 1 year ago

Working on this now.

zepumph commented 1 year ago

I committed what @jessegreenberg explained above. @Nancy-Salpepi can you please confirm this is fixed on master, and please also test interrupting the quick info with the sim voicing off toggle (which we want to confirm is still fixed from https://github.com/phetsims/joist/issues/846).

Everything was sounding great to me in friction. When interrupting a quick-info button, I heard: reset all button, grabbed book, and sim voicing toggle.

Nancy-Salpepi commented 1 year ago

This is working very nicely now in master! I hear everything I should--sim voicing off, reset all button, toolbar hidden, book grabbed, and everything in the nav bar.

jessegreenberg commented 1 year ago

Thanks @zepumph , https://github.com/phetsims/joist/commit/5b7dcffe80706e6dc1d6430e840bacc67aa9bb96 looks nice.

jessegreenberg commented 1 year ago

Pushed to joist friction-1.6 branch. I tested in the SHAs and it is working, ready to verify.

pixelzoom commented 1 year ago

@jessegreenberg You added ready-for-review, but no assignee to review. So assigning to you.

jessegreenberg commented 1 year ago

This is in the QA process and is being reviewed in in the next RC.

Nancy-Salpepi commented 1 year ago

This is working nicely in rc.3. Closing.