Highlight/focus is hard to remove from a launched sim

[KatieWoe]

Device Samsung OS Win 11 Browser Chrome Problem Description For https://github.com/phetsims/qa/issues/919 If you launch a sim while an element is highlighted, if you click outside it in the play area in the newly launched sim it does not unhighlight. You need to first click either that element or another. Steps to Reproduce

1. In Studio, grab an element such as a battery from the tool box
2. Click the element so it has a yellow focus
3. Launch the sim
4. In the launched sim, click the blue play area

[samreid]

Good discovery, thanks! I reproduced the same problem in CCK DC 1.3. @arouinfar can you please test and characterize the importance of investigation/maintenance release for this?

[samreid]

The problem is that this code:

      const clickToDismissListener = new DisplayClickToDismissListener( dismissListener );
      phet.joist.display.addInputListener( clickToDismissListener );

In VertexNode and CircuitElementNode only runs if the user clicked, not if the selection property was set programmatically (like via phet-io). So the solution would probably be finding a way to move that code out of the click listeners and making it more based on the model property like isSelected. Would we also move it outside of CircuitElement and Vertex -- more to the CircuitNode or ScreenView level, so there is only one of them?

[samreid]

I'm surprised this patch is working in my tests--probably need to review carefully and look for corner cases. Dealing with disposal. A code comment in VertexNode

