Interactive Highlights should lock with touchSnag

[jessegreenberg]

From phetsims/mean-share-and-balance#125.

Interactive Highlights don't work well for cases of touchSnag. I tried a solution in that issue but it didn't work (ended up reverting). So this issue is to fix it in a better way.

Example of the problem from @Nancy-Salpepi. The higlight should lock to the interactive component once the drag listener gets attached to the Pointer.

Visuals

Highlt.disappears.mov

[marlitas]

I went through the commits that @jessegreenberg reverted in phetsims/mean-share-and-balance#125 and brought those changes back in the above commit. Keeping an eye out on CT now to track down the resulting bugs.

[marlitas]

Looking at CT I see the Assertion failed: It should be impossible to already have a Pointer before locking from touchSnag assertion firing in:

• Center and Variability
• Geometric Optics (basics)
• Kepler's Laws
• Mean Share and Balance
• Models of the Hydrogen Atom
• Molarity
• Projectile Data Lab
• Ratio and Proportion
• Waves Intro

[marlitas]

Here's a patch that is helping for debugging:

Subject: [PATCH] debugging
---
Index: js/accessibility/voicing/InteractiveHighlighting.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/voicing/InteractiveHighlighting.ts b/js/accessibility/voicing/InteractiveHighlighting.ts
--- a/js/accessibility/voicing/InteractiveHighlighting.ts	(revision b17aec4f2bcd482f6276e6d646209e4a626affd9)
+++ b/js/accessibility/voicing/InteractiveHighlighting.ts	(date 1706031067168)
@@ -13,6 +13,7 @@
 import { Highlight } from '../../overlays/HighlightOverlay.js';
 import TEmitter from '../../../../axon/js/TEmitter.js';
 import memoize from '../../../../phet-core/js/memoize.js';
+import Property from '../../../../axon/js/Property.js';
 
 // constants
 // option keys for InteractiveHighlighting, each of these will have a setter and getter and values are applied with mutate()
@@ -22,6 +23,8 @@
   'interactiveHighlightEnabled'
 ];
 
