phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
http://scenerystack.org/
MIT License
9 stars 6 forks source link

GrabDragInteraction: Balloon drag cue doesn't hide immediately when moving the balloon. #868

Closed zepumph closed 2 months ago

zepumph commented 2 months ago

It only hides once you release and regrab the balloon. I think we probably want this to dissappear. Found while working on https://github.com/phetsims/density-buoyancy-common/issues/364

zepumph commented 2 months ago

I also reproduced on friction too. Probably this is in common code.

zepumph commented 2 months ago

This works, but likely isn't the best way to handle things. I can't find the regression, but there definitely is one from https://phet.colorado.edu/sims/html/balloons-and-static-electricity/latest/balloons-and-static-electricity_all.html.

```diff Subject: [PATCH] update doc, https://github.com/phetsims/tandem/issues/313 --- Index: js/accessibility/GrabDragInteraction.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/accessibility/GrabDragInteraction.ts b/js/accessibility/GrabDragInteraction.ts --- a/js/accessibility/GrabDragInteraction.ts (revision 4594372eb992aff14f197cda267d178b0b26de9a) +++ b/js/accessibility/GrabDragInteraction.ts (date 1724799086595) @@ -620,7 +620,11 @@ this.node.inputEnabledProperty.lazyLink( boundUpdateVisibilityForCues ); + const x = () => this.updateVisibilityForCues(); + keyboardDragListener.dragEndAction.executedEmitter.addListener( x ); + this.disposeGrabDragInteraction = () => { + keyboardDragListener.dragEndAction.executedEmitter.removeListener( x ); this.node.removeInputListener( this.dragListener ); this.node.inputEnabledProperty.unlink( boundUpdateVisibilityForCues );
zepumph commented 2 months ago

Looks like the regression dates back to when KeyboardListener was added to GrabDragInteraction. https://github.com/phetsims/scenery-phet/commit/db713ca794457e00cc4e3d5e58563456f1800227

@jessegreenberg, we would like to call "updateVisibilityForCues" whenever a keyup occurs, not just for space/enter. Can you help me out with this line here:

https://github.com/phetsims/scenery-phet/blob/4594372eb992aff14f197cda267d178b0b26de9a/js/accessibility/GrabDragInteraction.ts#L569

jessegreenberg commented 2 months ago

This was working in BASE 1.4.0 but is broken in 1.5.0 (Nov 2021).

I tried something similar to https://github.com/phetsims/scenery-phet/issues/868#issuecomment-2313715358 but using KeyboardDragListener.isPressedProperty but it did not work. The default callback for showDragCueNode always returns true. And the value for showDragCueNode provided by BalloonNode makes an incorrect assumption about the listener order.

Anyway, I am wondering if we can remove these options:

      showGrabCueNode: () => {
        return this.grabDragModel.grabDragCueModel.numberOfKeyboardGrabs < 1 && node.inputEnabled;
      },
      showDragCueNode: () => {
        return true;
      },

And make it consistent for GrabDragInteraction? If not, maybe replace those with axon.Propertys so client can directly control visibility?

If continuing with https://github.com/phetsims/scenery-phet/issues/868#issuecomment-2313715359 is best, a new basic listener on keydown is probably the way to do it.

zepumph commented 2 months ago

I committed the KeyboardListener equivalent of a keyup listener for dragging keys above. In regards to the showDragCueNode defaulting to always true, I believe that this is an acceptable default, as it is up to the client to determine the logic to hid the cueing. I like the idea of using Properties instead, and @samreid and I had also discussed this over in https://github.com/phetsims/scenery-phet/issues/869. I didn't understand the issue with the listener order. Would you like to discuss that more?

zepumph commented 2 months ago

@jessegreenberg and I discussed, and we now understand the problem fully, and like the listener that was added above as a solution. Closing.