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

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

Closed
jessegreenberg opened this issue Sep 2, 2022 · 20 comments
Closed

Comments

@jessegreenberg
Copy link
Contributor

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 #852.

@jessegreenberg
Copy link
Contributor Author

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
Copy link
Contributor

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

@terracoda terracoda removed their assignment Sep 2, 2022
@zepumph
Copy link
Member

zepumph commented Sep 6, 2022

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

  • Should we voice something when we disable it, like "Sim voicing off, only supported in English."
  • What happens when we have voicing on, switch locales, and then come back to english? Do you need to re-enable voicing?
  • What happens when one person reaches out to us with, "I know English as a second language, and I would like to see my primary language as much as possible but also want voicing, even if only in English."

@jessegreenberg
Copy link
Contributor Author

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
Copy link
Contributor

terracoda commented Sep 7, 2022

Dynamic name/label for Voicing toggle switch

  • When no additional languages are available in the Preferences Menu
    image

  • When additional languages are made available in the Preferences menu
    image

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:

  • Voicing off. Only available with English.

@jessegreenberg
Copy link
Contributor Author

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
Copy link
Member

zepumph commented Sep 8, 2022

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
Copy link
Contributor Author

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?

@zepumph
Copy link
Member

zepumph commented Sep 8, 2022

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
Copy link
Contributor Author

jessegreenberg commented Sep 8, 2022

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
Copy link
Member

zepumph commented Sep 8, 2022

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".

@zepumph zepumph removed their assignment Sep 8, 2022
@jessegreenberg
Copy link
Contributor Author

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

@jessegreenberg
Copy link
Contributor Author

In the above commit, I changed it so

  • If locale changes while Voicing is enabled, the next time "en" locale is selected Voicing will be enabled again.
  • The "Voicing" label of the panel section remains visible, but is disabled when locale is not english. Voicing is disabled and subcomponents are hidden.

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

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 9, 2022

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
Copy link
Contributor

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

@jessegreenberg
Copy link
Contributor Author

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
Copy link
Contributor Author

From slack and above comments, the design in #853 (comment) is the one we will go with.

There are two more changes to make discussed over slack.

  • The visual Voicing label and description should be translatable.
  • We should make sure that if the sim starts in a non-english language but English is an available locale, Voicing should be available for use (when English is selected).

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Sep 21, 2022

The items in #853 (comment) 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
Copy link
Member

zepumph commented Oct 4, 2022

Looks great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants