Objects can be picked up as you enter screen

[KatieWoe]

Device
iPad 6th Gen
OS
iPadOS 14.4
Browser
Safari
Problem Description
For phetsims/qa#600. Seems to be a touch issue. When on the home screen, if you enter one of the screens by tapping a part of the icon that turns in to the object, the object will be picked up as you enter the screen. This is done without lifting your finger.

Steps to Reproduce

1. Go to the home screen
2. Enter the first screen ty tapping on the left side of the icon. Your finger will end up over the red chips
3. May take a few attempts to get positioning right.

Visuals

Image.from.iOS.3.MP4

[jbphet]

This is a common code problem, not an issue unique to the sim. The reasons that we've never run into it before are that:

• On this sim, there are things (coins) right behind the screen selection icon
• The coins move when being dragged to be higher than the drag point so that they are easy to see when they are being dragged with a finger

I duplicated the behavior in Energy Forms and Changes by creating a stack of blocks that were behind the "Intro" screen selection icon on the home screen and adding code to indicate when the iron block was being dragged (a.k.a. "user controlled"). I could touch the "Intro" screen selection icon on the home screen and end up with the block in the user-controlled state. Here's a screen shot (my finger is on the block):

So I have two questions about this for @samreid, since I think he did most of the work on the home screen:

• Why do transition to the screens on a touch event and not a touch-and-release? I just messed around with my iPad for a while, and this doesn't seem like typical behavior. In fact, our other buttons, including our nav bar, don't do this - they wait for touch release.
• Why doesn't the home screen "absorb" the touch event instead of allowing it to be sent to the selected screen?

If we decide to do something about this, we should log an issue in the appropriate repo and reference this one, or move it.

In case we need it, here is the debug code that I added to EFACIntroScreenView:

    const debugReadout = new Text( 'Hey what is', {
      font: new PhetFont( 20 ),
      fill: 'red',
      top: 300,
      left: 100
    } );
    this.addChild( debugReadout );

    model.blockGroup.getElement(0).userControlledProperty.link( userControlled => {
      debugReadout.text = 'User controlled:' + userControlled;
    } );

[jbphet]

@amanda-phet - This seems like a rare enough use case, and the consequences so minor, that I don't think that it should block the release of this sim. Agreed?

[samreid]

Why do transition to the screens on a touch event and not a touch-and-release? I just messed around with my iPad for a while, and this doesn't seem like typical behavior. In fact, our other buttons, including our nav bar, don't do this - they wait for touch release.

I think this is probably an oversight--it seems better to treat them like normal buttons that require a release for activation. I don't recall any specific design decisions to make them fire on down. Will we also want to show a different highlight for pressed/armed/pressed-out, etc? Should this be part of joist 2.0?

[samreid]

@jbphet and I reviewed the behavior. We tried the following approaches:

(1) We tried all sorts of interruptions, and couldn't get any of them to work:

          phet.joist.display.interruptInput();
          phet.joist.display.rootNode.interruptSubtreeInput();
          event.pointer.interruptAll();
          event.pointer.interruptAttached();
          console.log( 'bonjour' );
          homeScreenModel.screenProperty.value = screen;
          screen.view.interruptSubtreeInput();

We were surprised by this--we thought at least one of these interruptions would stop the behavior. Perhaps @jonathanolson can help us understand this better?

(2) We changed fireOnDown: true, // to match prior behavior to false, and by also getting rid of the touchover listener, it worked pretty well because the buttons fire on release (but we didn't fully test this approach), but it lost the "swipe across icons" behavior that we have had since joist's inception. We weren't sure how important that swipe behavior is. If we want to keep the swipe behavior, maybe there is a way to implement it. But we thought we would check whether it is necessarily at all. @jbphet remarked that the swipe behavior doesn't seem too important.

This probably shouldn't block Number Line Operations because the sim is still usable after accidentally grabbing something in a sim screen.

We decided to move this issue to joist.

@jbphet volunteered to investigate (2) and run it past designers if it is working well.

[jonathanolson]

We were surprised by this--we thought at least one of these interruptions would stop the behavior. Perhaps @jonathanolson can help us understand this better?

My hypothesis (without any confirmation, but makes sense) is:

