Slider keyboard "step" options don't work if their value is smaller than constrainValue step.

[pixelzoom]

In Fourier, I have amplitude sliders that I want to configure like this:

// Amplitude values for the slider steps.
const STEP = 0.05;
const FINE_STEP = 0.01;
const COARSE_STEP = 0.1;
...
  options = merge( {
...
     // constrain dragging to STEP
     constrainValue: amplitude => {
      if ( amplitude !== amplitudeRange.min && amplitude !== amplitudeRange.max ) {
        amplitude = Utils.roundToInterval( amplitude, STEP );
      }
      return amplitude;
    };

    // prom
    keyboardStep: STEP
    shiftKeyboardStep: FINE_STEP,
    pageKeyboardStep: COARSE_STEP,

The designers want dragging constrained to STEP - no intermediate values. This was working great until I added the PDOM options, and discovered that options.constrainValue is also passed to AccessibleValueHandler. For Slider.js:

      constrainValue: _.identity, // called before valueProperty is set, passed to AccessibleValueHandler as well

So shiftKeyboardStep is not working, because it's value is smaller than STEP.

Why does AccessibleValueHandler need to use constrainValue when it has specific step sizes? If it has specific step sizes, should it ignore constrainValue?

@jessegreenberg @zepumph suggestions on how to proceed? And again, dragging must be constrained to STEP, with no intermediate values.

[zepumph]

What a fun problem. We, in the past, have had good success using these two options in combination, like using constainValue to appropriately round all step sizes. Is there a patch/commit for a component that I could play around with before recommending a path forward?

[pixelzoom]

The example code in https://github.com/phetsims/sun/issues/698#issue-868238784 is a simplified version of fourier-making-waves AmplitudeSlider.js, where you'll find this TODO:

    //TODO https://github.com/phetsims/sun/issues/698 constrainValue is overriding shiftKeyboardStep

[zepumph]

The "high" priority label is for me to get to it anytime in the next 2 weeks. Feel free to remove if @jessegreenberg is able to answer your questions.

[pixelzoom]

I'm going to re-title this issue, because the problem is not specific to shiftKeyboardStep. It will be a problem for any step option(s) that have a smaller value smaler than the step used by constrainValue.

[pixelzoom]

@arouinfar and I were trying to figure out why this doesn't seem to be a problem in GFL. I found this in NumberControl.js, added by @zepumph in https://github.com/phetsims/scenery-phet/commit/17e8ac331798e553941013079b84be4b73c80cde:

    // By default, constrain to multiples of delta, see #384
    const defaultConstrainValue = value => {
      const newValue = Utils.roundToInterval( value, options.delta );

      return getCurrentRange().constrainValue( newValue );
    };
...
    // Support shift key stepping based on the arrow key delta, but that may be more minute than constrainValue allows
    // for the slider.
    if ( options.sliderOptions.constrainValue ) {
      const oldConstrainValue = options.sliderOptions.constrainValue;
      options.sliderOptions.constrainValue = value => {
        if ( this.slider.getShiftKeyDown() ) {
          return defaultConstrainValue( value );
        }
        return oldConstrainValue( value );
      };
    }

It's circumventing constrainValue when the Shift key is down. This must have been added as a workaround for GFL. This doesn't seem appropriate in NumberControl, should live in Slider. Does anyone recall why this was added to NumberControl instead of Slider?

A workaround would be to move this to Slider. A more longterm solution is to revisit the "step size" options and make them work for all input modes.

[pixelzoom]

I asked:

Does anyone recall why this was added to NumberControl instead of Slider?

Looking at commit messages... This identical issue was discussed and addressed by @zepumph and @jessegreenberg in https://github.com/phetsims/scenery-phet/issues/528. But for some reason, all of the work was done in NumberControl, and the identical problem was not addressed in Slider. Do either of you recall why the Slider-specific stuff (like the code shown above) was not done in Slider?

[pixelzoom]

@jessegreenberg and I discused this today via Zoom. In the longterm, it makes sense to take a look at all of the "step size" related options to Slider, as is suggested in https://github.com/phetsims/sun/issues/703. But that's likely to be a bigger project than can be completed in the timeframe needed for Fourier 1.0. So for the short-term, @jessegreenberg is going to investigate moving the NumberControl workaround into Slider.

@jessegreenberg I'll be happy to review this work, and test-drive in Fourier.

[jessegreenberg]

This much was done in the above two commits, moving the check for shiftKeyDown to AccessibleValueHandler and removing it from NumberControl. I tested the case in Fourier and it is working as expected, and checked a number of things using AccessibleValueHandler (NumberControl, sim sliders, NumberSpinner, hands in ratio-and-proportion, appendages in john-travoltage).

@pixelzoom could you please review?

[pixelzoom]

Looks great, thanks @jessegreenberg. I tweaked one comment in the above commit when something non-optional was still documented as "optional". Tested in Fourier and GLF, and everything is working as expected/desired.

Closing.

[pixelzoom]

Reopening. Because constrainValue is now specific to mouse/touch, there is some documentation of options that needs to be revised,

This:

        // @private {function(number):number} - called before valueProperty is set
        this._constrainValue = options.constrainValue;

        // @private {function(number,number):number} - called before constrainValue called and valueProperty is set
        this._a11yMapValue = options.a11yMapValue;

and:

          // {function(number):number} - It should return the constrained value, called before valueProperty is set
          constrainValue: _.identity,

and:

          /**
           * Called before constraining and setting the Property. This is useful in rare cases where the value being set
           * by AccessibleValueHandler may change based on outside logic. This is for mapping value changes from input listeners
           * assigned in this type (keyboard/alt-input) to a new value before the value is set.
           * @type {Function}
           * @param {number} newValue - the new value, unformatted
           * @param {number} previousValue - just the "oldValue" like the Property listener
           * @returns {number} - the mapped value, ready to be constrained
           */
          a11yMapValue: _.identity,

I'll make a pass at revising, then assign to @jessegreenberg for review.

[pixelzoom]

Oh wait. constrainValue is not mouse/touch specific. It's currently ignored only when the Shift key is down. Hmm.... maybe we should just leave the doc as is for now? @jessegreenberg what do you think?

And regardless of whether the doc gets changed, this issue should be left open. There's a workaround for shiftKeyboardStep, but the other 2 options could still be a problem (though unlikely).

[jessegreenberg]

Thanks, I updated the documentation for the option to make it clear there.

I am seeing a couple of CT errors related to this change.

fourier-making-waves : interactive-description-fuzzBoard : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/fourier-making-waves/fourier-making-waves_en.html?continuousTest=%7B%22test%22%3A%5B%22fourier-making-waves%22%2C%22interactive-description-fuzzBoard%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1623286501224%22%2C%22timestamp%22%3A1623288831965%7D&brand=phet&ea&fuzzBoard&supportsInteractiveDescription=true&memoryLimit=1000
Query: brand=phet&ea&fuzzBoard&supportsInteractiveDescription=true&memoryLimit=1000
Uncaught Error: Assertion failed: value failed isValidValue: 3.04
Error: Assertion failed: value failed isValidValue: 3.04
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/assert/js/assert.js:25:13)
at Object.isValueValid (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/axon/js/ValidatorDef.js:324:39)
at validate (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/axon/js/validate.js:36:18)
at NumberProperty.validateNumberProperty (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/axon/js/NumberProperty.js:98:47)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/axon/js/NumberProperty.js:138:12
at TinyProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/axon/js/TinyEmitter.js:86:18)
at NumberProperty._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/axon/js/Property.js:271:23)
at NumberProperty.set (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/axon/js/Property.js:186:14)
at ComponentSpacingSlider.handleKeyDown (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/sun/js/accessibility/AccessibleValueHandler.js:578:33)
at Input.dispatchToListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623286501224/scenery/js/input/Input.js:1884:25)
id: Bayes Chrome
Snapshot from 6/9/2021, 8:55:01 PM

