clear on disposal instead of using a workaround, #1615 phetsims/densi…

…ty-buoyancy-common#97

Signed-off-by: Michael Kauzmann <[email protected]>

js/accessibility/FocusDisplayedController.ts

@@ -1,4 +1,4 @@
-// Copyright 2021-2024, University of Colorado Boulder
+// Copyright 2021-2023, University of Colorado Boulder
 
 /**
  * Responsible for setting the provided focusProperty to null when the Focused node either
@@ -22,11 +22,12 @@ class FocusDisplayedController {
   private visibilityTracker: TrailVisibilityTracker | null = null;
 
   // When there is value, we will watch and update when there are changes to the displayed state of the Focus trail.
-  private focusProperty: TProperty<Focus | null> | null;
+  private focusProperty: TProperty<Focus | null>;
 
   // Bound functions that are called when the displayed state of the Node changes.
   private readonly boundVisibilityListener: () => void;
   private readonly boundInstancesChangedListener: ( instance: Instance ) => void;
+  private readonly boundNodeDisposedListener: () => void;
 
   // Handles changes to focus, adding or removing listeners
   private readonly boundFocusListener: ( focus: Focus | null ) => void;
@@ -36,6 +37,7 @@ class FocusDisplayedController {
 
     this.boundVisibilityListener = this.handleTrailVisibilityChange.bind( this );
     this.boundInstancesChangedListener = this.handleInstancesChange.bind( this );
+    this.boundNodeDisposedListener = this.handleNodeDisposed.bind( this );
 
     this.boundFocusListener = this.handleFocusChange.bind( this );
     this.focusProperty.link( this.boundFocusListener );
@@ -58,7 +60,7 @@ class FocusDisplayedController {
    */
   private handleTrailVisibilityChange(): void {
     if ( this.visibilityTracker && !this.visibilityTracker.trailVisibleProperty.value ) {
-      this.focusProperty!.value = null;
+      this.focusProperty.value = null;
     }
   }
 
@@ -68,10 +70,19 @@ class FocusDisplayedController {
    */
   private handleInstancesChange( instance: Instance ): void {
     if ( instance.node && instance.node.instances.length === 0 ) {
-      this.focusProperty!.value = null;
+      this.focusProperty.value = null;
     }
   }
 
+  /**
+   * While this focus-clear is mostly covered by listening for instance changes, there is an intermediate state between
+   * when a Node is disposed, and when the Instance tree is updated to reflect that disposal (during updateDisplay()).
+   * This function handles that atypical case (pretty much impossible to get to in PhET sims except during fuzzing).
+   */
+  private handleNodeDisposed(): void {
+    this.focusProperty.value = null;
+  }
+
   /**
    * Add listeners that watch when the Displayed state of the Node with Focus has changed,
    * including visibility of the trail and attachment to a scene graph.
@@ -85,6 +96,7 @@ class FocusDisplayedController {
 
     this.node = focus.trail.lastNode();
     this.node.changedInstanceEmitter.addListener( this.boundInstancesChangedListener );
+    this.node.disposeEmitter.addListener( this.boundNodeDisposedListener );
   }
 
   /**
@@ -99,6 +111,7 @@ class FocusDisplayedController {
     }
     if ( this.node ) {
       this.node.changedInstanceEmitter.removeListener( this.boundInstancesChangedListener );
+      this.node.disposeEmitter.removeListener( this.boundNodeDisposedListener );
       this.node = null;
     }
   }
@@ -107,11 +120,10 @@ class FocusDisplayedController {
 
     // this disposes the TrailVisibilityTracker and removes any listeners on the Node
     this.removeDisplayedListeners();
-    this.focusProperty!.unlink( this.boundFocusListener );
+    this.focusProperty.unlink( this.boundFocusListener );
 
     this.node = null;
     this.visibilityTracker = null;
-    this.focusProperty = null;
   }
 }
 

js/accessibility/voicing/InteractiveHighlighting.ts

@@ -427,14 +427,11 @@ const InteractiveHighlighting = memoize( <SuperType extends Constructor<Node>>(
           const focus = display.focusManager.pointerFocusProperty.value;
           const locked = !!display.focusManager.lockedPointerFocusProperty.value;
 
-          // Workaround for https://github.com/phetsims/density-buoyancy-common/issues/97. We should not try to
-          // highlight lock if the focus is disposed, but it won't be cleaned up until instances change upon next updateDisplay().
-          const focusIsNotDisposedWorkaround = !focus?.trail.lastNode().isDisposed;
-
           // Focus should generally be defined when pointer enters the Node, but it may be null in cases of
           // cancel or interrupt. Don't attempt to lock if the FocusManager already has a locked highlight (especially
           // important for gracefully handling multitouch).
-          if ( focus && !locked && focusIsNotDisposedWorkaround ) {
+          if ( focus && !locked ) {
+            assert && assert( !focus.trail.lastNode().isDisposed, 'Focus should not be set to a disposed Node' );
 
             // Set the lockedPointerFocusProperty with a copy of the Focus (as deep as possible) because we want
             // to keep a reference to the old Trail while pointerFocusProperty changes.