```diff Subject: [PATCH] Update copyrights, see https://github.com/phetsims/phet-io-wrapper-lab-book/issues/13 --- 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 8bc8f42555bc2f8b613ceb441d09691e92fe3363) +++ b/js/view/CircuitElementNode.ts (date 1678630080417) @@ -7,16 +7,14 @@ */ import Vector2 from '../../../dot/js/Vector2.js'; -import { Node, NodeOptions, PressListenerEvent, SceneryEvent } from '../../../scenery/js/imports.js'; +import { Node, NodeOptions, PressListenerEvent } from '../../../scenery/js/imports.js'; import CCKCConstants from '../CCKCConstants.js'; import circuitConstructionKitCommon from '../circuitConstructionKitCommon.js'; import Circuit from '../model/Circuit.js'; import CircuitElement from '../model/CircuitElement.js'; -import CircuitElementEditContainerNode from './CircuitElementEditContainerNode.js'; import CCKCScreenView from './CCKCScreenView.js'; import CircuitNode from './CircuitNode.js'; import Vertex from '../model/Vertex.js'; -import DisplayClickToDismissListener from '../../../joist/js/DisplayClickToDismissListener.js'; import optionize from '../../../phet-core/js/optionize.js'; import CircuitNodeDragListener from './CircuitNodeDragListener.js'; @@ -194,40 +192,6 @@ if ( !ignoreFocus ) { this.focus(); } - - const disposeListener = () => { - phet.joist.display.removeInputListener( clickToDismissListener ); - clickToDismissListener.dispose(); - }; - - // listener for 'click outside to dismiss' - const dismissListener = ( event: SceneryEvent ) => { - - // if the target was in a CircuitElementEditContainerNode, don't dismiss the event because the user was - // dragging the slider or pressing the trash button or another control in that panel - const trails = event.target.getTrails( ( node: Node ) => { - - // If the user tapped any component in the CircuitElementContainerPanel or on the selected node - // allow interaction to proceed normally. Any other taps will deselect the circuit element - return node instanceof CircuitElementEditContainerNode || node === this; - } ); - - if ( trails.length === 0 ) { - disposeListener(); - if ( this.disposeEmitter.hasListener( disposeListener ) ) { - this.disposeEmitter.removeListener( disposeListener ); - } - circuitNode.circuit.selectionProperty.value = null; - } - }; - - const clickToDismissListener = new DisplayClickToDismissListener( dismissListener ); - phet.joist.display.addInputListener( clickToDismissListener ); - - // If the user deletes the element with the delete button, make sure to detach the display input listener - // so the next drag will work right away, - // see https://github.com/phetsims/circuit-construction-kit-common/issues/368 - this.disposeEmitter.addListener( disposeListener ); } } } Index: js/view/VertexNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/VertexNode.ts b/js/view/VertexNode.ts --- a/js/view/VertexNode.ts (revision 8bc8f42555bc2f8b613ceb441d09691e92fe3363) +++ b/js/view/VertexNode.ts (date 1678630169932) @@ -210,21 +210,21 @@ vertex.selectionProperty.value = vertex; - const dismissListener = ( event: SceneryEvent ) => { - if ( !_.includes( event.trail.nodes, this ) && !_.includes( event.trail.nodes, this.vertexCutButtonContainer ) ) { - vertex.selectionProperty.value = null; - this.clearClickListeners(); - } - }; - const clickToDismissListener = new DisplayClickToDismissListener( dismissListener ); - phet.joist.display.addInputListener( clickToDismissListener ); - this.clickToDismissListeners.push( clickToDismissListener ); + // const dismissListener = ( event: SceneryEvent ) => { + // if ( !_.includes( event.trail.nodes, this ) && !_.includes( event.trail.nodes, this.vertexCutButtonContainer ) ) { + // vertex.selectionProperty.value = null; + // // this.clearClickListeners(); + // } + // }; + // const clickToDismissListener = new DisplayClickToDismissListener( dismissListener ); + // phet.joist.display.addInputListener( clickToDismissListener ); + // this.clickToDismissListeners.push( clickToDismissListener ); } - else { - - // Deselect after dragging so a grayed-out cut button doesn't remain when open vertex is connected - this.clearClickListeners(); - } + // else { + // + // // Deselect after dragging so a grayed-out cut button doesn't remain when open vertex is connected + // this.clearClickListeners(); + // } } } ); @@ -272,7 +272,7 @@ circuit.circuitChangedEmitter.removeListener( this.updateStrokeListener ); // Remove the global listener if it was still enabled - this.clearClickListeners(); + // this.clearClickListeners(); this.dragListener.dispose(); this.removeInputListener( this.dragListener ); @@ -388,13 +388,13 @@ /** * Remove click listeners */ - private clearClickListeners(): void { - this.clickToDismissListeners.forEach( listener => { - phet.joist.display.removeInputListener( listener ); - listener.dispose(); - } ); - this.clickToDismissListeners.length = 0; - } + // private clearClickListeners(): void { + // this.clickToDismissListeners.forEach( listener => { + // phet.joist.display.removeInputListener( listener ); + // listener.dispose(); + // } ); + // this.clickToDismissListeners.length = 0; + // } /** * Sets whether the node is draggable, used as a callback for interrupting the drag listener 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 8bc8f42555bc2f8b613ceb441d09691e92fe3363) +++ b/js/view/CircuitNode.ts (date 1678630169919) @@ -19,7 +19,7 @@ import Bounds2 from '../../../dot/js/Bounds2.js'; import Utils from '../../../dot/js/Utils.js'; import Vector2 from '../../../dot/js/Vector2.js'; -import { Node, Path } from '../../../scenery/js/imports.js'; +import { Node, Path, SceneryEvent } from '../../../scenery/js/imports.js'; import scissorsShape from '../../../sherpa/js/fontawesome-4/scissorsShape.js'; import RoundPushButton from '../../../sun/js/buttons/RoundPushButton.js'; import Tandem from '../../../tandem/js/Tandem.js'; @@ -68,6 +68,8 @@ import DogNode from './DogNode.js'; import ResistorType from '../model/ResistorType.js'; import AmmeterConnection from '../model/AmmeterConnection.js'; +import CircuitElementEditContainerNode from './CircuitElementEditContainerNode.js'; +import DisplayClickToDismissListener from '../../../joist/js/DisplayClickToDismissListener.js'; // constants @@ -570,6 +572,32 @@ else { this.circuitDebugLayer = null; } + + // listener for 'click outside to dismiss' + phet.joist.display.addInputListener( new DisplayClickToDismissListener( ( event: SceneryEvent ) => { + + // if the target was in a CircuitElementEditContainerNode, don't dismiss the event because the user was + // dragging the slider or pressing the trash button or another control in that panel + const trails = event.target.getTrails( ( node: Node ) => { + + // If the user tapped any component in the CircuitElementContainerPanel or on the selected node + // allow interaction to proceed normally. Any other taps will deselect the circuit element + return node instanceof CircuitElementEditContainerNode || node instanceof CircuitElementNode || node instanceof VertexNode; + } ); + + if ( trails.length === 0 ) { + // disposeListener(); + // if ( this.disposeEmitter.hasListener( disposeListener ) ) { + // this.disposeEmitter.removeListener( disposeListener ); + // } + this.circuit.selectionProperty.value = null; + } + } ) ); + + // If the user deletes the element with the delete button, make sure to detach the display input listener + // so the next drag will work right away, + // see https://github.com/phetsims/circuit-construction-kit-common/issues/368 + // this.disposeEmitter.addListener( disposeListener ); } /** ```

