Incorrect default sounds for selecting combo box items

[pixelzoom]

First reported in phetsims/my-solar-system#307 (comment) ...

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 #721, and I've addressed your TODO by added a link to that issue in 20b4c23.

[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 8b4861f 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?

9f93c29
20b4c23

[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: #861 (comment)

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

[zepumph]

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