+let instanceCounter = 0;
+
 type SelfOptions = {
   interactiveHighlight?: Highlight;
   interactiveHighlightLayerable?: boolean;
@@ -47,6 +50,8 @@
     // at one time.
     private _pointer: null | Pointer = null;
 
+    private _pointerProperty: Property<null | Pointer>;
+
     // A map that collects all of the Displays that this InteractiveHighlighting Node is
     // attached to, mapping the unique ID of the Instance Trail to the Display. We need a reference to the
     // Displays to activate the Focus Property associated with highlighting, and to add/remove listeners when
@@ -83,7 +88,10 @@
     // while the pointer is down over something that uses InteractiveHighlighting.
     private readonly _pointerListener: TInputListener;
 
+    private readonly interactiveID: number = instanceCounter;
+
     public constructor( ...args: IntentionalAny[] ) {
+      instanceCounter++;
       super( ...args );
 
       this._activationListener = {
@@ -94,6 +102,15 @@
         down: this._onPointerDown.bind( this )
       };
 
+      this._pointerProperty = new Property( this._pointer );
+
+      this._pointerProperty.link( pointer => {
+        console.log( 'interactiveID: ', this.interactiveID );
+        this._pointer = pointer;
+        console.log( 'pointer changed', pointer );
+
+      } );
+
       this._changedInstanceListener = this.onChangedInstance.bind( this );
       this.changedInstanceEmitter.addListener( this._changedInstanceListener );
 
@@ -284,7 +301,8 @@
      * a down event or move event which could do further updates on the event Pointer or FocusManager.
      */
     private _onPointerEntered( event: SceneryEvent<MouseEvent | TouchEvent | PointerEvent> ): void {
-
+      console.log( 'pointer number: ', this.interactiveID );
+      console.log( 'pointer entered value: ', this._pointerProperty.value );
       let lockPointer = false;
 
       const displays = Object.values( this.displays );
@@ -344,7 +362,8 @@
      * deactivate the HighlightOverlay. This can also fire when visibility/pickability of the Node changes.
      */
     private _onPointerExited( event: SceneryEvent<MouseEvent | TouchEvent | PointerEvent> ): void {
-
+      console.log( 'pointer number: ', this.interactiveID );
+      console.log( 'pointer exiting', this._pointerProperty.value );
       const displays = Object.values( this.displays );
       for ( let i = 0; i < displays.length; i++ ) {
         const display = displays[ i ];
@@ -359,7 +378,7 @@
                // We do not want to remove the lockedPointerFocus if this event trail has nothing
                // to do with the node that is receiving a locked focus.
                event.trail.containsNode( lockedPointerFocus.trail.lastNode() ) ) ) {
-
+          console.log( 'going to release', this._pointerProperty.value );
           // unlock and remove pointer listeners
           this._onPointerRelease( event );
         }
@@ -371,8 +390,7 @@
      * event, the pointerFocusProperty will have been set first from the `enter` event.
      */
     private _onPointerDown( event: SceneryEvent<MouseEvent | TouchEvent | PointerEvent> ): void {
-
-      if ( this._pointer === null ) {
+      if ( this._pointerProperty.value === null ) {
         let lockPointer = false;
 
         const displays = Object.values( this.displays );
@@ -405,16 +423,20 @@
      * @param [event] - may be called during interrupt or cancel, in which case there is no event
      */
     private _onPointerRelease( event?: SceneryEvent<MouseEvent | TouchEvent | PointerEvent> ): void {
-
+      console.log( 'current pointer value: ', this._pointerProperty.value );
       const displays = Object.values( this.displays );
       for ( let i = 0; i < displays.length; i++ ) {
         const display = displays[ i ];
         display.focusManager.lockedPointerFocusProperty.value = null;
       }
-
-      if ( this._pointer && this._pointer.listeners.includes( this._pointerListener ) ) {
-        this._pointer.removeInputListener( this._pointerListener );
-        this._pointer = null;
+      if ( this._pointerProperty.value && this._pointerProperty.value.listeners.includes( this._pointerListener ) ) {
+        console.log( 'removing listener' );
+        this._pointerProperty.value.removeInputListener( this._pointerListener );
+        this._pointerProperty.value = null;
+      }
+      else {
+        console.log( 'Pointer Number: ', this.interactiveID );
+        console.log( 'no pointer', this._pointerProperty.value );
       }
     }
 
@@ -437,11 +459,12 @@
      * Save the Pointer and add a listener to it to remove highlights when a pointer is released/cancelled.
      */
     private savePointer( eventPointer: Pointer ): void {
-      assert && assert( this._pointer === null,
+      assert && assert( this._pointerProperty.value === null,
         'It should be impossible to already have a Pointer before locking from touchSnag' );
 
-      this._pointer = eventPointer;
-      this._pointer.addInputListener( this._pointerListener );
+      console.log( 'pointer assigned' );
+      this._pointerProperty.value = eventPointer;
+      this._pointerProperty.value.addInputListener( this._pointerListener );
     }
 
     /**
@@ -457,10 +480,16 @@
       // If the event Pointer is attached to a PressListener there is some activation input happening, so
       // we should "lock" the highlight to this target until the pointer is released.
       if ( eventPointer.attachedListener && eventPointer.attachedListener.listener instanceof PressListener ) {
-        assert && assert( this._pointer === null,
+        console.log( 'pointer number: ', this.interactiveID );
+        console.log( 'pointer value', this._pointerProperty.value );
+        if ( this._pointerProperty.value !== null ) {
+          debugger;
+        }
+        assert && assert( this._pointerProperty.value === null,
           'It should be impossible to already have a Pointer before locking from touchSnag' );
 
         // A COPY of the focus is saved to the Property because we need the value of the Trail at this event.
+        console.log( 'pointer locked focus set' );
         focusManager.lockedPointerFocusProperty.set( new Focus( newFocus.display, newFocus.trail.copy() ) );
         pointerLock = true;
       }

[zepumph]

This is still failing many sims on CT. Is there anything I can do to help with this?

Uncaught Error: Assertion failed: It should be impossible to already have a Pointer before locking from touchSnag

[marlitas]

@zepumph we are currently meeting with @jessegreenberg after having done some investigation on our own that did not get very far.

[jessegreenberg]

@jbphet @marlitas and I paired on this issue for a while today. We found a a problem in InteractiveHighlighting where it was not updating itself if the FocusManager.lockedPointerFocusProperty was set or cleared outside of its class - it incorrectly assumed it had full ownership of the Property.

Hopefully fixed by adding a listener to FocusManager.lockedPointerFocusProperty that will update InteractiveHighlighting state when the Property changes. I am not seeing errors from local aqua and touch snag case in phetsims/mean-share-and-balance#125 still seems fixed.

@jbphet @marlitas can you please review this commit?

[marlitas]

There was a small grammar thing I committed above (@jessegreenberg please feel free to change if I didn't get the right sentiment).

The commit looks good to me, I think I'm more curious about what is updating lockedPointerFocusProperty outside of InteractiveHighlight... is that expected and okay? I probably don't actually need the details. Just was curious.

Thanks for the fix Jesse!

[jessegreenberg]

It is probably not the best software design, but lockedPointerFocusProperty belongs to the scenery Display's FocusManager - a Display-global object that controls state of focus highlights. Each instance of InteractiveHighlighting controls that Property based on user input. But the Property is also set elsewhere in scenery.

Unfortunately, I am seeing the assertion from this commit on CT so we are not finished yet. Adding my assignment back but I might not be able to revisit until next week. This issue blocks RCs, if anyone needs to publish we should revert all commits in this issue.

EDIT: Actually, I am going to revert all changes in this issue so get main stable again until we can fix the problem.

EDIT: Changed my mind again. We will look into this soon. This is an example where branching processes would be helpful.

[jessegreenberg]

Here are the currently known CT errors from this issue:

geometric-optics-basics : multitouch-fuzz : unbuilt
http://128.138.93.172/continuous-testing/ct-snapshots/1706134356763/geometric-optics-basics/geometric-optics-basics_en.html?continuousTest=%7B%22test%22%3A%5B%22geometric-optics-basics%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1706134356763%22%2C%22timestamp%22%3A1706135755567%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Uncaught Error: Assertion failed: It should be impossible to already have a Pointer before locking from touchSnag
Error: Assertion failed: It should be impossible to already have a Pointer before locking from touchSnag
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1706134356763/assert/js/assert.js:28:13)
at assert (InteractiveHighlighting.ts:464:18)
at attemptHighlightLock (InteractiveHighlighting.ts:300:31)

mean-share-and-balance : multitouch-fuzz : unbuilt
http://127.0.0.1/continuous-testing/ct-snapshots/1706178011278/mean-share-and-balance/mean-share-and-balance_en.html?continuousTest=%7B%22test%22%3A%5B%22mean-share-and-balance%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1706178011278%22%2C%22timestamp%22%3A1706191801021%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Error: Assertion failed: this listener still on the lockedPointerFocusProperty indicates a memory leak
window.assertions.assertFunction<@http://127.0.0.1/continuous-testing/ct-snapshots/1706178011278/assert/js/assert.js:28:13
at window.assertions.assertFunction< (http://127.0.0.1/continuous-testing/ct-snapshots/1706178011278/assert/js/assert.js:28:13)
at assert (InteractiveHighlighting.ts:491:18)
at attemptHighlightLock (InteractiveHighlighting.ts:407:31)
at inputEvent (Input.ts:1907:69)
at dispatchToListeners (Input.ts:1947:11

EDIT: Here is what I see on CT

I let MSaB and RaP fuzz for ten minutes with ?brand=phet&ea&interactiveHighlightsInitiallyEnabled&disableModals&fuzz&fuzzPointers=2 and have not it it yet.

EDIT: Found a way to reproduce the problem thanks to a stack trace from the Tambo failure.

1. Open the PhET Menu
2. Press and hold with a finger on the About... menu item
3. Tap somewhere else in the sim to close the menu. You will see the highlight incorrectly visible.
4. Open the phet menu again and try to click on the About... item. This will cause the assertion.

EDIT: These steps cause problems for published sims as well. I believe that this is not a new problem, but that it is being caught by a new assertion.

[jessegreenberg]

OK - I think I found the fix. The listener was not being cleared when the Node was removed the scene graph. In addition, setting of the lockedFocusHighlightProperty was incomplete, so a FocusDisplayedController was added for it in the FocusManager. This is working well for aqua for me locally. Lets watch CT and very this is better before next steps.

[jessegreenberg]

OK, this appears to be fixed on CT and ready to review again. What a doozy! @marlitas @jbphet, can you please review and is there anything else to test before closing?

[jbphet]

I just did a quick review. For the most part, the behavior and the code look good. I have two observations:

• There is a comment in FocusManager that was added that says, "NOTE: It is possible that the addition of the FocusDisplayedController for lockedPointerFocusProperty makes this unnecessary, but it is left in for now." This seems like a potential challenge for future maintainers. It says "for now" - should they consider removing it? How would they know if it is safe to do so? I'd suggest testing and seeing if it can be removed and, if not, modify the comment to explain why it is necessary.
• All of the touch-snag scenarios that I tested worked fine except for the one in center-and-variability where a ball is grabbed on a stack, other balls are kicked on top of it, then the lower ball is dragged. The drag works fine, but the highlight stays focused on the top ball of the screen. I also saw some other odd behavior. To be clear, I think this is a super unusual case and may well be rare enough that it isn't worth addressing, but I thought I'd mention it since it was one of the cases we were testing earlier that was shown to be problematic.

I'm going to unassign myself but leave @marlitas take a look before passing it back to @jessegreenberg.

[marlitas]

I agree with the comments @jbphet made above and have no other notes.

Concerning the second point... We might run into this odd behavior again in the fourth screen of Mean Share and Balance, but I think it will be best to address it at that time rather than try to do it proactively without active development.

[jessegreenberg]

Thank you both for the review - that is a great point. I made the note a TODO and will come back to this next week.

But for now, CT is clear and this is no longer blocking publication.

[kathy-phet]

See if this related issue is fixed: phetsims/scenery-phet#708

[jessegreenberg]

In the above commits I made two changes.

First, the highlights lock into place more frequently. I noticed in buoyancy that the highlights didn't lock because the dragging doesn't use a PressListener. Locking only when the attached listener is a PressListener is added in 026b2e6 and it wasn't clear why. So I removed that part and it behaves more like it did before this issue.

Then I convinced myself that 992ac9f replaces an old option of FocusDisplayedController called onFocusRemove. So I removed it, addressing the TODO.

I let sims that support alt input run locally with ?fuzz&fuzzPointers=2&interactiveHighlightsInitiallyEnabled&disableModals , and did not see any errors. I also tested BASE, buoyancy, and quadrilateral to look for problems.

Ill let CT test this for a few iterations before closing.

[jessegreenberg]

There are two green columns in CT since these commits. I think we are ready to close.