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

Changing locale can cause Preferences Dialog to change size #215

Closed
Nancy-Salpepi opened this issue Apr 4, 2023 · 5 comments
Closed

Changing locale can cause Preferences Dialog to change size #215

Nancy-Salpepi opened this issue Apr 4, 2023 · 5 comments
Assignees
Labels
type:bug Something isn't working type:wontfix This will not be worked on

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air (m1 chip)

Operating System
13.2.1

Browser
safari 16.3

Problem description
For phetsims/qa#925, when I cycle through the different locals in the Localization Tab, the size of the Preferences Dialog changes sometimes.

Steps to reproduce

  1. Go to the localization tab and click through all of the locale options

Visuals

dialogChangesSize.mp4
Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪papali ea nomoro‬ URL: https://phet-dev.colorado.edu/html/number-play/1.1.0-dev.10/phet/number-play_all_phet.html Version: 1.1.0-dev.10 2023-03-30 22:43:33 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.3 Safari/605.1.15 Language: en-US Window: 1254x705 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: {}
@Nancy-Salpepi Nancy-Salpepi added the type:bug Something isn't working label Apr 4, 2023
@zepumph
Copy link
Member

zepumph commented Apr 4, 2023

@chrisklus and I investigated this. In number compare we were able to just say supportsSound:false into the preferences, but because of game sounds, we can't do that here. (see phetsims/number-compare#19 for the original paper trail).

  • Next we tried to just get rid of the sound text since it didn't have a toggle, and that didn't work without a lot of jury rigging in the preferences.
  • Next we added a supportsSpeechSynthesis to preference model so that we could have a flag to make it show the sound toggle even without voicing, but then we found another downstream hard coding of audio=voicingOrSound in audioManager.anySubcomponentEnabledProperty. Some notes about the initial implementation over in Toggle switch or Sound for Voicing in preferences should be removed when only one audio feature is available joist#736.
  • This made us realize that in actuality, there should be a specific on/off switch for the speech synthesis preference, and the "hear aloud" sound should be inside that control. Maybe we could add the voice settings while we were at it.
  • We are totally stumped on this front! We will next pursue maxWidths on preferences.
Subject: [PATCH] update doc, https://github.com/phetsims/perennial/issues/269
---
Index: joist/js/preferences/PreferencesModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/preferences/PreferencesModel.ts b/joist/js/preferences/PreferencesModel.ts
--- a/joist/js/preferences/PreferencesModel.ts	(revision fad2ef5f9b5c8614e22f72eb200201575f0ed1cc)
+++ b/joist/js/preferences/PreferencesModel.ts	(date 1680640982502)
@@ -73,6 +73,10 @@
   // when running with English locales, accessibility strings are not made available for translation yet.
   supportsVoicing?: boolean;
 
+  // Different from voicing, which is a built in feature of PhET sims, this should be set to true for sims that
+  // have speech synthesis but aren't using voicing to accopmlish it.
+  supportsSpeechSynthesis?: boolean;
+
   // Whether to include checkboxes related to sound and extra sound. supportsExtraSound can only be
   // included if supportsSound is also true.
   supportsSound?: boolean;
