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

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

Closed
zepumph opened this issue Aug 27, 2024 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented Aug 27, 2024

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

@zepumph zepumph self-assigned this Aug 27, 2024
@zepumph
Copy link
Member Author

zepumph commented Aug 27, 2024

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

@zepumph
Copy link
Member Author

zepumph commented Aug 27, 2024

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.

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
Copy link
Member Author

zepumph commented Aug 27, 2024

Looks like the regression dates back to when KeyboardListener was added to GrabDragInteraction. db713ca

@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:

this.updateVisibilityForCues();

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Aug 27, 2024
@zepumph zepumph changed the title Balloon drag cue doesn't hide immediately when moving the balloon. GrabDragInteraction: Balloon drag cue doesn't hide immediately when moving the balloon. Aug 27, 2024
@zepumph zepumph transferred this issue from phetsims/balloons-and-static-electricity Aug 27, 2024
@jessegreenberg
Copy link
Contributor

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

I tried something similar to #868 (comment) 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 #868 (comment) is best, a new basic listener on keydown is probably the way to do it.

zepumph added a commit that referenced this issue Aug 29, 2024
zepumph added a commit to phetsims/scenery that referenced this issue Aug 29, 2024
@zepumph
Copy link
Member Author

zepumph commented Aug 29, 2024

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 #869. I didn't understand the issue with the listener order. Would you like to discuss that more?

@zepumph
Copy link
Member Author

zepumph commented Sep 5, 2024

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

@zepumph zepumph closed this as completed Sep 5, 2024
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

3 participants