Cannot deselect circuit components on the Lab screen

[matthew-blackman]

I am unable to deselect a highlighted circuit component on the Lab screen. This does not occur in CCK-AC or CCK-Virtual Lab. To reproduce the issue:

1. Go to the Lab screen on CCK DC
2. Drag any component from the carousel
3. Select the component to show a yellow highlight
4. Click in the blue area to try and deselect the component

In mine and @samreid's initial testing, the component could not be deselected without selecting another component. This is a high-priority bug to address, since users are likely to run into this frequently on the Lab screen of the published sim.

[KatieWoe]

Confirmed on published. Assigning @samreid

[samreid]

Through investigation with this patch, I have identified the troublesome area:

Subject: [PATCH] Add TODOs, see https://github.com/phetsims/density-buoyancy-common/issues/154
---
Index: js/view/CircuitNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/view/CircuitNode.ts b/js/view/CircuitNode.ts
--- a/js/view/CircuitNode.ts	(revision c6a08ea65f78942da48ed6dca37b3fe04442fcbb)
+++ b/js/view/CircuitNode.ts	(date 1717593895237)
@@ -593,6 +593,7 @@
     }
 
     // listener for 'click outside to dismiss'
+    console.log('add listener');
     phet.joist.display.addInputListener( new DisplayClickToDismissListener( ( event: SceneryEvent ) => {
 
       // if the target was in a CircuitElementEditContainerNode, don't dismiss the event because the user was
@@ -604,8 +605,22 @@
         return node instanceof CircuitElementEditContainerNode || node instanceof CircuitElementNode || node instanceof VertexNode;
       } );
 
+      console.log( 'click to dismiss' );
+      // debugger;
+
       if ( trails.length === 0 ) {
+        const oldSelection = this.circuit.selectionProperty.value;
+
         this.circuit.selectionProperty.value = null;
+        this.circuit.dirty = true;
+        // debugger;
+
+        // iterate over all CircuitElementNodes and mark as dirty
+
+        Object.values( this.circuitElementNodeMap ).forEach( value => {
+          // debugger;
+          value.dirty = true;
+        } );
       }
     } ) );
   }
Index: js/model/Circuit.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/Circuit.ts b/js/model/Circuit.ts
--- a/js/model/Circuit.ts	(revision c6a08ea65f78942da48ed6dca37b3fe04442fcbb)
+++ b/js/model/Circuit.ts	(date 1717593739448)
@@ -282,8 +282,12 @@
       phetioValueType: NullableIO( ReferenceIO( OrIO( [ CircuitElement.CircuitElementIO, Vertex.VertexIO ] ) ) ),
       phetioFeatured: true
     } );
+    this.selectionProperty.link( selection => {
+      console.log( 'selection changed: ', selection === null ? null : selection.phetioID );
+    } );
 
     const emitCircuitChanged = () => {
+      // debugger;
       this.dirty = true;
       this.circuitChangedEmitter.emit();
     };
@@ -903,6 +907,7 @@
    * dirty and compute in step if anything has changed.
    */
   private markDirty(): void {
+    // debugger;
     this.dirty = true;
   }
 
Index: js/view/WireNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/view/WireNode.ts b/js/view/WireNode.ts
--- a/js/view/WireNode.ts	(revision c6a08ea65f78942da48ed6dca37b3fe04442fcbb)
+++ b/js/view/WireNode.ts	(date 1717593497263)
@@ -220,6 +220,7 @@
      * When the view type changes (lifelike vs schematic), update the node
      */
     const markAsDirty = () => {
+      console.log( 'hello mark as dirty' );
       if ( this.isDisposed ) {
         return;
       }
@@ -298,6 +299,7 @@
       } );
       this.addInputListener( this.dragListener );
 
+      console.log('WIRE TO SELECTION PROPERTY');
       circuitNode!.circuit.selectionProperty.link( markAsDirty );
     }
     else {
@@ -355,6 +357,7 @@
    * CCKCLightBulbNode calls updateRender for its child socket node
    */
   public updateRender(): void {
+    console.log( 'updat.e render ' );
     const view = this.viewTypeProperty.value;
     if ( view === CircuitElementViewType.LIFELIKE ) {
 
Index: js/view/CircuitElementNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/view/CircuitElementNode.ts b/js/view/CircuitElementNode.ts
--- a/js/view/CircuitElementNode.ts	(revision c6a08ea65f78942da48ed6dca37b3fe04442fcbb)
+++ b/js/view/CircuitElementNode.ts	(date 1717593184989)
@@ -82,6 +82,7 @@
 
     // Flag to indicate when updating view is necessary, in order to avoid duplicate work when both vertices move
     this.dirty = true;
+    debugger;
     this.disposeEmitter.addListener( () => circuitElement.startDragEmitter.removeListener( startDragListener ) );
   }
 
@@ -89,6 +90,7 @@
    * Mark dirty to batch changes, so that update can be done once in view step, if necessary
    */
   protected markAsDirty(): void {
+    debugger;
     this.dirty = true;
   }
 
@@ -133,7 +135,9 @@
    * called during the view step
    */
   public step(): void {
+    console.log( 'step' );
     if ( this.dirty ) {
+      console.log( 'was dirty' );
 
       this.updateRender();
       this.dirty = false;

The problem is that DisplayClickToDismissListener does not support more than one instance. For instance, testing on main for CCK-AC, neither screen 2 nor 3 can deselect circuit elements. However, if running any single screen with ?screens=2 or ?screens=3, it is fine.

DisplayClickToDismissListener is only used by Circuit Construction Kit, and attaches to pointer itself on press. Hence when there are 3 DisplayClickToDismissListeners, only the 1st one (for screen 1) attaches to the pointer. The others fail to attach because of this line:

https://github.com/phetsims/joist/blob/084b04e87cf07445a11a0c7915984617795ddad1/js/DisplayClickToDismissListener.ts#L59-L65

This code has been out for review in phetsims/joist#770 since Feb 2022. What is the best way to address this problem?

[jessegreenberg]

Thanks for identifying the problem. A solution has been committed in phetsims/joist#770, would you like to review or verify?

It sounds like this is in the published version and we may want to redeploy CCK sims, let me know if I should do that or help with that.

[samreid]

The problem is in the DC sim but not in the AC sim. @kathy-phet @matthew-blackman should we republish? Or just when we publish with changes for data fluency?

I confirmed @jessegreenberg fixes are great, thanks!