Incorrect default sounds for selecting combo box items

[pixelzoom]

First reported in https://github.com/phetsims/my-solar-system/issues/307#issuecomment-1845528651 ...

The default sounds for selecting combo box items is a set of tones that decreases in pitch from top to bottom of the listBox. The relevant code in ComboBoxListBox is:

    // Make a list of sound generators for the items, using defaults if nothing was provided.
    const itemSelectedSoundPlayers = items.map( item => {
      return item.soundPlayer ?
             item.soundPlayer :
             multiSelectionSoundPlayerFactory.getSelectionSoundPlayer( items.indexOf( item ) );
    } );

Comparing main to published version for a few sims (my-solar-system, geometric-optics,...) shows that this is no longer behaving as expected. In main, the same tone is played for every item.

Changing the above code to this (below) results in no change; still the same tone for every item.

    // Make a list of sound generators for the items, using defaults if nothing was provided.
    const itemSelectedSoundPlayers = items.map( item => {
      return multiSelectionSoundPlayerFactory.getSelectionSoundPlayer( items.indexOf( item ) );
    } );

multiSelectionSoundPlayerFactory.getSelectionSoundPlayer seems to be behaving as expected in other contexts - e.g. AquaRadioButtonGroup, RectangularRadioButtonGroup.

I have not investigate whether this problem has been published in any sims. I guess we'll need to do that when we learn when the problem was introduced.

This blocks creation of the my-solar-system 1.3 release branch, and we've just completed dev testing. So this is high priority. @jbphet volunteered to investigate - thanks!

[jbphet]

This problem has appeared because the combo box list box is being closed before the value of the managed property is set. The sound generation code for the closing (i.e. going invisible) for the list box used to look like this:

        // sound generation - assumes that the property value has been updated before this list box is hidden
        assert && assert( selectionWhenListBoxOpened !== undefined, 'no value for property when list box opened' );
        if ( selectionWhenListBoxOpened === property.value ) {
          options.closedNoChangeSoundPlayer.play();
        }
        else {
          const indexOfSelection = items.findIndex( item => item.value === property.value );
          itemSelectedSoundPlayers[ indexOfSelection ].play();
        }

Note the bit about it assuming 'the property value has been updated before this list box is hidden'. Somewhere along the way that assumption became incorrect. This commit is a good possibility for when that changed.

The tricky part about addressing this is that there are quite a number of different places where the list box is hidden. Fortunately, there only seems to be a single code path where the box is hidden when the user has selected something new, so I've added a variable to track the selection when that occurs that gets set before the list box is closed. The sound generation code now uses that variable to decide what sound to play.

@pixelzoom - Please review the change. As part of this, please address the TODO I've added about whether the documentation I added about why the Property can't be set before calling focusButtonCallback() is correct.

[pixelzoom]

@pixelzoom - Please review the change. As part of this, please address the TODO I've added about whether the documentation I added about why the Property can't be set before calling focusButtonCallback() is correct.

Changes looks good, and default sound is now behaving as expected when selecting items in the listBox. I tested in my-solar-system, density, and fourier-making-waves.

Regarding the TODO... Yes, you were correct that you need to need to wait to set property.value until after focus is set. That was discovered in https://github.com/phetsims/sun/issues/721, and I've addressed your TODO by added a link to that issue in https://github.com/phetsims/sun/commit/20b4c237aea470fd7c1453e79324f87ac95040d3.

[pixelzoom]

This issue no longer blocks publication of my-solar-system 1.3, so removing the blocks-publication label.

Now we need to figure out which sims to MR. Above, @jbphet identified https://github.com/phetsims/sun/commit/8b4861fd80122b2026b92815763b65222a74c96b as a likely candidate for when this regression occurred, which was Oct 4, 2021. That's > 2 years. So I think it would easist to just MR all published PhET-iO sims tht have a ComboBox, or maybe even just MR all published PhET-iO sims. My understanding is that this MR will be rolled into https://github.com/phetsims/phet-io/issues/1974, so assigning to @zepumph to decide how to proceed.

[zepumph]

Should I MR both commits?

https://github.com/phetsims/sun/commit/9f93c2988f938b654ae89296a8811ea71645e3f2 https://github.com/phetsims/sun/commit/20b4c237aea470fd7c1453e79324f87ac95040d3

[pixelzoom]

I suspect so. But @jbphet is the person to ask. I was the reviewer.

[jbphet]

The 2nd commit is just a documentation change, so it is not strictly necessary for the correct functionality, but it does improve the comments in the code. If it is not significantly more difficult, please include both.

[zepumph]

RE: https://github.com/phetsims/sun/issues/861#issuecomment-1850527763

This issue no longer blocks publication of my-solar-system 1.3, so removing the blocks-publication label.

This means that I do need to match up MSS 1.3 in this issue. Please tell me if that isn't true.

[pixelzoom]

The commits in https://github.com/phetsims/sun/issues/861#issuecomment-1851084862 were made on 12/8/23. my-solar-system 1.3 branch was created on 12/11/23. So no, you do not need to apply these shas to my-solar-system 1.3.

[zepumph]

The commits in #861 (comment) were made on 12/8/23. my-solar-system 1.3 branch was created on 12/11/23. So no, you do not need to apply these shas to my-solar-system 1.3.

Hmmm. That timeline checkouts out, but these commits aren't on the MSS 1.3 branch. The fix came in on a friday, and the RC was made on that monday. Perhaps main wasn't fully pulled before making the branch?

https://github.com/phetsims/sun/commits/cd7f97d50fc7298c6f82f0f473b6f7bb1b535f87/

Anywho, I'll add this to MSS now. I'm running into trouble proceeding with https://github.com/phetsims/sun/issues/862 because it requires these changes first.

[zepumph]

Added. We are ready for testing.

[zepumph]

For testing in QA:

• Confirm in any runtime that interacting with a ComboBox has the sound design. The listbox should have differing sounds (higher sound for items earlier in the list) upon selection.

[zepumph]

This was confirmed fixed by QA for all MR sims with a Combo Box. Closing