@@ -230,6 +234,7 @@
       audioOptions: optionize<AudioPreferencesOptions, AudioPreferencesOptions, BaseModelType>()( {
         tandemName: AUDIO_MODEL_TANDEM,
         supportsVoicing: phetFeatures.supportsVoicing,
+        supportsSpeechSynthesis: false,
         supportsSound: phetFeatures.supportsSound,
         supportsExtraSound: phetFeatures.supportsExtraSound,
         customPreferences: []
@@ -272,12 +277,15 @@
                               phet.chipper.locale.startsWith( 'en' ) ||
                               ( phet.chipper.allowLocaleSwitching && _.some( localeProperty.validValues, value => value.startsWith( 'en' ) ) )
                             );
+    const supportsSpeechSynthesis = options.audioOptions.supportsSpeechSynthesis &&
+                            SpeechSynthesisAnnouncer.isSpeechSynthesisSupported();
 
     // Audio can be disabled explicitly via query parameter
     const audioEnabled = phet.chipper.queryParameters.audio !== 'disabled';
 
     this.audioModel = {
       supportsVoicing: supportsVoicing && audioEnabled,
+      supportsSpeechSynthesis: supportsSpeechSynthesis && audioEnabled,
 
       supportsSound: options.audioOptions.supportsSound && audioEnabled,
       supportsExtraSound: options.audioOptions.supportsExtraSound && audioEnabled,
Index: joist/js/audioManager.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/audioManager.ts b/joist/js/audioManager.ts
--- a/joist/js/audioManager.ts	(revision fad2ef5f9b5c8614e22f72eb200201575f0ed1cc)
+++ b/joist/js/audioManager.ts	(date 1680641658279)
@@ -69,6 +69,7 @@
     this.audioAndVoicingEnabledProperty = DerivedProperty.and( [ this.audioEnabledProperty, voicingManager.enabledProperty ] );
 
     this.anySubcomponentEnabledProperty = new DerivedProperty(
+      // TODO: but this doesn't respect non voicing speech synthesis
       [ soundManager.enabledProperty, voicingManager.enabledProperty ],
       ( soundEnabled, voicingEnabled ) => {
         return soundEnabled || voicingEnabled;
Index: joist/js/preferences/AudioPreferencesPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/preferences/AudioPreferencesPanel.ts b/joist/js/preferences/AudioPreferencesPanel.ts
--- a/joist/js/preferences/AudioPreferencesPanel.ts	(revision fad2ef5f9b5c8614e22f72eb200201575f0ed1cc)
+++ b/joist/js/preferences/AudioPreferencesPanel.ts	(date 1680640982511)
@@ -63,7 +63,7 @@
       // If only one of the audio features are in use, do not include the toggle switch to
       // enable/disable that feature because the control is redundant. The audio output should go
       // through the "Audio Features" toggle only.
-      const hideSoundToggle = audioModel.supportsVoicing !== audioModel.supportsSound;
+      const hideSoundToggle = ( audioModel.supportsSpeechSynthesis || audioModel.supportsVoicing ) !== audioModel.supportsSound;
 
       const soundPanelSection = new SoundPanelSection( audioModel, {
         includeTitleToggleSwitch: !hideSoundToggle
Index: number-play/js/number-play-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/number-play/js/number-play-main.ts b/number-play/js/number-play-main.ts
--- a/number-play/js/number-play-main.ts	(revision 297759ed289b16f5e2a402fbd6dfc816d9e704df)
+++ b/number-play/js/number-play-main.ts	(date 1680641013955)
@@ -55,7 +55,8 @@
           NumberPlayStrings.hearTotalStringProperty,
           NumberPlayStrings.hearTotalDescriptionStringProperty,
           NumberSuiteCommonPreferencesNode.hasScreenType( TenScreen ) || NumberSuiteCommonPreferencesNode.hasScreenType( TwentyScreen ) )
-      } ]
+      } ],
+      supportsSpeechSynthesis: true
     },
     localizationOptions: {
       includeLocalePanel: false,

@zepumph
Copy link
Member

zepumph commented Apr 4, 2023

We found that fixing #216 really solved this problem, given the above challenge, we feel like this is good enough. Most likely @Nancy-Salpepi will be able to find a case where some scaling occurs on locale switch, but probably we can't do anything about it for this release.

@Nancy-Salpepi please review on master.

@Nancy-Salpepi
Copy link
Author

Nancy-Salpepi commented Apr 5, 2023

There are a lot more locales in master, but if I cycle through the same ones that are in the dev version I don't see any difference with what I originally reported.
Based on your previous comments, I guess we are closing as won't fix? I will leave that to you.

@chrisklus
Copy link
Contributor

I was looking at this again last night and also did not see any improvement, contrary to what I observed on @zepumph's machine at some point yesterday. We will look back into it.

@zepumph
Copy link
Member

zepumph commented Apr 5, 2023

@chrisklus and I discussed and reproduced this once again, we opened phetsims/joist#918 and will close this issue as a wontfix for this publication.

@zepumph zepumph added the type:wontfix This will not be worked on label Apr 5, 2023
@zepumph zepumph closed this as completed Apr 5, 2023
@zepumph zepumph closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working type:wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants