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

audio=disabled hides navbar icon but doesn't disable audio #543

Closed
Tracked by #721
arouinfar opened this issue Sep 17, 2021 · 10 comments
Closed
Tracked by #721

audio=disabled hides navbar icon but doesn't disable audio #543

arouinfar opened this issue Sep 17, 2021 · 10 comments
Assignees
Labels

Comments

@arouinfar
Copy link
Contributor

For phetsims/qa#706

Steps to reproduce

  1. Run 1.5.0-rc.1 with audio=disabled
  2. Open Preferences dialog and select Audio tab
  3. Turn on Audio Features toggle
  4. Sim now makes sound and there is no way to mute it (short of muting the browser tab)

In master, the sim crashes if you try turn on Audio Features with audio=disabled and this error appears in the console:

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

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: {}

@arouinfar
Copy link
Contributor Author

The bug in master has been logged in phetsims/joist#746.

@jbphet
Copy link
Contributor

jbphet commented Oct 1, 2021

I'm not sure that I agree that the described behavior is problematic. Above, @arouinfar said:

Sim now makes sound and there is no way to mute it

There actually is a way to mute it, and that it to turn audio back off by using the same control that the user would have had to use to turn it on. If they turned it off, they would have to be aware that this control exists.

@arouinfar - Back to you for your thoughts on this. If you think the behavior is suboptimal, let's set up a design meeting slot to discuss it with other members of the design team, and please include @emily-phet and @jessegreenberg.

There was a bunch of work on this done under phetsims/joist#724. I started looking through it to figure out if the current behavior was ever explicitly specified, but it's a long issue and I didn't want to take the time to work through it unless necessary.

As an aside, and for what it's worth, the bug in master also exists on the 1.5 branch, it just doesn't show up in the built version because asserts are stripped out, and the code seems to handle it okay.

@jbphet jbphet assigned arouinfar and unassigned arouinfar Oct 1, 2021
@jbphet
Copy link
Contributor

jbphet commented Oct 1, 2021

Oh, and @arouinfar - we're hoping to the next RC on the 1.5 branch very soon, so if you think that this should block that release, please say so right away.

@jessegreenberg
Copy link
Contributor

The common code issue for this is phetsims/joist#746. Commits have been made to fix this, and I would recommend cherry picking these into a joist branch for the BASE 1.5 release.
phetsims/joist@ed6c7a6 and phetsims/joist@034eacb.

With these commits the "Audio" tab in the Preferences dialog won't be shown when the audio=disabled query parameter is used.

phetsims/joist#746 is still open to work on a longer term improvement but these two commits have been reviewed.

@arouinfar
Copy link
Contributor Author

Thanks @jessegreenberg!

The current behavior in the 1.5 branch is buggy. Using audio=disabled should truly disable all audio in the sim. In phetsims/joist#746 we decided that the navbar icon and the Audio tab of the Preferences dialog should be hidden with audio=disabled. This behavior is currently in master and seems to be working well.

The longer-term improvement that @jessegreenberg referenced will still have the same behavior for the end-user, which is why I recommended cherry-picking the interim solution into the 1.5 branch.

@jbphet
Copy link
Contributor

jbphet commented Oct 1, 2021

Thanks @arouinfar and @jessegreenberg. You're right, it's a much better solution to have the audio disappear from the "Preferences" menu when the audio=disabled parameter is used. I've moved these changes onto the 1.5 branch and will make sure this is tested in the next RC.

@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Oct 18, 2021

Using MacBook Air (m1 chip) iOS 11.6 + safari 15:
With audio=disabled, I can still hear a sound with the first object I touch. The sound is much lower, but it is still there. Once that sound is produced, sound is disabled.
I didn't see this issue with Mac + chrome.

Here is an example with the reset button. You may need to turn your volume all the way up to hear it.

sound.mov

@jbphet
Copy link
Contributor

jbphet commented Oct 18, 2021

I think I know what is causing the behavior that @Nancy-Salpepi describes above. I'm going to make an executive decision here and say that this is non-blocking to the 1.5 release of BASE, and that we should consider this issue fixed and file a separate common-code issue about the short sound when muted.

@Nancy-Salpepi - Can you please create a separate issue in the tambo repo that describes the problem from the previous comment and includes the video clip? Then, if there are no other problems related to audio=disabled, add a comment to that effect and assign this back to me.

@Nancy-Salpepi
Copy link

No additional issues seen with audio=disabled.

@jbphet
Copy link
Contributor

jbphet commented Oct 19, 2021

Excellent, thanks. Closing.

@jbphet jbphet closed this as completed Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants