search

phetsims / sun

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

NumberSpinner and FineCoarseSpinner do not make sound when using the Home and End keys. #886

Closed pixelzoom closed 2 weeks ago

pixelzoom commented 1 month ago

Discovered in https://github.com/phetsims/gas-properties/issues/281 ...

NumberSpinner and FineCoarseSpinner do not make sound when using the Home and End keys. They both extend AccessibleNumberSpinner, which provides none of the sound implementation. Instead, those classes actually press the arrow 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 );

Adding input listeners for Home and End to NumberSpinner and FineCoarseSpinner seems like the most straighforward approach. But a better solution might be something like SliderOptions soundGenerator and valueChangeSoundGeneratorOptions.

pixelzoom commented 1 month ago

High priority because this blocks Gas Properties RC 1.1.

pixelzoom commented 1 month ago

To unblock https://github.com/phetsims/gas-properties/issues/281 for RC, I created sim-specific subclasses of NumberSpinner and FineCoarseSpinner that add sound for the Home/End buttons, see https://github.com/phetsims/gas-properties/commit/280850da784b3695b43134c20f06716b3f4d1801. When this issue has been addressed, you'll need to remove those sim-specific subclasses. You can find them by searching for TODO https://github.com/phetsims/sun/issues/886.

pixelzoom commented 1 month ago

As noted in https://github.com/phetsims/gas-properties/issues/281#issuecomment-2207110945 ... Like Slider, we should NOT hear the Home/End sound when the value is already at max/min.

Nancy-Salpepi commented 3 weeks ago

This is also seen in https://github.com/phetsims/qa/issues/1110 when jumping to min/max number of loops.

pixelzoom commented 3 weeks ago

This has now been reported for the Gas Properties suite, FEL suite, and MSaB. In Gas Properties, I resorted to the workaround described in https://github.com/phetsims/gas-properties/issues/281#issuecomment-2207001340:

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

If we have to do this for FEL (https://github.com/phetsims/faradays-electromagnetic-lab/issues/193) and MSaB (https://github.com/phetsims/mean-share-and-balance/issues/313) that's more sim-specific workaround, more technical debt to undo when this is addressed generally in common-code.

pixelzoom commented 3 weeks ago

Posted to Slack#planning:

For the next iteration… Spinners are currently missing sounds for the Home/End buttons. This has now been reported by QA for at least 3 sims. Should we continue to apply sim-specific workarounds (which will need to be removed later) or should we prioritize addressing https://github.com/phetsims/sun/issues/886 ?

jessegreenberg commented 3 weeks ago

I should be able to get to this this week. This will be a nice enhancement but IMO is not worth adding sim-specific workarounds.

jessegreenberg commented 2 weeks ago

This was done in the above commits. To summarize:

@jbphet @marlitas can you please review? Note the new duplication between NumberSpinner and FineCoarseSpinner. I considered moving some code into AccessibleNumberSpinner or AccessibleValueHandler to factor it out but decided against it because

marlitas commented 2 weeks ago

The onInput documentation starts with // Called after any user input onto this component. That makes me wonder how/when endInput is called. I'm probably just not understanding the relationship of those three options completely.

Other than that I looked through the code and the new option is thoroughly documented and well implemented. The duplication is unfortunate, but upon further examination fully understood and I think it is the right choice.

Back to @jessegreenberg to wrap up.

jessegreenberg commented 2 weeks ago

Thanks for reviewing @marlitas. I removed some old documentation that was more confusing than helpful. Hopefully that is more clear.

@pixelzoom you should be able to remove the workaround code pointing to this issue, can you confirm?

@samreid I think you will have to keep the code in SelectorNode, or replace it with something like this: https://github.com/phetsims/scenery-phet/commit/9790871cf7f8cab227bc3c5fc7081ee39840dbd8.

samreid commented 2 weeks ago

@jessegreenberg can you please elaborate why the SelectorNode code may remain necessary, and why it will not play an extra sound? Do you recommend keeping it or the replacement you identified above? Please advise.

jessegreenberg commented 2 weeks ago

Yes, the new sound code was added to NumberSpinner and FineCoarseSpinner. I decided not to put it in AccessibleNumberSpinner for the reasons described in https://github.com/phetsims/sun/issues/886#issuecomment-2239844488.

One change from this issue is that the onInput callback provides the previous value. If you would like, you can use onInput instead of creating a KeyboardListener and tracking a previousValue in SelectorNode like https://github.com/phetsims/scenery-phet/commit/9790871cf7f8cab227bc3c5fc7081ee39840dbd8. One benefit is that onInput should work better if listener orders are changed, but otherwise the behavior should be the same.

samreid commented 2 weeks ago

Thanks for the clarification. The prior implementation worked well during QA testing and it doesn't seem there is enough motivation to spend more time on PDL home/end sounds at this time. Will keep the changes in mind for future work. Self-unassigning.

pixelzoom commented 2 weeks ago

@pixelzoom you should be able to remove the workaround code pointing to this issue, can you confirm?

Working great. Workaround removed in the above commits.

Assign to @samreid to address the TODO in PDL that will prevent closing this issue. If you're going to "keep the changes in mind for future work", then creating a sim-specific issue would be most appropriate.

samreid commented 2 weeks ago

TODO removed, it seems this issue is ready to close. If not, please re-open for any reason. Closing.