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

How should Voicing work with the new locale switching feaure? #853

Closed jessegreenberg closed 1 year ago

jessegreenberg commented 1 year ago

Voicing is only supported in English and the feature and its controls are hidden when sims are running with other languages. How should Voicing behave with the new locale switching feature? If we do nothing, the output of Voicing will be strange. Voicing only includes English voices. Accessibility uses a mix of translatable and non-translatable strings. So Voicing output will be a mix of English and non-English strings, all spoken with English pronunciation.

We could disable Voicing when a non-English locale is selected. Or disable and hide all related controls. We might need to add visible text somewhere that describes that it is only available in English. We could disable and hide all voicing controls when locale switching feature is supported? I am not sure how locale switching is getting deployed, but this would be problematic if locale switching becomes a standard feature.

This blocks the publication of RaP and Friction and will inform how we handle https://github.com/phetsims/joist/issues/852.

jessegreenberg commented 1 year ago

I think we should disable and hide Voicing and controls when the locale is switched from english. That matches previous expectations for the feature. The downside is that the Preferences Dialog could resize when changing languages. Benefit is that all these strings related to Voicing controls in Preferences will not be available to translators. Since the feature doesn't work in other languages, it could be confusing for them to see these strings.

terracoda commented 1 year ago

Excellent points @jessegreenberg, I agree, I think Voicing needs to be disabled when the language is switched to something other than English.

zepumph commented 1 year ago

Sounds fine to me, though it may increase technical debt in the future when we flip the switch on translating these.

jessegreenberg commented 1 year ago

Thanks all, I made some simple changes to get started. Voicing is now disabled and the Voicing section is hidden when locale is changed from English. If Voicing is enabled when this happens, we hear the same "Voicing off" response from the "Voicing" toggle switch (which we got for free from existing listeners).

@zepumph raised some good questions, I sent a demo over slack of this behavior and reached out to design to ask if those or other items are needed.

terracoda commented 1 year ago

Dynamic name/label for Voicing toggle switch

And it still might make sense to say why Voicing gets turned off if a learner chooses Voicing before choosing their language. If a third response is possible, I would suggest:

jessegreenberg commented 1 year ago

OK thanks @terracoda. Added both of those in the above commits. "Voicing (English Only)" is the new label when there are more locales available. The response for changing locale includes "Only available with English".

@zepumph I think this is done, over to you to review if you wish and confirm Friction/RaP are no longer blocked.

zepumph commented 1 year ago

There are other locales that are english:

https://github.com/phetsims/chipper/blob/55f01f582ff42427c5b2f12a070edbe0c47f0706/js/data/localeInfoModule.js#L223-L237

Do we want to support all of them by using 'startsWith' instead of triple equals?

Also, It is almost certainly possible to make the HBox respect the bounds of the invisible voicing section. I couldn't figure it out in 2 minutes of playing, but is that something we would like so that the language selector isn't quite so awkward, re-laying-out when switching from English?

jessegreenberg commented 1 year ago

Thanks for reviewing!

support all of them by using 'startsWith' instead of triple equals?

Great idea! Done above.

Also, It is almost certainly possible to make the HBox respect the bounds of the invisible voicing section.

I see what you mean. I found one way to accomplish this with excludeInvisibleChildrenFromBounds: false. The VBox to the right of the Voicing content does not shift to the left and I think that is correct. But it probably could if you prefer. Does this look better?

emily-phet commented 1 year ago

@jessegreenberg Thanks for the video in Slack, it was very helpful. I think it's a little unfortunate to have the voicing feature disappears from the audio tab when a new locale is chosen, rather than having it disabled. But, I recognize it would be a bit awkward to either have the Voicing content present in English, or to have a translated label for something available only in English.

I'm not sure if this was suggested, but has it been considered having the label for Voicing remain present and disabled, but perhaps all the sub options disappear when a new locale is selected?

Related, I'm not sure what the conclusion was re voicing behavior if voicing is on, then selection of a new locale turns it off, and then an English language locale is selected again...would voicing return or not? If it returns, having the Voicing feature label always stay present even if disabled could be the most conceptually consistent choice. If it stays off, then perhaps that's more aligned with having Voicing disappear as a feature when a non-English language locale is selected, and then if English is selected later, the feature re-appears, but since it was "gone" and now "back" not have it enabled.

zepumph commented 1 year ago

I like everything I heard above a lot. Especially having voicing enabled again when returning but seeing a greyed out switch in the on position.

Also @jessegreenberg, is there anything in the code that could possible re-enable voicing when the localeProperty is off? I believe all those controls will be hidden, but it seems a bit hard to see all cases.

jessegreenberg commented 1 year ago

Thanks @emily-phet and @zepumph.

I'm not sure if this was suggested, but has it been considered having the label for Voicing remain present and disabled, but perhaps all the sub options disappear when a new locale is selected?

Not yet, this could be done.

would voicing return or not?

Currently it does not, but it could. I think the behavior you described where making the "Voicing" label invisible when it does not return matches what is implemented. If it would be better to have Voicing return in this case, we could keep the label visible. Which do you prefer?

but seeing a greyed out switch in the on position.

If Voicing is not enabled, I don't think the Voicing switch should be in the "on" position. It would also be difficult to do this since the PreferencesToggleSwitch is tied to the SpeechSynthesisAnnouncer.enabeldProperty.

could possible re-enable voicing when the localeProperty is off?

Sorry, what does "off" mean for the localeProperty?

zepumph commented 1 year ago

Sorry, I meant to say "could possible re-enable voicing when the localeProperty caused it to turn off originally and then was set back to en".

jessegreenberg commented 1 year ago

Thanks! I understand now, I believe it should be. Ill take a look.

jessegreenberg commented 1 year ago

In the above commit, I changed it so

I recorded another demo and posted on slack to get votes on which version to use.

pixelzoom commented 1 year ago

Please regenerate all PhET-iO API files. ph-scale-basics is failing in CT. By running grunt generate-phet-io-api, I can see that no PhET-iO API files were updated, and other sims will fail similarly.

Now that strings are PhET-iO instrumented, adding a string for a sim requires regenerating that sim’s API file. Adding a common-code string (like you’ve done in JoistStrings.ts) requires regenerating all API files.

terracoda commented 1 year ago

@jessegreenberg, I liked the demo in slack where the Voicing switch and help text were left disabled. The responses sound good, too.

jessegreenberg commented 1 year ago

Please regenerate all PhET-iO API files. ph-scale-basics is failing in CT.

This was done for all sims in https://github.com/phetsims/phet-io-sim-specific/commit/131a41d9431460b6205a77ff708b370737945cbb. grunt compare-phet-io-api shows no diferences in any sim in perennial/data/phet-io-api-stable.

jessegreenberg commented 1 year ago

From slack and above comments, the design in https://github.com/phetsims/joist/issues/853#issuecomment-1241322613 is the one we will go with.

There are two more changes to make discussed over slack.

jessegreenberg commented 1 year ago

The items in https://github.com/phetsims/joist/issues/853#issuecomment-1244085194 were done in the above commits. @zepumph could you please review this? Sorry, I know you are currently out but you did the first review and expressed thoughts about how this should behave over slack. I think it can wait for your return and is only blocking for Friction/RaP publications.

EDIT: I forgot I need to regenerate APIs since this changed string files. Commit for that coming up.

zepumph commented 1 year ago

Looks great, thanks!