Sim crashes with audio=disabled if user tries to enable audio from the Preferences dialog

[arouinfar]

Steps to reproduce:

1. Run a sim with a Preferences dialog on master with audio=disabled such as GLFB or BASE.
2. Open the Preferences dialog and select the Audio tab
3. Attempt to turn on Audio Features toggle
4. The sim freezes (crashes) and an error like this pops up in the console (example from BASE).

Uncaught Error: Assertion failed: must be connected to a display to use UtteranceQueue features
    at window.assertions.assertFunction (assert.js:25)
    at NavigationBarAudioToggleButton.alertDescriptionUtterance (ParallelDOM.js:2683)
    at pressedListener (NavigationBarAudioToggleButton.js:169)
    at TinyProperty.emit (TinyEmitter.js:86)
    at BooleanProperty._notifyListeners (Property.js:271)
    at BooleanProperty.set (Property.js:186)
    at BooleanProperty.set value [as value] (Property.js:341)
    at ToggleSwitch.Action.parameters.validValues (ToggleSwitch.js:169)
    at Action.execute (Action.js:227)
    at DragListener.end [as _end] (ToggleSwitch.js:239)

Troubleshooting Info for BASE

Name: Balloons and Static Electricity
URL: https://bayes.colorado.edu/dev/phettest/balloons-and-static-electricity/balloons-and-static-electricity_en.html?ea&brand=phet&audio=disabled
Version: 1.6.0-dev.0 (unbuilt)
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/93.0.4577.82 Safari/537.36
Language: en-US
Window: 1440x789
Pixel Ratio: 2/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: 15 uniform: 1024
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 80)
Max viewport: 16384x16384
OES_texture_float: true
Dependencies JSON: {}

Troubleshooting Info for GFLB

Name: Gravity Force Lab: Basics
URL: https://bayes.colorado.edu/dev/phettest/gravity-force-lab-basics/gravity-force-lab-basics_en.html?ea&brand=phet&audio=disabled
Version: 1.2.0-dev.0 (unbuilt)
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/93.0.4577.82 Safari/537.36
Language: en-US
Window: 1440x789
Pixel Ratio: 2/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: 15 uniform: 1024
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 80)
Max viewport: 16384x16384
OES_texture_float: true
Dependencies JSON: {}

Both of these sims are currently in RC testing. This particular bug is not present in the RC branches, but the behavior is buggy, see phetsims/balloons-and-static-electricity#543 and phetsims/gravity-force-lab-basics#311.

[jessegreenberg]

The comment before the assertion is

// If you run into this assertion, talk to @jessegreenberg and @zepumph, because it is quite possible we would
// remove this assertion for your case.

@zepumph do you think we can remove this? I think one of the benefits of using alertDescriptionUtterance is that it could gracefully handle silencing alerts when the associated Node is not in the scene graph.

However, the real bug for this issue is that any Audio controls are available in the Preferences Dialog when audio=disabled is used.

[jessegreenberg]

I thought about this patch:

Index: js/preferences/PreferencesConfiguration.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/preferences/PreferencesConfiguration.js	(revision 207264bc008a8f37535bf475521bb0b1c7d267d3)
+++ js/preferences/PreferencesConfiguration.js	(date 1631915041553)
@@ -24,6 +24,10 @@
     // are requested.
     const phetFeatures = phet.chipper.queryParameters;
 
+    // if the 'audio' query parameter is used to disable all audio, none of the features under audioOptions
+    // will be supported
+    const supportsAudio = phetFeatures.audio !== 'disabled';
+
     // For now, the Voicing feature is only available when we are running in the english locale, accessibility
     // strings are not made available for translation.
     const simLocale = phet.chipper.locale || 'en';
@@ -51,14 +55,17 @@
         // The entry point for Voicing, and if true the sim will support Voicing and Voicing options in Preferences.
         // The feature is only available on platforms where SpeechSynthesis is supported. For now, it is only available
         // when running with english locales, accessibility strings are not made available for translation yet.
-        supportsVoicing: phetFeatures.supportsVoicing && voicingManager.isSpeechSynthesisSupported() && simLocale === 'en',
+        supportsVoicing: phetFeatures.supportsVoicing &&
+                         voicingManager.isSpeechSynthesisSupported() &&
+                         simLocale === 'en' &&
+                         supportsAudio,
 
         // {boolean} - Whether or not to include checkboxes related to sound and enhanced sound. supportsEnhancedSound
         // can only be included if supportsSound is also true.
         // NOTE: When initialize-globals uses package.json to control sound features the packageJSON check
         // here can be removed
-        supportsSound: phetFeatures.supportsSound,
-        supportsEnhancedSound: phetFeatures.supportsEnhancedSound
+        supportsSound: phetFeatures.supportsSound && supportsAudio,
+        supportsEnhancedSound: phetFeatures.supportsEnhancedSound && supportsAudio
       },
 
       // configuration for controls in the "Input" tab of the PreferencesDialog

