Expand functionality of AccessibleValueHandler

[jessegreenberg]

In phetsims/scenery#951 @zepumph made a new type called AccessibleValueHandler that handles aria-valuetext for an a11y component that is driven by a NumberProperty. AccessibleSlider and AccessibleNumberSpinner are using this. There may be other things that can be moved into AccessibleValueHandler as well. For instance, the input handling of these two types are similar. AccessibleValueHandler could have functions like

       /**
        * Adds a listener to the Node that set the NumberProperty when home/end are pressed.
        **/
        addHomeEndListener() {}

Or instead of adding listeners we could just have reusable functions that do this work and add custom listeners in the subtypes.

We also mentioned in phetsims/scenery#951 we also mentioned that perhaps aria-valuetext and inputValue setters of Accessibility.js could be moved into the AccessibleValueHandler trait.

[zepumph]

So if we were going to make checkboxes, here may be a few to look into

• factoring out listeners into AccessibleValueHandler
• See what mutatable options we can take from Accessibility.js that really only apply to types that run on a Property.
• Part of this work should add mobile accessibility functionality to AccessibleNumberSpinner. I think that will be done by factoring out an input listener to the AccessibleValueHandler, but we should make sure that works, because it is needed for Make sure that this sim works with mobile gravity-force-lab-basics#119.

[jessegreenberg]

See what mutatable options we can take from Accessibility.js that really only apply to types that run on a Property.

In phetsims/scenery#951 we decided not to do this.

[jessegreenberg]

In #497 we decided to use input type=number for AccessibleNumberSpinner. So now I am wondering if NumberPicker should be using AccessibleSlider too. The largest difference is the use of CallbackTimer to time firing of value changes, but everything else is the same.

[jessegreenberg]

It seems like there are still enough differences that we should proceed with factoring listeners into AccessibleValueHandler.

Adding high priority label since this is blocking publication of gravity-force-lab-basics.

[jessegreenberg]

we could just have reusable functions that do this work and add custom listeners in the subtypes.

I think this is the most straight forward, and will be the easiest. Now that AccessibleNumberSpinner is a input type="number" instead of a div role="spinbutton" there are things that are in the AccessibleNumberSpinner listeners that are no longer necessary. So listeners between the two are even more similar now.

[jessegreenberg]

Ill summarize my changes here:

• Substantial amount of code was moved from AccessibleSlider into AccessibleValueHandler to be shared by both AccessibleSlider and AccessibleNumberSpinner.
• AccessibleValueHandler now implements handleKeyDown, handleKeyUp, handleChange, handleInput, handleBlur. The new pattern is that nodes mixing in AccessibleValueHandler will be responsible for adding these input listeners with addInputListener.
• A number of options were consolidated. AccessibleNumberSpinner used to have a11yValueDelta and a11yPageValueDelta. These have been changed to keyboardStep and pageKeyboardStep since they have been moved to AccessibleValueHandler.
• startDrag and endDrag were renamed to startChange and endChange since AccessibleValueHandler is more general than "dragging" operations.

I think those are the big things. One issue I ran into was phetsims/axon#248 which puts part of this solution on hold:

// TODO: This will emit events that signify that interaction is taking place, but it is currently
// broken with axon/timer.js, see https://github.com/phetsims/axon/issues/248 Add back in as soon
// as that issue is resolved.
// this.emitKeyState( event.domEvent.keyCode, true );

Until this is fixed keyboard interaction will not work in with NumberSpinner, and there won't be any styling during keyboard interaction with NumberPicker.

[jessegreenberg]

@zepumph could you please take a look at these changes to AccessibleValueHandler? Mostly, how do you feel about the new pattern where the mixing node adds input listeners from AccessibleValueHandler? Also, I have a note above accessibleNumberSpinnerHandleKeyDown that describes how it would be nice to override a trait function, but I couldn't think of a nice way to do it, are you aware of a way?

Let me know if it would be best to discuss at next dev meeting.

[jessegreenberg]

Part of this work should add mobile accessibility functionality to AccessibleNumberSpinner

This is also done. I think this is ready for review.

[zepumph]

• Why are the default options pulled out of the options extend call? options = _.extend( {}, defaults, options );. That seems like an extra object allocation that isn't needed.
• ariaOrientation seems a bit strange for a number spinner, but I guess it makes sense because it is an input of type number.
• AccessibleValueHandler is missing some parameter jsdoc
• Here is a code snippet from keydown:

                    self.accessibleNumberSpinnerHandleKeyDown( event );

                    downCallback = self.accessibleNumberSpinnerHandleKeyDown.bind( self, event );
                    runningTimerCallbackKeyCode = event.domEvent.keyCode;
                    self._callbackTimer.addCallback( downCallback );
                    self._callbackTimer.start();

I don't really understand why we need to call the keydown function twice, once immediately and once after a timer.

• In addition to the above comment, I'm curious if there was only one place where accessibleNumberSpinnerHandleKeyDown was called, then it seems like we could just call handleKeyDown and emitKeyState directly from keydown and we wouldn't need the intermediary.

[jessegreenberg]

Thank you for the review!

Why are the default options pulled out of the options extend call?

We need to separate defaults from options so that fields that are not included in defaults do not get applied to the Node twice if the mixing type calls mutate as well. This is most important for
options that might change transform. For example, this prevents scale from being applied twice from options to sliders because Slider.js also calls mutate.

AccessibleValueHandler is missing some parameter jsdoc

Thanks, added it.

I don't really understand why we need to call the keydown function twice, once immediately and once after a timer.

In the original design of AccessibleNumberSpinner, it was decided that it was important for press and hold behavior for keys to behave similarly to press and hold behavior for mouse/touch. This timer is how we support options timerInterval and timerDelay.

In addition to the above comment, I'm curious if there was only one place where accessibleNumberSpinnerHandleKeyDown was called, ...

In order to control timing I think we will still need both calls.

[zepumph]

Alright sounds good. Thanks for the quick response.

[jessegreenberg]

A fix was added for phetsims/axon#248 so the TODO can be removed now.

[jessegreenberg]

TODO removed and styling of the buttons is working correctly again. That was the last thing, this issue can be closed.