[samreid]

I feel a good next step would be design and code review/testing (probably in a mini-meeting). Once approved, we can cherry-pick this into DC and VL.

[zepumph]

I applied the patch and poked through it. The patch makes sense, but too much seemlingly important stuff was commented out. So it feels like pairing-to-proceed (ptp) is the best path forward on my end. Let me know if you want to schedule something.

[zepumph]

An MR will look something like this:

 (turn off transpiler)
node js/scripts/master-pull-status.js --allBranches
Maintenance.reset();
Maintenance.checkBranchStatus();

Maintenance.reset();
Maintenance.createPatch( 'circuit-construction-kit-common', 'https://github.com/phetsims/circuit-construction-kit-common/issues/988' );
Maintenance.addNeededPatch( 'circuit-construction-kit-dc', '1.3', 'circuit-construction-kit-common' );
Maintenance.addNeededPatch( 'circuit-construction-kit-dc-virtual-lab', '1.3', 'circuit-construction-kit-common' );
Maintenance.addPatchSHA( 'circuit-construction-kit-common', 'c9d6d8ddd2b54e72cb9025360f84db82fdad76f0' );
Maintenance.applyPatches();
Maintenance.updateDependencies();

[zepumph]

@arouinfar will verify on master, and then we will continue on with the patching above.

1. Apply above commands in an MR
2. Maintenance.deployReleaseCandidates() (will do both), or kick off each separately, depending on what is best.
3. Maintenance.deployProduction() (will just deploy dc, not virtual lab because it isn't released yet).
4. grunt production in virtual lab to finally publish it when ready.

[arouinfar]

Looks good in master, back to you @samreid.

[samreid]

In running the node js/scripts/master-pull-status.js --allBranches, we see:

scenery-phet ERROR: Error: git pull --rebase in ../scenery-phet failed with exit code 1
stderr:
There is no tracking information for the current branch.
Please specify which branch you want to rebase against.
See git-pull(1) for details.

    git pull <remote> <branch>

If you wish to set tracking information for this branch you can do so with:

    git branch --set-upstream-to=origin/<branch> gravity-and-orbits-1.6

-----=====] finished pulls [=====-----

-----=====] finished npm [=====-----

So we will adjust the script to set the tracking information if pull fails.

[samreid]

Maintenance.deployReleaseCandidates() (will do both), or kick off each separately, depending on what is best.

This step looks like it only ran for DC, not for VL.

[samreid]

Deployed: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc/1.3.2-rc.1/phet/circuit-construction-kit-dc_en_phet.html Deployed: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc/1.3.2-rc.1/phet-io/ Please wait for the build-server to complete the deployment, and then test! RC versions deployed

...

Deployed: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc-virtual-lab/1.3.0-rc.2/phet/circuit-construction-kit-dc-virtual-lab_en_phet.html Deployed: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc-virtual-lab/1.3.0-rc.2/phet-io/ Please wait for the build-server to complete the deployment, and then test!

[KatieWoe]

Looks good for both DC and VL