revise logic for checking whether dragging is via the keyboard

[pixelzoom]

SliderUnit.js, added by @jbphet:

    // override the start and end drag functions in the options
    const providedStartDragFunction = options.sliderOptions.startDrag;
    options.sliderOptions.startDrag = function( event ) {
      if ( event.type === 'keydown' ) {
        self.sliderDraggingByKeyboardProperty.set( true );
      }
      providedStartDragFunction && providedStartDragFunction();
    };
    const providedEndDragFunction = options.sliderOptions.endDrag;
    options.sliderOptions.endDrag = function() {
      self.sliderDraggingByKeyboardProperty.set( false );
      providedEndDragFunction && providedEndDragFunction();
    };

In 10/17/19 dev meeting, @jbphet said that the logic for checking whether dragging is via the keyboard is now handled in scenery, and needs to be revised here.

[pixelzoom]

10/17/19 dev meeting:

The above logic and BooleanProperty probably belongs in Slider. It may belong in DragListener.

@zepumph noted that PressListener.a11yClickingProperty is probably what we want to use here. Also noted that DragListener and KeyboardDragListener need to be unified.

[jbphet]

I like the idea of having the information about how the slider is being dragged in Slider or DragListener. I just spent some time looking at how best to do this, and I have some observations and questions. Let's start with a general proposal. How about instead of having a BooleanProperty like the one shown in the code above we have a general property - something like dragStateProperty - whose values are from an enum and indicate how the drag is occurring. I did something like this in MassControl.js. This property would become a public read-only member of Slider.js.

In the current implementation of Slider.js, drag handling is split between an instance of SimpleDragHandler and the AccessibleSlider mixin. The former handles mouse- and touch-based drag events, the latter handles keyboard-based drag events. We could implement the state property by having the SimpleDragHandler update the state as appropriate, and by passing in start and end drag handler functions to AccessibleSlider so that it can change the state as well. I could implement this now, but the note from @zepumph above about using PressListener.a11yClickingProperty and unifying the drag listeners give me pause.

@pixelzoom - how would you feel about me adding the drag state property to Slider.js as it is now, and whoever unifies the listeners can deal with it if and when the unification happens? I'd just rather not try to overhaul Slider to use different listeners since it seems a little out of my bailiwick.

[jbphet]

Related issue: phetsims/scenery#1016

[pixelzoom]

@pixelzoom - how would you feel about me adding the drag state property to Slider.js as it is now, and whoever unifies the listeners can deal with it if and when the unification happens?

3 problems with that plan:

(1) dragStateProperty may not be the best solution for listeners (DragListener, SimpleDragHander). Has @jonathanolson been consulted?
(2)dragStateProperty is unlikely to be moved to the listeners for a long time.
(3) Other things that are draggable won't be able to take advantage of dragStateProperty if it's specific to Slider.

This feature belongs in DragListener and SimpleDragHandler, not in Slider. I recommend spending the time to address this correctly now. And moving this issue to scenery.

[jbphet]

@jonathanolson - please log your input on how you think this should be handled, then mark this for dev meeting so we can decide who will make the changes and when.

[zepumph]

This was discussed as part of phetsims/scenery#1016 just now with @jbphet, @jessegreenberg, and me. In general, ,since this issue was tagged, the general sound implementation pattern has changed from listening to the model to instead listening to user input. We decided that the Properties were only created to facilitate the sound generators listening to the model, and that they wouldn't be needed if they were instead listening directly to when the sliders change.

To fix this we will move the "sliderClick" DiscreteSoundGenerator instances from the screen view into SliderUnit. @jbphet also mentioned that perhaps there is a better way to use the sound besides DiscreteSoundGenerator.

I am in this sim already, so I will take this on.