phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Alt input and UI sound for FineCoarseSpinner (and NumberSpinner?) #847

Closed pixelzoom closed 2 months ago

pixelzoom commented 3 months ago

Needed for Gas Properties suite PhET-iO release, https://github.com/phetsims/gas-properties/issues/191.

In https://github.com/phetsims/gas-properties/issues/213:

  • [ ] FineCoarseSpinner - in Particles accordion boxes. How should alt input behave? Should it be like NumberSpinner? Should each button get focus and operate like a push button? Or should the entire FineCoarseSpinner get focus and use left/right arrows for increment/decrement and shift for fine/coarse?

In https://github.com/phetsims/gas-properties/issues/214:

  • [ ] FineCoarseSpinner - all 4 buttons currently have the same default push button sound. Should there be different sounds for increment/decrement and fine/coarse?
pixelzoom commented 3 months ago

From https://github.com/phetsims/gas-properties/issues/213#issuecomment-2014974052, here are @terracoda's thoughts on alt input:

@pixelzoom, I agree, I think the FineCoarseSpinner is a number spinner type interaction. I think the number spinners In GFLB and RaP are implemented like discrete sliders.

The FineCoarseSpinner adds a second mode to the number spinner - a single-release and bunch-release. This could be handled in different ways, so it would be good to discuss options.

One option is to make 2 tab stops (like two separate number spinners on top of each other) and have one release singles and the other release bursts, but both share the same model value total. The first Tab Stop encases the single buttons, the second Tab Stop encases the burst buttons.

Option two could use custom modifier keys on a single number spinner to release singles or bursts. E.g. arrow keys -> singles; Page Up/Page Down -->> bursts. Just an idea. Depending on keys, the appropriate button is highlighted.

There could be other options. These are just ideas.

pixelzoom commented 3 months ago

Regardless of what we do with hotkeys or the individual push buttons, we'll need a group focus highlight. I added that in https://github.com/phetsims/scenery-phet/commit/886af758ab272a072139493c14d2d7dbc3e9e48e and it looks like this:

screenshot_3186
pixelzoom commented 3 months ago

Looking at NumberSpinner, there seems to be more work to do there too:

jessegreenberg commented 3 months ago

For alt input, I think we will want this to behave like NumberSpinner, here is a patch and a build to help with discussion. I am not sure about the sound question.

```patch Subject: [PATCH] Remove opt out of Interactive Highlights, see https://github.com/phetsims/friction/issues/201 --- Index: js/FineCoarseSpinner.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/FineCoarseSpinner.ts b/js/FineCoarseSpinner.ts --- a/js/FineCoarseSpinner.ts (revision 69f356768c26e0d2af3ca23e272f118cc16f3ef2) +++ b/js/FineCoarseSpinner.ts (date 1713373102027) @@ -17,38 +17,50 @@ import Tandem from '../../tandem/js/Tandem.js'; import NumberDisplay, { NumberDisplayOptions } from './NumberDisplay.js'; import sceneryPhet from './sceneryPhet.js'; +import AccessibleNumberSpinner, { AccessibleNumberSpinnerOptions } from '../../sun/js/accessibility/AccessibleNumberSpinner.js'; type SelfOptions = { deltaFine?: number; // amount to increment/decrement when the 'fine' tweakers are pressed deltaCoarse?: number; // amount to increment/decrement when the 'coarse' tweakers are pressed spacing?: number; // horizontal space between subcomponents numberDisplayOptions?: StrictOmit; - arrowButtonOptions?: StrictOmit; + arrowButtonOptions?: StrictOmit; }; -export type FineCoarseSpinnerOptions = SelfOptions & StrictOmit; +type ParentOptions = AccessibleNumberSpinnerOptions & NodeOptions; -export default class FineCoarseSpinner extends Node { +export type FineCoarseSpinnerOptions = SelfOptions & StrictOmit; + +export default class FineCoarseSpinner extends AccessibleNumberSpinner( Node, 0 ) { private readonly disposeFineCoarseSpinner: () => void; public constructor( numberProperty: NumberProperty, providedOptions?: FineCoarseSpinnerOptions ) { const options = optionize, NodeOptions>()( { + StrictOmit, ParentOptions>()( { // SelfOptions deltaFine: 1, deltaCoarse: 10, spacing: 10, + // AccessibleNumberSpinnerOptions + valueProperty: numberProperty, + enabledRangeProperty: numberProperty.rangeProperty, + + // Instead of changing the value with keyboard step options, the arrow buttons are synthetically + // pressed in response to keyboard input so that the buttons look pressed. + keyboardStep: 0, + shiftKeyboardStep: 0, + pageKeyboardStep: 0, + // NodeOptions disabledOpacity: 0.5, // {number} opacity used to make the control look disabled tandem: Tandem.REQUIRED, tandemNameSuffix: 'Spinner', phetioFeatured: true, - phetioEnabledPropertyInstrumented: true, - groupFocusHighlight: true // see https://github.com/phetsims/scenery-phet/issues/794 + phetioEnabledPropertyInstrumented: true }, providedOptions ); const range = numberProperty.range; @@ -58,6 +70,7 @@ // options for the 'fine' arrow buttons, which show 1 arrow const fineButtonOptions: ArrowButtonOptions = combineOptions( { + focusable: false, numberOfArrows: 1, arrowWidth: 12, // width of base arrowHeight: 14, // from tip to base @@ -80,6 +93,7 @@ // options for the 'coarse' arrow buttons, which show 2 arrows const coarseButtonOptions = combineOptions( {}, fineButtonOptions, { + focusable: false, numberOfArrows: 2, arrowSpacing: -0.5 * fineButtonArrowHeight, // arrows overlap @@ -145,6 +159,20 @@ }; numberProperty.link( numberPropertyListener ); // unlink required in dispose + // pdom - script clicks of arrow buttons from alt input events so that the buttons look pressed while the key is down + const increasedListener = ( isDown: boolean ) => { + if ( isDown ) { + this.shiftKeyDown ? incrementFineButton.pdomClick() : incrementCoarseButton.pdomClick(); + } + }; + const decreasedListener = ( isDown: boolean ) => { + if ( isDown ) { + this.shiftKeyDown ? decrementFineButton.pdomClick() : decrementCoarseButton.pdomClick(); + } + }; + this.pdomIncrementDownEmitter.addListener( increasedListener ); + this.pdomDecrementDownEmitter.addListener( decreasedListener ); + this.disposeFineCoarseSpinner = () => { if ( numberProperty.hasListener( numberPropertyListener ) ) { ```