1. User presses on the home-screen button
2. FireListener attaches to the Pointer
3. Button fires immediately, which causes the screen switch
4. Sim.js screen change listener calls interruptSubtreeInput() on the home screen
5. The FireListener unattaches to the Pointer (because of the interruption)
6. The user is now on the new screen, with their finger down but unattached
7. A tiny touch-move (while the finger is still down) happens, touch-snagging the draggable object (since the pointer isn't attached).

The conceptual way of making this work generally would be to mark something on the Pointer saying "this pointer was interrupted, so don't allow it to touch-snag anything else"

[jbphet]

I just discussed this issue with @kathy-phet as part of the issue review prior to publishing number-line-operations. She agrees that these buttons should fire on touch release, the way our other buttons do, rather than on touch start, as they do now.

[jbphet]

Issue #624 is referenced in a comment in HomeScreenButton.js, and appears pretty related to this issue.

[jbphet]

I decided to pursue the avenue of making the buttons fire on touch release rather than trying to interrupt the input, since this seems like the way the buttons should behave anyway. I essentially removed the feature where the user can select different home screen buttons by sliding around, and it seems to solve the problem. I didn't commit this, but a patch is included below.

The next question is whether we should make this tradeoff or not. To summarize, we can make the home button behavior more consistent with other buttons and solve the problem of accidentally picking up items in the sim by removing the feature where the user can put their finger down on one home screen icon and then move to another without lifting up.

@samreid wasn't sure about the history of the slide-around-select feature, so I'm going to mark this for dev meeting and perhaps we can make a decision there, or forward it on to design meeting.

By the way, there might be a way to have our cake and eat it too, i.e. to have the slide-around-select behavior work in conjunction with fire-on-touch-release. I was disinclined to spend much time on this it's not particularly conventional behavior compared to, say, an iPad home screen, and since I think that keeping these buttons simple is the wisest choice. However, if we really feel that this feature is important, I or someone else could look into ways where we could continue to support it.

Index: js/HomeScreenButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/HomeScreenButton.js b/js/HomeScreenButton.js
--- a/js/HomeScreenButton.js	(revision 7d9981f35ad3e0f92afa90f304e5dea6173eac94)
+++ b/js/HomeScreenButton.js	(date 1612383965034)
@@ -167,11 +167,9 @@
     let buttonWasAlreadySelected = false;
 
     // If the button is already selected, then set the sim's screen to be its corresponding screen. Otherwise, make the
-    // button selected. The one exception to the former sentence is due to the desired behavior of selecting on
-    // touchover, in which case we need to guard on touchdown since we don't want to double fire for touchover and
-    // touchdown, see https://github.com/phetsims/joist/issues/624
-    const buttonDown = () => {
-      if ( isSelectedProperty.value && ( !( fireListener.pointer instanceof Touch ) || buttonWasAlreadySelected ) ) {
+    // button selected.
+    const buttonFired = () => {
+      if ( isSelectedProperty.value ) {
         homeScreenModel.screenProperty.value = screen;
       }
       else {
@@ -180,7 +178,7 @@
     };
 
     const fireListener = new FireListener( {
-      fire: buttonDown,
+      fire: buttonFired,
       tandem: options.tandem.createTandem( 'inputListener' )
     } );
     this.addInputListener( fireListener );
@@ -197,15 +195,6 @@
       out: () => isHighlightedProperty.set( false )
     } );
 
-    // If you touch an unselected button, it become selected. If then without lifting your finger you swipe over to the
-    // next button, that one becomes selected instead.
-    this.addInputListener( {
-      touchover: event => {
-        buttonWasAlreadySelected = homeScreenModel.selectedScreenProperty.value === screen;
-        homeScreenModel.selectedScreenProperty.value = screen;
-      }
-    } );
-
     // set the mouseArea and touchArea to be the whole local bounds of this node, because if it just relies on the
     // bounds of the icon and text, then there is a gap in between them. Since the button can change size, this
     // assignment needs to happen anytime the bounds change.

[KatieWoe]

This can be done in RaP: phetsims/qa#601.

[KatieWoe]

Additionally, as discussed with @jbphet over Zoom. On multiple sims published, if you click down on a home icon with a mouse, you enter the screen without letting go. This does not pick up items as it does in touch.

[chrisklus]

From 2/4/21 dev meeting:

Devs generally think that firing on touch up is the way to go and will simplify things.

We discussed improving HomeScreenButton by using some button component, i.e. button model, from Sun, but think that these buttons are too custom of behavior to do so.

JB: How about I try the patch above and make a dev version to try it out on a touch screen. In the process, I'll try out integrating more standard listeners like PressListener more. I will attempt to keep the behavior of touch sliding around on the home screen buttons in this change. UPDATE: This was cancelled per the discussion below:

SR + more devs - why don't we use normal sun buttons, not two-touch buttons, on the Home Screen? Single-click buttons feel like a better user experience. Devs are on board with this and we would not like to re-discuss with everyone and Kathy at the next dev meeting, but instead @jbphet will schedule time with @kathy-phet.

@jessegreenberg will revert the touch-on-down change so it's back in the original state, and then we will proceed with design time for reevaluating the home screen.

[samreid]

SR + more devs - why don't we use normal sun buttons, not two-touch buttons, on the Home Screen?

To elaborate on this, @pixelzoom and @jbphet said this has been a confusing user experience when they share PhET sim home screens with others.

[samreid]

From developer meeting:

@jbphet: Since @kathy-phet determined that we will not investigate a larger-scale change in #690, I'll work on addressing this by having the buttons fire on button up. This is a small-scale enough change that it won't warrant accessibility changes, or require interviews. I'll try to keep the functionality that you can mouse/touch over icons to slide around and make them larger, so we don't change more than is necessary. If I run into problems with the "slide around to make them larger" then I may publish a version that doesn't have that behavior to see if it's OK.

@zepumph volunteered to review when ready.

[jonathanolson]

I did more review, and the Scenery change should be safe to merge to master.

I haven't identified why tiny differences are showing up, but I'm not concerned about it. The following temporary change aqua-tested fine, showing no OTHER use of these parameters in that space:

diff --git a/js/listeners/PressListener.js b/js/listeners/PressListener.js
index 810bcd60..b2e5ebe2 100644
--- a/js/listeners/PressListener.js
+++ b/js/listeners/PressListener.js
@@ -643,11 +643,35 @@ class PressListener {
     // Notify after the rest of release is called in order to prevent it from triggering interrupt().
     this._releaseListener( event, this );
 
+    const pointer = this.pointer;
+    const pressedTrail = this.pressedTrail;
+    Object.defineProperty( this, 'pointer', {
+      get: () => {
+        throw new Error();
+      }
+    } );
+    Object.defineProperty( this, 'pressedTrail', {
+      get: () => {
+        throw new Error();
+      }
+    } );
+
     callback && callback();
 
+    pointer.cursor = null;
+
+    Object.defineProperty( this, 'pointer', {
+      value: null,
+      writable: true
+    } );
+    Object.defineProperty( this, 'pressedTrail', {
+      value: null,
+      writable: true
+    } );
+
     // These properties are cleared now, at the end of the onRelease, in case they were needed by the callback or in
     // listeners on the pressed Property.
-    this.pointer.cursor = null;
+
     this.pointer = null;
     this.pressedTrail = null;
   }

so I'm going to merge in the Scenery change.

[jonathanolson]

Ok, the related single-line change on fireHomeButtonsOnUp should be good to go, but I'm going to pass things back to you for that branch in general. Let me know if you need anything else.

[jbphet]

@zepumph and @chrisklus - You two are listed as the authors for HomeScreenButton.js, so one of you should probably review the changes. You can sort out which one between you.

Please assign this issue back to me when you're done regardless of the outcome. If no changes are necessary based on your review, I plan to work with QA to decide if we should do any specific testing for this change.

[zepumph]

Looks really nice! Thanks so much for taking this on. I like the change in scenery, and especially like that @jonathanolson signed off on it!

[jbphet]

@zepumph - Thanks for the review.

@KatieWoe - I've given this some thought, and I think it would be wise to have QA verify that the changes to the home screen button behavior work correctly on all our supported platforms with a particular emphasis on touch-based devices. This change is fairly subtle, but it affects every multi-screen sim, so it seems prudent to make sure there aren't any unanticipated consequences. I'll assign this to you to do or to delegate the testing. Basically, I think verifying it on a small number of 2, 3, and 4 screen sims using the various devices at QA's disposal should suffice, with an eye out for anything unexpected that would negatively impact our users.

[KatieWoe]

Testing using Number Line Operations and Energy Forms and Changes:

• Win 10 Chrome
• Win 10 Firefox
• MacOS 11 Chrome
• MacOS 11 Safari
• ChromeOS Chrome
• iPadOS Safari

[KatieWoe]

Looks good. QA is done

[jbphet]

Awesome. Thanks @KatieWoe. Closing.