search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Fix GrabDragInteraction.enabledProperty #870

Closed zepumph closed 1 month ago

zepumph commented 2 months ago

Originally added in https://github.com/phetsims/scenery-phet/issues/720 and then rediscovered as buggy in https://github.com/phetsims/scenery-phet/issues/869. Enabled should not change if the node is focusable.

https://github.com/phetsims/scenery-phet/blob/1b0992b/js/accessibility/GrabDragInteraction.ts#L628

We do not want enabled to control focusability! Instead we just want it to handle input enabled and the visibility of the cues.

zepumph commented 2 months ago

I believe that this is best. @jessegreenberg can you give me a spot check on this? I discussed it with @samreid also and we liked it, especially for Buoyancy. Feel free to close.

jessegreenberg commented 1 month ago

Excellent, thanks. Can you remind me why all of the listeners don't need the enabledProperty? At the moment only the pressReleaseListener is receiving it.

zepumph commented 1 month ago

A disabled GrabDragInteraction can never be in its grabbed state. So we don't need to disable all the listeners that apply to that state later in the interaciton. I believe it could be just fine to forward enabled to those, but it just isn't needed.

Further more, we want disabled components to have focus/blur logic run still, so the DragListener, and the click listener in idleState is really all there is to worry about.

jessegreenberg commented 1 month ago

OK, makes sense. Thanks! Closing.