Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dragStateProperty to DragListener and SimpleDragHandler #1016

Closed
jbphet opened this issue Nov 11, 2019 · 8 comments
Closed

Add dragStateProperty to DragListener and SimpleDragHandler #1016

jbphet opened this issue Nov 11, 2019 · 8 comments

Comments

@jbphet
Copy link
Contributor

jbphet commented Nov 11, 2019

In the process of adding sound to simulations, I have run into a number of cases where the code needed to know how the user was interacting with a slider in order to determine how and when to play a sound. In these cases, the startDrag and endDrag methods of the drag handler were modified to set local state variables that kept track of the type of dragging. An issue was logged about this in phetsims/ohms-law#152, and that issue includes a code example of how this was done.

Since this is likely to come up for any sort of dragging, it would be helpful to have some sort of state indication that is inherent to the drag handler itself. I recently added code to a class called MassControl in Gravity Force Labs that set a state variable via the drag handlers. It looked like this (code not related to this issue was omitted):

      startDrag: event => {
        if ( event.type === 'enter' || event.type === 'move' || event.type === 'down' ) {
          this.sliderDragStateProperty.set( SliderDragState.DRAGGING_VIA_POINTER );
        }
        else if ( event.type === 'keydown' ) {
          this.sliderDragStateProperty.set( SliderDragState.DRAGGING_VIA_KEYBOARD );
        }
      },
      endDrag: () => {
        this.sliderDragStateProperty.set( SliderDragState.NOT_DRAGGING );
      },

This turned out to be very useful and seemed fairly general. Shall we add this to the drag handlers themselves?

Assigning to @jonathanolson for his input, since these are Scenery input handlers, and marking for dev meeting for more general review. I'm happy to do the implementation if approved.

@jessegreenberg
Copy link
Contributor

This looks really nice to me, I agree this is useful. There are two other event types that we currently listen for with sliders which are input and change which come from iOS VoiceOver or a switch device. We will want to include those as another value in SliderDragState.

I wonder if this is more general than dragging, we may want this information when using buttons or other things. So should this kind of thing be in PressListener instead of DragListener?

@jonathanolson
Copy link
Contributor

This sounds great to add in. What are all desired states?

@Denz1994
Copy link
Contributor

Denz1994 commented Dec 5, 2019

As discussed in dev-meeting on 12/05/19:

@zepumph mentioned the scenery event as detailed above may work (instead using event.isAlly()). Additionally using sceneryPointer may help determine if the event comes from the PDOM or another source. This may work for Sonification, but there are other general inputs that can benefit from this event recognition (keyboard, touch or pointer).

@jbphet will make an attempt at using event.isAlly()

@samreid
Copy link
Member

samreid commented Dec 5, 2019

I suggested using startEventProperty.value = event so we have the entire event API at our disposal. Then in the end event, we could set startEventProperty.value = null.

@zepumph
Copy link
Member

zepumph commented Feb 5, 2020

@jbphet , phetsims/sun#536 is very similar to this issue. I would love to discuss before either of us go further on these.

@zepumph zepumph self-assigned this Feb 5, 2020
@zepumph
Copy link
Member

zepumph commented Apr 6, 2020

In order to have the code as @jbphet had it above in DragListener, KeyboardDragListener would have to be integrated into it. This would complicate this issue a fair bit, but I feel like it may be good work to do.

It is hard to know that these always correspond to the same Node across the project, though they likely overlap mostly. I want to mark this for discussion with @jessegreenberg tomorrow to see how it may look to add KeyboardDragListener logic into DragListener.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Apr 7, 2020

I am hesitant to add KeyboardDragListener into DragListener and I think most usages of DragListener would not want KeyboardDragListener functionality. Since it is so difficult to describe arbitrary 2D motion, we generally try to use alternatives first like AccessibleSlider for 1D motion, or even a limited set of positions for an object when using a keyboard.

@zepumph
Copy link
Member

zepumph commented Apr 7, 2020

After discussing with @jessegreenberg today, we thought through potential general solution to know whether events come from the PDOM or not. Note that we already have SceneryEvent.prototype.isFromPDOM(), and that has the capability to check for any event passed into a listener callback. We thought about potentially having a trait for Node that just has inputFromPDOMProperty or something. The implementation of which would listen to all scenery events and set the Property based on the most recent event. This implementation though does really make sense for most interactions. Take activating a button. While there are keydown/keyup or down/up events that accompany that, the "activation" is really only a single moment that comes from an event. Same with a checkbox. I couldn't really figure out how the mixin would be used outside of the dragging case.

That lead us to discuss the specific Ohms Law case with @jbphet. It is explained more in phetsims/ohms-law#152, but briefly the Property mentioned above will likely go away when using a newer pattern for sound where we listen to user interaction (from the events) instead of the to the model.

From this I think we are ready to close this issue, and not implement anything for scenery listeners.

Closing

@zepumph zepumph closed this as completed Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants