NumberControl: How do constrainValue, delta, and shiftKeyboardStep work together?

[zepumph]

From phetsims/gravity-force-lab#173, the request was that the slider moved by 10, and the tweakers moved by 1. The issue as it pertains to a11y is that there is no a11y for the tweakers, everything interfaced through the slider. So adding "constrainValue to 10" there makes it impossible to move the slider by 1.

@jessegreenberg how should we remedy this? The naive and bad way to fix this is to stop trying to have the a11y interaction deviate from the visual interaction. Then we could just click the buttons. But I'm not convinced that is best just yet.

[jessegreenberg]

What do you think about changing the interval of constrainValue in MassControl so that it depends on whether shift key is down like

            constrainValue: v => {

              const interval = this.numberControl.slider.shiftKeyDown ? 1 : 10;
              return Util.roundToInterval( v, interval );
            },

Alternatively, arrow tweakers in NumberControl don't call constrainValue but round to the delta of the arrow button. Maybe we should do that in AccessibleValueHandler - when shift key is down, we round to nearest shiftKeyboardStep and don't call constrainValue. That is pretty much what the snippet above is doing, just moved to AccessibleValueHandler.

[jessegreenberg]

We took a look at this today. Overall, we are not thrilled with the implementation in NumberControl using attemptedIncreaseEmitter and attemptedIncreaseEmitter to press the arrow buttons when appropriate. We agreed that it would be nice if we could find another way to have the arrow buttons look clicked while only having the slider in the PDOM.

However, I did try the constrainValue in #528 (comment) and it does seem to work.

[zepumph]

while only having the slider in the PDOM.

This issue is kinda an example of how NumberControl is more than just a single PhET implementation of a Slider. We want to provide constraint to the slider to round by 10, but we want to be able to provide finer specificity with the shift key down.

To me this is close to redefining constrainValue, giving shiftKeyStep a backdoor around that constraint. Maybe if we just expanded the API for Slider somehow, this could be fixed.

It seems that a separate issue altogether is how to support visual pressing of the arrow buttons when shift key is pressed. That feels lesser than the first problem I outlined.

[zepumph]

Assigning for comment.

[jessegreenberg]

@zepumph and I discussed this on 9/24/19. We decided on 3 action steps given the current state of this issue:

• We look into the recommendation in NumberControl: How do constrainValue, delta, and shiftKeyboardStep work together? #528 (comment) a bit more, but try to build it into NumberControl.
• We would like to have a meeting with an a11y designer to determine if the styling of these buttons is really necessary. This is a very difficult thing to do with sun buttons. The current implementation isn't great and this requirement is making further work in this issue difficult.
• After this, we can look into a better way to make buttons look pressed without actually pressing them if we must.

[zepumph]

This blocks publication because it is breaking NumberControl shift keyboard step. Any sim with alternative input using a NumberControl should wait for this one (like GFL).

[zepumph]

We would like to have a meeting with an a11y designer to determine if the styling of these buttons is really necessary. This is a very difficult thing to do with sun buttons. The current implementation isn't great and this requirement is making further work in this issue difficult.

@terracoda and I discussed this, and we think that styling the tweakers isn't very important for accessibility. We think there are two alternatives that we could do instead:

1. This preferred solution is to disable the tweaker buttons when the slider has dom focus. On blur we re-enabled them. This will help communicate that it is expected that you can't navigate to those buttons.
2. The alternative if we don't end up liking that is to just leave them alone, with no styling. We don't think it will be too difficult of a reach for a keyboard user to figure out that they can do everything they need to with just the NumberControl slider.

Having discussed with @jessegreenberg earlier, I think he will also like option (1). Please let me know if that isn't the case. After we implement this, as well as fix the shift keyboard step, we think it will be good to run it past a visual designer to get their thoughts.

[zepumph]

@jessegreenberg, @emily-phet, and I discussed this earlier today, and we agreed to go with (1) from above for now.

@twant and I implemented this in the above commits. It felt really good to delete the code that was reaching into AccessibleValueHandler, and to instead just have to pass the delta in as the shiftKeyboardStep. @twant and I were also able to come up with a solution that we liked for setting enabled/disabled on focus. GFL is now fixed. Furthermore it fixed the shift keyboard interaction in all number controls, since it is set to the arrow button delta.

@jessegreenberg please review.

[pixelzoom]

Assigned to @jessegreenberg for review for 16 months. Raising priority to high. @jessegreenberg when can you get to this?

[jessegreenberg]

This looks great. I had forgotten that we used to actually click the tweaker buttons to accomplish this, this change is much better.