But don't like that supportsAudio is used for each field of audioOptions. Also, if someone passes options to PreferencesConfiguration that override audioOptions and don't consider audio=disabled this will happen again.

So I went with the above commit instead to remove audio controls from Preferences with audio=disabled. @zepumph can you please review this commit and the suggestion to remove the assertion in ParallelDOM?

[zepumph]

It seems like there is a setup already for handling the view, but this feels a bit more "modely" to me. Perhaps there is no spot currently, but there will be after #743.

Also, in my naive perspective, I would think you should use AudioManager.supportsAudio instead of checking on the query parameter.

What do you think?

[arouinfar]

So I went with the above commit instead to remove audio controls from Preferences with audio=disabled.

Hiding the Audio tab of the Preferences dialog when audio=disabled makes sense to me.

What happens if the only content left in a Preferences dialog is the blurb under General? The Preferences dialog would be essentially empty at that point. Should we hide the Preferences dialog/button completely in that situation?

[jessegreenberg]

I think you are correct. PreferencesConfiguration is essentially the model for this, do you prefer the first patch in #746 (comment)?

I would think you should use AudioManager.supportsAudio

You are right about this if we keep it in the view, just updated that part at least.

[zepumph]

PreferencesConfiguration is essentially the model for this,

That patch is a bit quirky, but I don't hate it, those features should already be listening to the global control in general. Presumably that is just demonstrating the underlying lack of model. Wouldn't that get better once we consolidate the config and control Properties into a PreferencesModel?

[zepumph]

The comment before the assertion is

// If you run into this assertion, talk to @jessegreenberg and @zepumph, because it is quite possible we would
// remove this assertion for your case.

@zepumph do you think we can remove this? I think one of the benefits of using alertDescriptionUtterance is that it could gracefully handle silencing alerts when the associated Node is not in the scene graph.

I would recommend keeping this assertion for now. Without it, there will be a no op when we would expect to see an utterance. This will be much more difficult catch without the assertion.

[jessegreenberg]

Wouldn't that get better once we consolidate the config and control Properties into a PreferencesModel?

OK sounds good, lets come back to this after #743. Adding the "on-hold" label.

I would recommend keeping this assertion for now. Without it, there will be a no op when we would expect to see an utterance.

OK sounds good.

[jessegreenberg]

What happens if the only content left in a Preferences dialog is the blurb under General? The Preferences dialog would be essentially empty at that point. Should we hide the Preferences dialog/button completely in that situation?

@arouinfar correct, it would only have the "General" tab. I hadn't thought of this case because the Preferences dialog is added as to sims as they get new features that would use it instead of across the board eagerly. But this didn't consider query parameters like this. If you would like to hide the button in cases like this we can do that. The only downside I can imagine is in development, I could see someone trying to add the NavigationBarPreferencesButton for the first time without content and wondering why the button isn't showing up. But some documentation would hopefully avoid confusion.

[arouinfar]

What happens if the only content left in a Preferences dialog is the blurb under General? The Preferences dialog would be essentially empty at that point. Should we hide the Preferences dialog/button completely in that situation?

@arouinfar correct, it would only have the "General" tab. I hadn't thought of this case because the Preferences dialog is added as to sims as they get new features that would use it instead of across the board eagerly. But this didn't consider query parameters like this. If you would like to hide the button in cases like this we can do that. The only downside I can imagine is in development, I could see someone trying to add the NavigationBarPreferencesButton for the first time without content and wondering why the button isn't showing up. But some documentation would hopefully avoid confusion.

It sounds like it could be messy and less dev-friendly to completely hide the dialog if query parameters reduce its contents to essentially nothing (General tab with just the default blurb). It's possible that this scenario isn't actually realistic. I looked through the sims with a11y features and Waves Intro is the only sim that supports sound without also supporting alt input (though it has partial support for alt input). If a sim supports alt input, it would likely have a Visual tab with the Interactive Highlights option, right? Moving forward, the goal is for sims to include both sound and alt input in their 1.0 releases (though this doesn't apply to sims currently nearing the finish line).

@jessegreenberg if you think audio=disabled is likely to create a functionally empty dialog for our users, I would hide the Preferences button. Otherwise, I don't know if it's worth the time and awkwardness for future devs.

[arouinfar]

@jessegreenberg can you please recommend which commits @jbphet should cherry-pick into BASE?

[jessegreenberg]

Yes, after reviewing I think both commits in this should be cherry-picked into a joist branch for BASE 1.5. That includes
ed6c7a6 and 034eacb. Ill also mention this in phetsims/balloons-and-static-electricity#543.

[jessegreenberg]

Now that we have a better PreferencesModel from #743 we can wrap this up. Though I am not exactly sure what is best. My thinking is that we need to move the check added in 034eacb into the PreferencesModel such that PreferencesModel.audioModel is accurately describes supported features.

Done in the above commit. @zepumph can you please look at this in conjunction with the changes from #743? Can this now be closed?

[zepumph]

Looks great! Thanks.