Should I hear a sound when jumping to min/max?

[Nancy-Salpepi]

Test device
MacBook Air M1 chip

Operating System
14.5

Browser
Chrome

Problem description
phetsims/qa#1100, on all screens when I jump to the min/max of the number spinners there is no sound.
There is a sound when I jump to the min/max of a slider. I know that these are UI sounds that were "turned on" for the sim, so if this is scope creep than feel free to ignore!

Steps to reproduce
On any screen, tab to a number spinner and press the home and end keys.

[pixelzoom]

Reproduced in main, this affects all sims that have NumberSpinner and FineCoaseSpinner.

NumberSpinner and FineCoaseSpinner are common-code components that extends AccessibleNumberSpinner, which extends AccessibleValueHandler. @jessegreenberg was it an intentional part of the design not to have Home/End sounds for these UI components, or is this an oversight?

[pixelzoom]

@jessegreenberg is out this week, so this may be blocked until 7/8.

[arouinfar]

@pixelzoom I'm inclined to think this is an oversight. There's a possible paper trail in phetsims/fourier-making-waves#192 and phetsims/sun#697 that I'm still looking through.

[pixelzoom]

Summary from Slack#design-sims (see below) is that we should add sound for the Home/End buttons, and use the same sound as for the arrow buttons. I'll investigate how to do this.

Slack#design-sims

@pixelzoom Alt input question for designers… In #281, it was reported that (unlike sliders) spinners do not make a sound when using the Home/End keys to jump to max/min. Was this an intentional part of the design, or is this an implementation oversight (bug)?

@arouinfar Does NumberSpinner use the slider sound API?

@pixelzoom No. Slider extends AccessibleSlider. NumberSpinner extends AccessibleNumberSpinner.
AccessibleSlider and AccessibleNumberSpinner both extend AccessibleValueHandler.

@pixelzoom I don’t believe there’s any problem with the class hierarchy. But I believe that the Home/End key may not have been considered for AccessibleNumberSpinner.

@pixelzoom Also note that (unlike sliders) spinners make the same sound for increment & decrement. And would presumable use the same sound for Home/End. The issue now is that there is no sound for Home/End.

@arouinfar I found a similar bug report in Fourier, which is one of the first sims to have UI sound. I could not find any issues in sun or tambo about NumberSpinner sound. Since sims with UI sound only do not get tagged on the website, and I don’t have the ability to search all sim package.json, it’s difficult to find a paper trail for this sort of thing.

@pixelzoom @jessegreenberg is out this week, so I can’t ask about the design and implementation.
Also… I’d prefer not to get into redesigning sound for spinners. I’d just like to know if there should be sound for Home/End (I think yes) and if it’s OK to use the same sound as the increment/decrement arrows. (edited)

@arouinfar agree, redesigning is out of scope, so reusing the increment/decrement sound for home/end seems reasonable.

[pixelzoom]

I've discovered that sound for NumberSpinner and FineCoarseSpinner is not handled by AccessibleNumberSpinner. Those classes actually press the buttons when arrow keys are pressed, which results in the buttons making sounds. For example in NumberSpinner:

    // pdom - NumberSpinner uses AccessibleValueHandler for accessibility, but it was decided that keyboardStep
    // and shiftKeyboardStep should have the same behavior as the NumberSpinner ArrowButtons AND the ArrowButtons
    // should look depressed when interacting with those keys. To accomplish this we actually press the ArrowButtons
    // in response to input with those keys. keyboardStep and shiftKeyboardStep are set to zero so the value isn't
    // modified again by AccessibleValueHandler. See https://github.com/phetsims/scenery/issues/1340.
    assert && assert( options.keyboardStep === undefined, 'NumberSpinner sets keyboardStep, it will be the same as deltaValue' );
    assert && assert( options.shiftKeyboardStep === undefined, 'NumberSpinner sets shiftKeyboardStep, it will be the same as deltaValue' );
    assert && assert( options.pageKeyboardStep === undefined, 'NumberSpinner sets pageKeyboardStep, it should not be used with NumberSpinner' );
    options.keyboardStep = 0;
    options.shiftKeyboardStep = 0;
    options.pageKeyboardStep = 0;
...
    // pdom - click arrow buttons on press of arrow keys so that the Property value changes by deltaValue
    // and the buttons look depressed
    const increasedListener = ( isDown: boolean ) => ( isDown && incrementButton.pdomClick() );
    const decreasedListener = ( isDown: boolean ) => ( isDown && decrementButton.pdomClick() );
    this.pdomIncrementDownEmitter.addListener( increasedListener );
    this.pdomDecrementDownEmitter.addListener( decreasedListener );

[pixelzoom]

Moving the general issue of adding Home/End sounds to phetsims/sun#886. This issue is blocked until that is resolved.

[pixelzoom]

To unblock this issue, I created sim-specific subclasses of NumberSpinner and FineCoarseSpinner that add sound for the Home/End buttons, see 280850d. Those subclasses will need to be removed when this problem has been addressed generally in phetsims/sun#886, as I've noted in that issue and a TODO code comment.

[pixelzoom]

@Nancy-Salpepi please review, close if OK. Home/End should make the same sound left/right/up/down keys.

[Nancy-Salpepi]

I now hear sounds when jumping to min/max.

One oddity I see is if I am already at the max value and press the up or right arrow I won't hear a sound, but if I press the End key I will still get a sound. The same things happens when at the min value.

[pixelzoom]

@Nancy-Salpepi said:

One oddity I see is if I am already at the max value and press the up or right arrow I won't hear a sound, but if I press the End key I will still get a sound. The same things happens when at the min value.

Discussed with @Nancy-Salpepi ... To clarify, we're hearing the Home/End sounds when we're already at max/min. That's undesirable, and not like (for example) Slider.

Unfortunately, I can’t check the value to see if we’re at max/min already, because NumberSpinner (AccessibleValueHandler probably) is setting the value to max/min before my workaround code is called. I don’t think that’s worth holding things up. So we will live with this workaround. I’ll make a note to handle it properly in phetsims/sun#886. And I'll close this issue.