Unable to fully manipulate objects when zoomed in

[KatieWoe]

Test device
iPad
Operating System
iPadOS 15.1
Browser
Safari
Problem description
For phetsims/qa#747. This is most impactful on iPad/touch devices since there is only one way to zoom and pan.
When zoomed in on an object, if you click on it to bring up options to alter it (such as changing a battery's voltage) and then try to drag the screen down to view those options, the options go away. Since you are clicking out side the object to drag the screen, the options are hidden. You need to zoom out to both select the object and view the options. Or move the object closer.
This seems to be a very minor issue and can likely be dismissed.

Visuals

IMG_0086.MP4

[samreid]

Should we keep the object selected during pan/zoom? @arouinfar or @jessegreenberg can you please review and comment?

[arouinfar]

If you are on a computer, you can pan by scrolling and it won't dismiss the selection. It would be nice to keep the object selected while panning on a touch device, but I don't know if there is an easy way to detect the pan swipe action or if it is distinguishable from other touch events.

If there is a straightforward way to keep the object selected during pan/zoom, I think it's worth doing. If not, I'm fine with closing as wont-fix.

[samreid]

Is it important to deselect the selected object if there is a tap in the play area that doesn't initiate or continue pan/zoom?

[arouinfar]

Is it important to deselect the selected object if there is a tap in the play area that doesn't initiate or continue pan/zoom?

Yes, it's important to be able to deselect an object by tapping in the play area.

[samreid]

@jessegreenberg can you please review this issue and advise?

I slacked @jessegreenberg:

Hi Jesse, we had a question about pan/zoom in #789. CCK has a feature where you can select an element by clicking on it, and clicking away should deselect it. But can we also pan/zoom when something is selected without deselecting it?

[samreid]

@arouinfar and I discussed that we would need to change "deselect on touch down" to "deselect on touch up", but then we would also need to guard on whether the touch up participated in a pan/zoom event. We'll wait to hear from @jessegreenberg about that part. We also discussed that it's not an ideal UX to have to pan around looking for controls when zoomed in.

This problem is already present in the published CCK DC version.

[jessegreenberg]

I wanted to see if we could prevent this from happening by checking if the Pointer had an attached listener in the clickToDismissListener. But that didn't work because of the order of listeners. On down, the pan/zoom listener isn't attached yet because it gets attached on move. The up event reaches the MultiListener before an up in the clickToDismissListener so event.pointer.isAttached() is false then too.

I got this working as we want with this patch, but I am not sure if this is best as a general pattern. It works by attaching the listener to the Pointer on a down event, and dismissing the controls on an up event on the Pointer. If a pan starts, the Pointer listener gets interrupted by MultiListener and the dismiss doesn't happen.

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 b5d1a57deeddb36b2ae76de3a4f117a6ec8948fa)
+++ b/js/view/CircuitElementNode.ts	(date 1638400775916)
@@ -10,9 +10,7 @@
 import dotRandom from '../../../dot/js/dotRandom.js';
 import Vector2 from '../../../dot/js/Vector2.js';
 import merge from '../../../phet-core/js/merge.js';
-import { KeyboardUtils } from '../../../scenery/js/imports.js';
-import { SceneryEvent } from '../../../scenery/js/imports.js';
-import { Node } from '../../../scenery/js/imports.js';
+import { KeyboardUtils, Node, Pointer, SceneryEvent } from '../../../scenery/js/imports.js';
 import CCKCConstants from '../CCKCConstants.js';
 import circuitConstructionKitCommon from '../circuitConstructionKitCommon.js';
 import Circuit from '../model/Circuit.js';
@@ -193,7 +191,7 @@
   }
 
 // @public overridden in subclasses
