phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Quick info alerts interrupted with Chrome 109 #898

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air (m1 chip)

Operating System 13.1

Browser Chrome Version 109.0.5414.87

Problem description For https://github.com/phetsims/qa/issues/879, on the newest version of chrome (109....) Quick Info alerts will not play when interrupted.

Steps to reproduce

  1. Turn on Voicing
  2. Press Overview
  3. Before Voicing is done, press Details --> Nothing is read. You have to press Details 2 more times for Voicing to start again.

I didn't see this issue on published GFLB and JT. When I tried GFLB on master, it happened there as well.

Visuals

https://user-images.githubusercontent.com/87318828/212174873-95fb45f7-446c-4094-9e48-a3c92ec9b88f.mov

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Quadrilateral‬ URL: https://phet-dev.colorado.edu/html/quadrilateral/1.0.0-dev.73/phet/quadrilateral_all_phet.html Version: 1.0.0-dev.73 2023-01-08 23:48:12 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/109.0.0.0 Safari/537.36 Language: en-US Window: 1480x781 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: {}
stemilymill commented 1 year ago

When looking into this with Nancy, I hadn't updated to the newest version of chrome yet, was still on 108.0.5359.125 and this behavior did not occur. I then updated to latest and the alerts were interrupted. This was consistent on mac and windows.

jessegreenberg commented 1 year ago

Thanks for the report. I confirmed it is not happening in Version 108.0.5359.125 (Official Build) (64-bit). And it is happening in Version 109.0.5414.75 (Official Build) (64-bit).

Nancy-Salpepi commented 1 year ago

@stemilymill also confirmed that it doesn't happen in published sims.

jessegreenberg commented 1 year ago

It is an issue for published ratio-and-proportion but not gravity-force-lab-basics. I suspect something from https://github.com/phetsims/utterance-queue/issues/97 may not be compatible with latest Chrome, but its just a guess.

Nancy-Salpepi commented 1 year ago

Sorry--we only tested published GFLB and JT! Forgot about RaP!

jessegreenberg commented 1 year ago

No worries! Only thought to check because it was published recently. If it wasn't there we would know the problem was introduced in the last two weeks.

jessegreenberg commented 1 year ago

I am going to transfer to joist, it seems specific to these buttons and is now a common code problem.

jessegreenberg commented 1 year ago

It is caused by this line:

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

So the order of events has changed when you press the second button.

jessegreenberg commented 1 year ago

I should note this is happening in master and published RaP but NOT in older sims. That made me suspect https://github.com/phetsims/utterance-queue/issues/97, but commenting out those changes did nothing.

jessegreenberg commented 1 year ago

When I comment out this line, the problem goes away:

https://github.com/phetsims/utterance-queue/blob/1c2007a9c3b1123ebba1ca2014ac0a72b76ba314/js/SpeechSynthesisAnnouncer.ts#L605-L607

And that matches this bug showing up in a platform update, I could see Chrome starting to fire BOTH the error and cancel event for some reason.

EDIT: I noticed that we were never removing the error listener. We should be, so I added this line. It fixed the problem!! But I don't know why. To be continued tomorrow.

Index: js/SpeechSynthesisAnnouncer.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/SpeechSynthesisAnnouncer.ts b/js/SpeechSynthesisAnnouncer.ts
--- a/js/SpeechSynthesisAnnouncer.ts    (revision 1c2007a9c3b1123ebba1ca2014ac0a72b76ba314)
+++ b/js/SpeechSynthesisAnnouncer.ts    (date 1673566796120)
@@ -647,6 +647,7 @@
     this.endSpeakingEmitter.emit( stringToSpeak, speechSynthesisUtteranceWrapper.utterance );
     this.announcementCompleteEmitter.emit( speechSynthesisUtteranceWrapper.utterance, speechSynthesisUtteranceWrapper.speechSynthesisUtterance.text );

+    speechSynthesisUtteranceWrapper.speechSynthesisUtterance.removeEventListener( 'error', speechSynthesisUtteranceWrapper.endListener );
     speechSynthesisUtteranceWrapper.speechSynthesisUtterance.removeEventListener( 'end', speechSynthesisUtteranceWrapper.endListener );
     speechSynthesisUtteranceWrapper.speechSynthesisUtterance.removeEventListener( 'start', speechSynthesisUtteranceWrapper.startListener );
jessegreenberg commented 1 year ago

Here is some logging output from the buggy case demonstrating why not removing the error event listener is a problem. Logging starts after pressing the second button: image

This shows that the error event for the first SpeechSynthesisUtterance happened asynchronously and after the second Utterance had been added to the UtteranceQueue.

So removing the error listener definitely fixes this problem.

jessegreenberg commented 1 year ago

@Nancy-Salpepi can you please confirm that this is fixed in master?

Nancy-Salpepi commented 1 year ago

it is indeed fixed @jessegreenberg

jessegreenberg commented 1 year ago

Great, thanks! Closing.

zepumph commented 1 year ago

For https://github.com/phetsims/friction/issues/327 we just need to cherry pick https://github.com/phetsims/utterance-queue/commit/2f64e9580771087750bdc3f1c5ac778abb089f1f