phetsims / number-suite-common

"Number Suite Common" contains code for use by sims that are part of the Number Suite
GNU General Public License v3.0
0 stars 0 forks source link

"1,2,3" should be read when selecting a Language or Voice in Preferences. #56

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 13.2.1

Browser Safari

Problem description For https://github.com/phetsims/qa/issues/917, after slack discussions with @chrisklus, clarification is needed for this suggestion from https://github.com/phetsims/number-suite-common/issues/52#issuecomment-1434966437:

When a language or voice is selected on the Localization tab, it should always read something in that language and voice. Again, don't try to make it conditional based on which screen is selected, or specific to what would be read for the current screen. Always "say" sometime, regardless of screen.

Currently, it is set up so that "1,2,3" is heard when cycling through the voices for a particular locale and not when switching from one locale to another. Was that the desired behavior?

pixelzoom commented 1 year ago

No, that's not the desired behavior for the Preferences dialog (LanguageAndVoiceControl). When you change either Lanuguage or Voice, "1,2,3" should be read. From https://github.com/phetsims/number-suite-common/issues/52#issuecomment-1437575156:

... When a language or voice is selected for Second Language, it should always read something (like "one") in that language and voice. ... When a language or voice is selected on the Localization tab, it should always read something in that language and voice. ...

chrisklus commented 1 year ago

@pixelzoom can we talk about this issue after standup tomorrow? I think I'm on board but have some questions.

Here's a patch to start this change - it's going to take a bit more work to get this working correctly. With this patch, the sim reads out "One two three" when you initially open the Preferences dialog (and are not viewing the relevant tab). So we may need to pass in a tabVisibleProperty or something (if that exists) to this control as a guard.

```diff Index: js/common/view/LanguageAndVoiceControl.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/LanguageAndVoiceControl.ts b/js/common/view/LanguageAndVoiceControl.ts --- a/js/common/view/LanguageAndVoiceControl.ts (revision 09f8f7d251c54735c91c70baebddd61331801f2e) +++ b/js/common/view/LanguageAndVoiceControl.ts (date 1678842918937) @@ -116,11 +116,7 @@ ( locale, voices ) => { if ( voices.length ) { utteranceQueue.numberSuiteCommonAnnouncer.setFirstAvailableVoiceForLocale( locale, voiceProperty ); - - // When changing the voiceProperty in this control, we don't want to hear the speech data being read - // out, only the test voice. So clear the general utteranceQueue that may have been triggered by setting a voice - // above - utteranceQueue.clear(); + !!voiceProperty.value && utteranceQueue.testVoiceBySpeaking( voiceProperty.value, locale ); const availableVoicesForLocale = utteranceQueue.numberSuiteCommonAnnouncer.getPrioritizedVoicesForLocale( locale ); ```
pixelzoom commented 1 year ago

@pixelzoom can we talk about this issue after standup tomorrow?

My schedule is crazy fluid this week. But if I make it to standup, I'll be happy to discuss after.

chrisklus commented 1 year ago

Ah right, sorry I forgot. I'll write here when I have more time if you aren't around tomorrow.

chrisklus commented 1 year ago

I've thought about this more an am on board to proceed with the proposed implementation.

chrisklus commented 1 year ago

Thanks for this issue @Nancy-Salpepi! @zepumph and I fixed this in the above commit. @Nancy-Salpepi could you please test on phettest? Feel free to close if looking good. Thanks!

Nancy-Salpepi commented 1 year ago

On Mac + safari: With "Hear Number Sentence" on, as I cycle through locales, I sometimes hear the Compare Sentence when there is no available voice for that locale.

Example of Steps for Primary Language:

  1. Turn on "Hear Number Sentence" in the Audio Tab
  2. Go to the Localization Tab and select a different locale that you know has available voices (ex. español) --you will hear 1,2,3
  3. Select a locale that you know doesn't have a Voice (ex. Faroese)--you will hear the compare statement

Example of Steps for Secondary Language:

  1. Turn on "Hear Number Sentence" in the Audio Tab
  2. In the Simulation Tab, turn on "Second Language"
  3. On the Compare Screen, switch to the second language (which is probably still English as this point)
  4. In the Simulation tab, select a different locale that you know has available voices (ex. español) --you will hear 1,2,3
  5. Select a locale that you know doesn't have a Voice (ex. Faroese)--you will hear the compare statement

https://user-images.githubusercontent.com/87318828/227001738-1dabeaa8-2bfc-4247-8178-142dbbcf283b.mov

chrisklus commented 1 year ago

@Nancy-Salpepi and I looked at this on my machine and were not able to reproduce what @Nancy-Salpepi was seeing in https://github.com/phetsims/number-suite-common/issues/56#issuecomment-1480063879.

What was described in that issue was definitely a bug during an interim state of fixing this issue but then @zepumph and I did a refactor that should not make it possible to trigger the comparison statement while in the preferences menu, so I was hoping that the test above was from a cached phettest. @Nancy-Salpepi is going to check again on her end, thanks :)

Nancy-Salpepi commented 1 year ago

@chrisklus you might find this bizarre, but I still see this issue in Number Compare. I do not see it in Number Play.

chrisklus commented 1 year ago

I am also seeing that and it is blowing my mind! In Chrome is it not consistently broken for me for Number Compare, but in Safari it appears to be.

chrisklus commented 1 year ago

@zepumph and I spent a good amount of time on this issue yesterday and ended up doing some more refactoring in side issues to support this work.

The reason the problem was only occurring in Number Compare is because when a locale is selected, the comparison string changes (even if the numbers didn't, just other words change) and so the data property was changing for the sim and triggering a read out. The reason this wasn't happening in Number Play is because the data there is ONLY the number of the screen, which is represented as the same numerical quantity for all languages. So changing the language doesn't trigger a data change readout.

@zepumph recommended investigating a way to not trigger a readout from a Property change, but instead focus on user input. This is how we did some restructuring for the voiceProperty over in https://github.com/phetsims/number-suite-common/issues/60.

I eventually gave up on this approach and just fixed this with a one line change in LanguageAndVoice control. The change was to cancel the utterance that happens from changing the locale in that control. This is the ONLY place that a change to the model happens where we don't want to read aloud, and feels more idiomatic with how this feature works. There are a large number of inputs that can trigger a read aloud, and so switching to an opt-in mentality for that feels wrong. I prefer having the model Property cover every case and then opting out of one of those.

@Nancy-Salpepi can you please test on phettest? For this latest issue and general 1,2,3 behavior since some underlying things changed in https://github.com/phetsims/number-suite-common/issues/62. Thanks! Feel free to close if looking good.

Nancy-Salpepi commented 1 year ago

This looks good in Number Compare 🎉. Closing!