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

Missing interactive highlights for some objects #141

Closed
Nancy-Salpepi opened this issue Apr 6, 2023 · 6 comments
Closed

Missing interactive highlights for some objects #141

Nancy-Salpepi opened this issue Apr 6, 2023 · 6 comments
Assignees
Labels

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip

Operating System
13.2.1

Browser
Safari

Problem description
For phetsims/qa#927, the bodies and velocity vectors are missing interactive highlights for the mouse.

Steps to reproduce

  1. Turn on interactive highlights
  2. Hover over bodies and velocity vectors

Visuals
Here is a video showing the objects missing interactive highlights (first moving my mouse, and then using Tab)

interactiveHighlight.mp4
@Nancy-Salpepi Nancy-Salpepi added the type:bug Something isn't working label Apr 6, 2023
@samreid samreid self-assigned this Apr 7, 2023
@samreid
Copy link
Member

samreid commented Apr 7, 2023

I was able to reproduce the same problem, using &interactiveHighlightsInitiallyEnabled

@samreid
Copy link
Member

samreid commented Apr 7, 2023

onPointerFocusChange is being called for other nodes but not for BodyNode or the velocity. @jessegreenberg likely has expertise in this area.

@jessegreenberg
Copy link
Contributor

Hey there! Information about instrumenting for Interactive Highlights can be found at interactive-highlights-quickstart-guide.md. Those components likely need to make use of InteractiveHighlighting.ts or InteractiveHighlightingNode.ts.

If you have any questions or would like to quickly pair on this together, feel free to reach out any time next week.

@jessegreenberg jessegreenberg removed their assignment Apr 7, 2023
AgustinVallejo added a commit to phetsims/solar-system-common that referenced this issue Apr 9, 2023
@AgustinVallejo
Copy link
Contributor

Thanks @jessegreenberg for sharing the document, this should be fixed now. However, I'm not sure if the way I implemented it is correct in one place: For the draggable vector the dragListener is on a Path constant. To add interactive highlights I created a class called InteractivePath and replaced the new Path, as you can see here: phetsims/solar-system-common@f17ffa3#diff-912a31ebd4d8da51db8db8f7aa959c83942c9c51c34289e7c869e4d3214027b0

Would that be okay? Is there a better way? I'm open for pairing on this.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Apr 10, 2023

Those changes look good @AgustinVallejo and its working well from what I can see, well done! Making an inner class for the Path seems reasonable.

One potential thing - UI components that use InteractiveHighlighting may need disposal if they are every destroyed. You might want to check if that is the case for any usages in this sim. I forgot about this in the quick start guide, I added a note about it.

@jessegreenberg jessegreenberg removed their assignment Apr 10, 2023
jessegreenberg added a commit to phetsims/phet-info that referenced this issue Apr 10, 2023
@samreid
Copy link
Member

samreid commented Apr 10, 2023

One potential thing - UI components that use InteractiveHighlighting may need disposal if they are every destroyed.

In ValuesColumnNode, it creates InteractiveNumberDisplay. Should those every be disposed (I can't see if they are already disposed or not).

Here's a patch that disposes the InteractiveHighlight in DraggableVectorNode:

Subject: [PATCH] Dispose
---
Index: js/view/DraggableVectorNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/view/DraggableVectorNode.ts b/js/view/DraggableVectorNode.ts
--- a/js/view/DraggableVectorNode.ts	(revision 516a2d96832e7b18a29add41ddf329fbcede1ab0)
+++ b/js/view/DraggableVectorNode.ts	(date 1681142885289)
@@ -180,8 +180,8 @@
       dragListener.dispose();
       keyboardDragListener.dispose();
       vectorPositionProperty.dispose();
+      grabArea.dispose();
     } );
-
 
     // For PhET-iO, when the node does not support input, don't show the drag circle
     const onInputEnabled = ( inputEnabled: boolean ) => {

I also wanted to comment that it seems unwieldy to have to create a subclass every time you want to add interactive highlights to something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants