diff --git a/js/accessibility/FocusDisplayedController.ts b/js/accessibility/FocusDisplayedController.ts index a603026f6..05dbc7ff9 100644 --- a/js/accessibility/FocusDisplayedController.ts +++ b/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 | null; + private focusProperty: TProperty; // 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; } } diff --git a/js/accessibility/voicing/InteractiveHighlighting.ts b/js/accessibility/voicing/InteractiveHighlighting.ts index 4b3d06356..d4b0b6020 100644 --- a/js/accessibility/voicing/InteractiveHighlighting.ts +++ b/js/accessibility/voicing/InteractiveHighlighting.ts @@ -427,14 +427,11 @@ const InteractiveHighlighting = memoize( >( 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.