search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Default sound for ComboBoxListBox does not supports dynamic order and visibility of items. #862

Closed pixelzoom closed 7 months ago

pixelzoom commented 7 months ago

Discovered while working on https://github.com/phetsims/sun/issues/861 …

The default sound design for ComboBoxListBox assigns sound with descending pitch. This is done in the constructor, and is based the order of the items argument.

ComboBoxListBox does not consider whether items are initially visibile Items are assigned a sound regardless of visibility. This is a problem with sims like My Solar System, where orbitalSystemComboBox.listBox contains 4 presets that are initially invisible, designed to be configured and made visible only by PhET-iO clients.

PhET-iO allows you to change the visibility of items, and the order of items (via the Up/Down UI in Studio). Sounds are not reassigned as visibility and order are changed. So a custom wrapper can end up breaking the sound design — no correlation between pitch and an item’s position in the listBox.

This is a problem in any published PhET-iO sim that has a ComboBox and supports UI sound. Since that includeds many sims, we will not consider this blocking for My Solar System 1.3, and will address it in whatever MR addresses other sims.

pixelzoom commented 7 months ago

Assigning to @zepumph @brent-phet and @kathy-phet to decide whether this needs to be rolled into https://github.com/phetsims/phet-io/issues/1974.

Assigning to @jbphet to investigate a UI sound solution that supports dynamic order and visibility of listBox items.

zepumph commented 7 months ago

It seems most important to have a fix on main. Then we can see how straight forward it would be to roll into the PhET-iO MR. @brent-phet let me know if you want to discuss anything else to determine priority.

zepumph commented 7 months ago

I'm keeping track of everything for my MR in https://github.com/phetsims/phet-io/issues/1974#issue-1997765227. Until told otherwise, I have noted that we aren't adding this to the MR, let me know if/when that changes.

jbphet commented 7 months ago

I've implemented a fix for this and reviewed it with @pixelzoom, who helped to improve it substantially. Unfortunately, it is comprised of two commits because I found an issue after committing it.

@pixelzoom and I tested removing and reordering list box items in My Solar System and verified that the behavior for the default sounds is correct. We also added tested adding a custom sound for an item and verified that it moved with the item. I also regression tested on other sims that use combo boxes.

Since @pixelzoom has already been through the code he feels like it has been adequately reviewed. Turning over to @zepumph for possible inclusion in an upcoming maintenance release.

pixelzoom commented 7 months ago

@zepumph note that this fix DOES need to be patched into my-solar-system 1.3, which is currently in RC testing.

zepumph commented 7 months ago

Added to all branches, ready for testing.

zepumph commented 7 months ago

To test this for QA:

  1. Go to studio
  2. Hide a good number of combo box selection
  3. Move around both visible and non visible items in the list box order.
  4. Confirm that the sound design is still correct. In which items at the top of the list have a higher pitch, and lower items and a lower pitch, and that they change in equal increments (upon selection). The visibility and order of items should not effect that.
zepumph commented 7 months ago

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