Multi-input: Assertion failed: The shape of the Node highlight should be set by now. Does it have bounds?

[zepumph]

I can trigger this error with brand=phet-io&phetioStandalone&ea&debugger&preferencesStorage&fuzz&fuzzBoard&disableModals.

This is most likely because we focus something before we have set the shape for the focusHighlight from an empty shape.

Uncaught Error: Assertion failed: The shape of the Node highlight should be set by now. Does it have bounds?
    at window.assertions.assertFunction (assert.js:45:13)
    at HighlightOverlay.activateHighlight (HighlightOverlay.ts:319:19)
    at HighlightOverlay.activateFocusHighlight (HighlightOverlay.ts:380:10)
    at HighlightOverlay.onFocusChange (HighlightOverlay.ts:719:12)
    at TinyProperty.notifyLoop (TinyEmitter.ts:213:7)
    at TinyProperty.emit (TinyEmitter.ts:185:18)
    at Property._notifyListeners (ReadOnlyProperty.ts:343:23)
    at Property.unguardedSet (ReadOnlyProperty.ts:287:14)
    at Property.set (ReadOnlyProperty.ts:267:12)
    at set value (Property.ts:54:11)
    at set pdomFocus (FocusManager.ts:274:43)
    at FocusManager.updatePDOMFocusFromEvent (FocusManager.ts:213:35)
    at onfocusin (BrowserEvents.js:768:18)
    at KeyboardFuzzer.chooseNextElement (KeyboardFuzzer.ts:70:21)
    at KeyboardFuzzer.fuzzBoardEvents (KeyboardFuzzer.ts:159:12)
    at SimDisplay.fuzzInputEvents (SimDisplay.ts:232:27)
    at Sim.runAnimationLoop (Sim.ts:1026:45)

This is likely the troubled area:

density-buoyancy-common/js/common/view/MassView.ts

Lines 114 to 119 in 1442534

	// If the cursor is over the mass, or if the mass has keyboard focus, show the interactive highlight.
	Multilink.multilink( [ this.isCursorOverProperty, this.isKeyboardFocusedProperty ], ( isCursorOver, isKeyboardFocused ) => {
	this.focusablePath!.shapeProperty = isCursorOver || isKeyboardFocused ?
	this.focusableShapeProperty : emptyShapeProperty;
	} );

We may decide to not fix this since it is about mouse/keyboard combined input, but I thought I'd log it.

[samreid]

I triggered that assertion in about 60 seconds of fuzzing. We create new HighlightPath( null ), it is unclear why HighlightOverlay expects them to have a non-null shape. Also, if that assertion was stripped, it is difficult to see what incorrect behavior would result:

[zepumph]

Agreed! Maybe we can convince @jessegreenberg that this patch is ok, and that you can show an invisible highlight when the shape is null. This will solve our race condition. Let's ask. Can we commit this? I was able to fuzz for 5 minutes and then I hit #390 instead of this error.

Subject: [PATCH] better autoselect behavior for Mass.ts, https://github.com/phetsims/density-buoyancy-common/issues/387
---
Index: js/overlays/HighlightOverlay.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/overlays/HighlightOverlay.ts b/js/overlays/HighlightOverlay.ts
--- a/js/overlays/HighlightOverlay.ts	(revision 03648177f22989f7ee79ab3342a06f4a719e4be7)
+++ b/js/overlays/HighlightOverlay.ts	(date 1725576535650)
@@ -315,10 +315,7 @@
 
       // if using a focus highlight from another node, we will track that node's transform instead of the focused node
       if ( highlight instanceof HighlightPath ) {
-        const highlightPath = highlight;
-        assert && assert( highlight.shape !== null, 'The shape of the Node highlight should be set by now. Does it have bounds?' );
-
-        if ( highlightPath.transformSourceNode ) {
+        if ( highlight.transformSourceNode ) {
           trailToTrack = highlight.getUniqueHighlightTrail( this.trail );
         }
       }

[jessegreenberg]

The commit message for the added assertion indicates this was used to help track down another problem that has since been fixed so I am fine removing this. phetsims/scenery@b7099c1.

I did so. Back to @samreid and @zepumph

[zepumph]

This is helpful in understanding the highlight system thanks. While reviewing the commit again I realized that this was also showing an improvement that we could make to MassView as well, so there is no null default to possibly set as the shape. @samreid can you please spotcheck and feel free to close?

[samreid]

Looks great, thanks, closing.