https://phet-dev.colorado.edu/html/jg-tests/scenery-phet_en_phet.html?screens=6

jessegreenberg commented 3 months ago

Some notes from discussion with @terracoda @pixelzoom @arouinfar:

1) We want it to behave like a NumberSpinner (like the patch in https://github.com/phetsims/scenery-phet/issues/847#issuecomment-2061786929) 2) The default button sounds are acceptable. 3) We need a generalized KeyboardHelpSection for the components that use AccessibleNumberSpinner.

pixelzoom commented 3 months ago

We need a generalized KeyboardHelpSection for the components that use AccessibleNumberSpinner.

FYI, in Gas Properties I added a placeholder for 'Spinner Controls', so that @arouinfar and I have something for designing the layout of our keyboard help dialogs. See https://github.com/phetsims/gas-properties/issues/215#issuecomment-2062565105 and SpinnerControlsKeyboardHelpSection. I'm not implying that this should inform the design or wording of what will be done for AccessibleNumberSpinner, just wanted you to be aware of it.

terracoda commented 2 months ago

Examples KBH Dialogues for Sliders. @pixelzoom provides starting examples for Number spinner and coarse number spinner (previous comment).

Sliders and number spinners share a lot of functionality and behavior. They differ in visual representation. And I think that the number spinners can only have 2 step sizes (default and small) whereas sliders can have 3 (default, small and large).

Default Slider Control from Fourier Screenshot 2024-04-19 at 16 02 33

Custom Heading for Sliders in Molarity Screenshot 2024-04-19 at 16 01 41

Even More Customized in GFL Screenshot 2024-04-19 at 16 04 45

terracoda commented 2 months ago

Hmm, the regular Number Spinner seems more like a slider and can have up to 3 step sizes, but the Fine Coarse Spinner seems to be limited to a default large step (or burst) and a small (or single release) with the Shift modifier.

{{Spinner || Slider || Spinner and Slider}} Controls (regular Number Spinner and Slider)

Spinner Controls (Fine Coarse Spinner)

Question: With Fine Coarse Spinner are you always adding and removing things? Just wondering if this makes more sense?

Spinner Controls (Fine Coarse Spinner Idea)

NOTE: Removed "in" before "bursts".

terracoda commented 2 months ago

Assigning to @arouinfar for comment. Am I capturing everything? Tagging @jessegreenberg and @pixelzoom.

terracoda commented 2 months ago

We'll need accessible text for the KBH dialog as well. I just add "with" in front of the keys, and add "arrow keys" or "key" at the end in most or all cases.

{{Spinner || Slider || Spinner and Slider}} Controls (regular Number Spinner and Slider)

Spinner Controls (Fine Coarse Spinner)

Spinner Controls (Fine Coarse Spinner Idea)

jessegreenberg commented 2 months ago

The above commits add the help content for "spinner" controls with SpinnerControlsKeyboardHelpSection. @terracoda and I met to review the look and PDOM content in the scenery-phet demo.

@pixelzoom can you please review? https://github.com/phetsims/scenery-phet/commit/f66256f4d9d86cd6e08e0985c97f2f8bc321993a can be ignored, it is an abandoned branch.

A while ago we talked about a new superclass for this. But I thought SliderControlsKeyboardHelpSection was still the appropriate class to extend for this. Let me know what you think, happy to make changes.

pixelzoom commented 2 months ago

I'm not a fan of SliderControlsKeyboardHelpSection extends SliderControlsKeyboardHelpSection because a spinner does not extend a slider. Probably OK for now, but I predict this will need to be changed in the future.

That said... SliderControlsKeyboardHelpSection looks good integrated into FEL, see screenshot below. But a different problem (and probably worth a different issue) is the amount of duplication that we have between keyboard help for various UI components, as demonstrated by "Slider" and "Spinner" in this screenshot. Rather than having keyboard help for each type of UI component, it would be more usable and scalable to expand the "Basic Actions".

screenshot_3286
jessegreenberg commented 2 months ago

Yes, that is a good idea. I made a new issue for that.

because a spinner does not extend a slider. Probably OK for now, but I predict this will need to be changed in the future.

OK, understood. I am going to close this for now but will keep that in mind. Thanks!