Convert to NumberControl

[zepumph]

In phetsims/scenery-phet#341 we added keyboard nav to number control. It would be nice to convert to use this common code component since it is visually the exact same as Plinko probability, then we would get keyboard nav for free.

RS:

Plinko (with some sweet focus highlight):

I talked to @jessegreenberg (responsible dev) and he gave me the go ahead to do this. @ariel-phet does this sound good to you?

[ariel-phet]

@zepumph seems reasonable to me as long as you can pretty much keep the layout and such like the original

[zepumph]

In converting to NumberControl. I think that we will loose multitouch capabilities. I believe I can add that to NumberControl though, seeing as I have an implementation to base it off of. @jessegreenberg how important is multitouch in RS?

[jessegreenberg]

I think most of the code related to multitouch was added to fix this issue: #104.

The fix goes through startDrag and endDrag, does NumberControl not support those?

[jessegreenberg]

Ah, is it because NumberControl calls startCallback for both the buttons and the slider and we want the listener to only be called for the slider?

[zepumph]

I think I'm going to update NumberControl to support a single startCallback OR individual callbacks for each interactive property. So a start/end for (a) left arrow (b) right arrow (c) slider.

[zepumph]

we want the listener to only be called for the slider?

We want a specific callback for the slider, and a different one for the arrows.

[jessegreenberg]

Got it, thanks. Here were the steps that broke things with these sliders initially:

Start in micro view (works with macro, but I'll assume you start with micro)
Use finger A to grab the proton slider
Switch to macro view
Switch to micro view
Move finger A out of the way to make room for finger B to also grab the proton slider
Release finger B

It is also possible that the fix added in #104 for this set of issues is no longer necessary, I think that a general fix was added recently so that events are canceled when screens are switched.

[zepumph]

Thanks @phet-steele for helping with this. @jessegreenberg he was able to test for that bug, removing the multitouch implementation, and it was still present. I think that we will need to keep this support, which pretty much doubles this type, and adds some gunk to NumberControl also, since we need to be able to attach specific callbacks to each component. It makes NumberControl very powerful, but also adds a bit of complexity. Otherwise the conversion to NumberControl is done. @jessegreenberg could you please review the above commits, especially those to NumberControl, like that addition of specific callbacks and the validation of them.

[zepumph]

@jessegreenberg one more important thing that I forgot to mention. Currently the multitouch implementation doesn't work for keyboard navigation. To cause the bug, focus the slider and then hold down right and then press the left key. I'm not sure how to go about solving this.

[zepumph]

Note that this was a problem before the conversion to NumberControl

[jessegreenberg]

Currently the multitouch implementation doesn't work for keyboard navigation. To cause the bug, focus the slider and then hold down right and then press the left key.

This is caused by a flaw in AccessibleSlider, we are calling endDrag every keyup, which I think is too requent. Instead, we should try to call endDrag when all range keys are held up.

[jessegreenberg]

I can no longer reproduce this bug. For NumberControl, I think this looks good. I agree about the added complexity but the ability to have specific callbacks for component seems like a nice enhancement. The only thing I noticed was that an assertion in validateCallbacksAndSetDefault wasn't correct since normalCallbacksPresent was assigned a function instead of a boolean. @zepumph can you please re-review my changes and close if this issue is done?

[zepumph]

This is all very nice. Including the addition of only calling endDrag is all keys are up. How thoroughly have you regression tested that change? I know that it will make things more straightforward for Friction (I was actually going to talk to you about this). I am not convinced that every slider implementation will be so flexible. I think we should create a new issue discussing KeyboardDragListener's endDrag, and whether it is best to support a callback whenever a single key is released (which I think may be important eventually). @jessegreenberg can you please comment on how you think this may be touching other sims, and then create that issue if you think that is the best way to proceed.

[jessegreenberg]

I know that it will make things more straightforward for Friction

The changes in this issue are only for AccessibleSlider and won't impact KeyboardDragListener. I feel confident in my testing this with sliders.

@zepumph and I discussed how to proceed with KeyboardDragListener and will do so in phetsims/scenery#849. Like that issue, maybe we should add an onKeyUp callback AND an endDrag callback to AccessibleSlider?

[zepumph]

Oops. . . thanks for your patience. Everything above sounds good. I don't think we need to add onKeyUp right now since there is no use for it. I would assume that it will be added at some point though. If you feel differently, please reopen and assign to me for completion. Otherwise we will continue in scenery. Closing