Because constrainValue isn't being used when shift is down, it requires that a shiftKeyboardStep be specified and that it adheres to the NumberProperty's validation.

[jessegreenberg]

Oops, this workaround was also in the wrong spot. Just checking for whether the shift key was down means that we aren't using constrainValue when shift is down other keys like home or end. The above two commits fixed the CT errors, @pixelzoom please be aware of https://github.com/phetsims/fourier-making-waves/commit/8d482bc84fd8cacd2d3b7f5aa7cce81f16f81a2e to Fourier.

[jessegreenberg]

CT is stable now, @pixelzoom can you please review these latest changes? If OK then the "high" priority label can be removed and a better solution for keyboardStep and pageKeyboardStep can be worked on another time, and probably as part of #703.

[pixelzoom]

Thanks for the doc updates, looks good.

You added this to ComponentSpacingSlider in https://github.com/phetsims/fourier-making-waves/commit/8d482bc84fd8cacd2d3b7f5aa7cce81f16f81a2e:

    keyboardStep: 1,      
+   shiftKeyboardStep: 1,
+   pageKeyboardStep: 1

Rather than requiring clients to redundantly specify shiftKeyboardStep and pageKeyboardStep, can AccessibleValueHandler set them to the value of keyboardStep if they have not been specified by the client?

[pixelzoom]

Oh boy... There's more to this issue than I orginally reported.

I originally described this problem as:

Slider keyboard "step" options don't work if their value is smaller than constrainValue step.

But there's another relationship that must exist between the step options and constrainValue. It's currently the case that keyboardStep and pageKeyboardStep must be multiples of the constrainValue interval.

For example, suppose I have a slider with these options -- you can try these values in AmplitudeSlider.js:

constrainValue: value => Utils.roundToInterval( amplitude, 0.5 ),
keyboardStep: 0.6,
shiftKeyboardStep: 0.1,
pageKeyboardStep: 1.25

shiftKeyboardStep will work fine because constrainValue is now ignored when the Shift key is down.

keyboardStep and pageKeyboardStep will NOT work correctly, because they are not a multiple of 0.5. The slider value will be stepped by the value of these options, and then rounded to the closest 0.5 interval when constrainValue is applied.

I don't know how likely this is to occur in a sim, but it's definitely not obvious/robust, and could lead to confusion/bugs. Should these be addressed now, or as part of the larger issue https://github.com/phetsims/sun/issues/703? @jessegreenberg thoughts?

[zepumph]

Most likely a subset of https://github.com/phetsims/scenery/issues/1298.

[zepumph]

https://github.com/phetsims/sun/issues/698#issuecomment-858775664 is EXACTLY what @jessegreenberg and @samreid and I just ran into for My Solar System in https://github.com/phetsims/my-solar-system/issues/105. We do not like the "cleverness" of how it can behave, and that, coupled with the possibility for a noop depending on your step (see https://github.com/phetsims/sun/issues/350), we are going to delete the constrainValue option from the keyboard side of things. Please see https://github.com/phetsims/sun/issues/837. Closing