-  abstract updateRender():void;
+  abstract updateRender(): void;
 
   /**
    * Handles when the node is dropped, called by subclass input listener.
@@ -250,17 +248,10 @@
 
       const disposeListener = () => phet.joist.display.removeInputListener( clickToDismissListener );
 
-      // listener for 'click outside to dismiss'
-      const clickToDismissListener = {
-
-        down: ( event: any ) => {
+      let pointer: Pointer | null = null;
+      const pointerListener = {
 
-          // When fuzzing, don't click away from the circuit element so eagerly, so that fuzzing has more of a chance to
-          // press the associated controls.
-          if ( phet.chipper.isFuzzEnabled() && dotRandom.nextDouble() < 0.99 ) {
-            return;
-          }
-
+        up: ( 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 ) => {
@@ -272,11 +263,39 @@
 
           if ( trails.length === 0 ) {
             phet.joist.display.removeInputListener( clickToDismissListener );
+            pointer!.removeInputListener( pointerListener );
+            pointer = null;
+
             if ( this.disposeEmitterCircuitElementNode.hasListener( disposeListener ) ) {
               this.disposeEmitterCircuitElementNode.removeListener( disposeListener );
             }
             circuitLayerNode.circuit.selectedCircuitElementProperty.set( null );
           }
+        },
+
+        interrupt: () => {
+          pointer!.removeInputListener( pointerListener );
+          pointer = null;
+        }
+      };
+
+      // listener for 'click outside to dismiss'
+      const clickToDismissListener = {
+
+        down: ( event: any ) => {
+
+          // When fuzzing, don't click away from the circuit element so eagerly, so that fuzzing has more of a chance to
+          // press the associated controls.
+          if ( phet.chipper.isFuzzEnabled() && dotRandom.nextDouble() < 0.99 ) {
+            return;
+          }
+
+          // Add the dismiss listener to the Pointer and attach it - if a pan starts while the Pointer
+          // is still down the Pointer listener will be interrupted and dismissal will never happen.
+          if ( !pointer ) {
+            pointer = event.pointer;
+            event.pointer.addInputListener( pointerListener, true );
+          }
         }
       };
       phet.joist.display.addInputListener( clickToDismissListener );

[jessegreenberg]

The above seems complicated but I can't think of anything better at the moment. There are many cases of adding a clickToDismissListener to phet.joist.display in the project and I would guess this is a problem for all of them. Maybe we can abstract the clickToDismissListener so that it does the work in #789 (comment) for us. I think that is the way I would recommend trying now.

[samreid]

I reviewed the patch and it seemed reasonable. In testing it seemed to work OK to select an element, then pan/zoom without losing selection. I also tested that tapping and releasing in the play area cleared the selected element. However, in testing a little more, I received this error:

assert.js:25 Uncaught Error: Assertion failed: Attempted to attach to an already attached pointer
    at window.assertions.assertFunction (assert.js:25)
    at Mouse.attach (Pointer.js? [sm]:286)
    at Mouse.addInputListener (Pointer.js? [sm]:175)
    at Object.down (CircuitElementNode.ts:297)
    at Input.dispatchToListeners (Input.js? [sm]:1913)
    at Input.dispatchEvent (Input.js? [sm]:1869)
    at Input.downEvent (Input.js? [sm]:1680)
    at Input.mouseDownAction.Action.phetioPlayback (Input.js? [sm]:302)
    at Action.execute (Action.js? [sm]:227)
    at Input.mouseDown (Input.js? [sm]:1146)

It wasn't clear to me what was causing this error or how to prevent it. Since we are trying to avoid delays for CCK AC release, we may want to take SHAs and work on this in parallel, or omit it from 1.0. In slack @jessegreenberg said:

I am going to create an issue in joist to try a more general solution/pattern

This issue may want to wait on the more general solution/pattern depending on how it goes.

[jessegreenberg]

Thanks @samreid, I am guessing there is an issue with my patch and I am failing to remove the listener on the pointer each release. Will be on the lookout for that in phetsims/joist#770 where I am working on a better way to support this.

[jessegreenberg]

I added a new DisplayClickToDismissListener in phetsims/joist#770 and tested it out in CCK in c71117f. I identified the problem noticed in #789 (comment) (I was trying to add the listener to a Pointer that was already attached with another listener). It seems to be working well, though phetsims/joist#770 is still up for review. I recommend putting this on hold until phetsims/joist#770 is closed in case further issues come out of review, then this issue can be cleaned up at the end.

[samreid]

phetsims/joist#770 still open as of Feb 10, 2022. But we have prioritized reviewing ready-for-review issues this iteration.