Firefox: Balloon behaves oddly if dragged with mouse after being dragged with keyboard nav

[KatieWoe]

Test device
Dell
Operating System
Win 11
Browser
Firefox (Not the same on Chrome)
Problem description
For phetsims/qa#721
If you move the balloon around with keyboard nav (and give it charge) and then drop it, then pick it up with mouse, the balloon acts oddly. It seems to think it has been dropped, picked up, and dropped again continually. Dropping it and picking it up again seems to resolve this.

Steps to reproduce

1. Use keyboard to pick up the balloon and give it a charge
2. Drop the balloon with keyboard nav (bug will happen even without this step)
3. Pick up the balloon with mouse interaction and drag the balloon around

Visuals

Troubleshooting information:

!!!!! DO NOT EDIT !!!!!
Name: ‪Balloons and Static Electricity‬
URL: https://phet-dev.colorado.edu/html/balloons-and-static-electricity/1.5.0-rc.2/phet/balloons-and-static-electricity_all_phet.html
Version: 1.5.0-rc.2 2021-10-11 21:55:48 UTC
Features missing: touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:93.0) Gecko/20100101 Firefox/93.0
Language: en-US
Window: 1280x667
Pixel Ratio: 1.5/1
WebGL: WebGL 1.0
GLSL: WebGL GLSL ES 1.0
Vendor: Mozilla (ANGLE (Intel(R) HD Graphics Direct3D11 vs_5_0 ps_5_0))
Vertex: attribs: 16 varying: 30 uniform: 4096
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

[KatieWoe]

Doesn't seem to happen in Safari

[jbphet]

I've done some investigation on this (it's easy to duplicate on master at the moment), and I was able to determine that when the described sequence is done on Firefox, there is a call to the onRelease method in BalloonNode's instance of GrabDragInteraction immediately after the call to onGrab and before the pointer has been released (i.e. the mouse button is still down). This doesn't happen on Chrome. I set a breakpoint and looked at the call stack in Firefox, and the onRelease is being called as a result of something related to "blur". This is getting into territory that I don't know well, so I'm going to have to turn this over to @zepumph, who is the listed author of GrabDragInteraction, and @jessegreenberg, who has been working on the a11y-related features for this sim.

In the hope that it could help, here is the stack trace from Firefox where the onRelease function is being called right after clicking on the balloon, which does not seem to happen in Chrome.

onRelease (http://localhost:8080/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonNode.js#403)
onRelease (http://localhost:8080/scenery-phet/js/accessibility/GrabDragInteraction.js#348)
releaseDraggable (http://localhost:8080/scenery-phet/js/accessibility/GrabDragInteraction.js#590)
blur (http://localhost:8080/scenery-phet/js/accessibility/GrabDragInteraction.js#493)
dispatchToListeners (http://localhost:8080/scenery/js/input/Input.js#1931)
dispatchToTargets (http://localhost:8080/scenery/js/input/Input.js#1970)
dispatchEvent (http://localhost:8080/scenery/js/input/Input.js#1884)
dispatchPDOMEvent (http://localhost:8080/scenery/js/input/Input.js#1071)
constructor (http://localhost:8080/scenery/js/input/Input.js#637)
execute (http://localhost:8080/axon/js/Action.js#227)
constructor (http://localhost:8080/scenery/js/input/Input.js#781)
connectListeners (http://localhost:8080/scenery/js/input/Input.js#923)
connectListeners (http://localhost:8080/scenery/js/input/Input.js#922)
initializeEvents (http://localhost:8080/scenery/js/display/Display.js#1344)
SimDisplay (http://localhost:8080/joist/js/SimDisplay.js#140)
Sim (http://localhost:8080/joist/js/Sim.js#547)
<anonymous> (http://localhost:8080/balloons-and-static-electricity/js/balloons-and-static-electricity-main.js#48)
launchSimulation (http://localhost:8080/joist/js/simLauncher.js#62)
launch (http://localhost:8080/joist/js/simLauncher.js#83)
proceedIfReady (http://localhost:8080/phet-core/js/asyncLoader.js#45)
proceedIfReady (http://localhost:8080/phet-core/js/asyncLoader.js#45)
createLock (http://localhost:8080/phet-core/js/asyncLoader.js#62)
<anonymous> (http://localhost:8080/brand/phet/images/logo_png.js#6)

[jbphet]

A couple of other notes:

• This problem exists in the published version (v1.4.16), but it doesn't appear to be quite as bad, because the balloon doesn't jump around as much when dragging after the incorrect release occurs. I don't know what it's like this, and will investigate if there isn't a good quick fix.
• I think this is a general issue with Firefox and GrabDragInteraction, but it doesn't come up much because most sims don't have anything that spontaneously happens when an item is released. BASE is a little unique in this regard, because a charged balloon will drift towards the sweater once it is released.

[zepumph]

Thanks for the investigation @jbphet, I am able to to reproduce that. Note that the exact bug seems to be because that is then triggering the endDragListener, which is changing the Balloon state (obviously).

I'll keep looking at what events we are getting in Firefox that we didn't expect (likely because of development and testing in Chrome)

[zepumph]

Here is the logging that shows what @jbphet suspected:

User interaction:

• tab to the balloon before this event log
• Make sure the mouse is already on top of the balloon before this event log
• press space
• mouse down
• alt+tab away without dragging so that you can get a log without the mouse release (which just fires onRelease again, but doesn't change the grab/drag state since its already in grabbable.

[Input] keydown(69891 keydown);
[Input] keyup(69954 keyup);
[Input] click(69973 click);
	[Input] focusOut(69975 focusout);
Turning to Draggable
	[Input] focusin(69979 focusin);
[Input] mouseDown('null', 742,518 71063 mousedown);
	[Input] downEvent Mouse changed:false
Turning to Draggable
[Input] focusOut(71074 focusout);
Turning to Grabbable
	endDragListener in balloonNode

Apply this patch for the logging:

Index: scenery-phet/js/accessibility/GrabDragInteraction.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/accessibility/GrabDragInteraction.js b/scenery-phet/js/accessibility/GrabDragInteraction.js
--- a/scenery-phet/js/accessibility/GrabDragInteraction.js	(revision f39c2f7bf6f3df23021ca8978ab33a48cd5cdbbf)
+++ b/scenery-phet/js/accessibility/GrabDragInteraction.js	(date 1635264169418)
@@ -595,6 +595,7 @@
    * @private
    */
   turnToGrabbable() {
+    phet.log && phet.log( 'Turning to Grabbable' );
     this.grabbable = true;
 
     // To support gesture and mobile screen readers, we change the roledescription, see https://github.com/phetsims/scenery-phet/issues/536
@@ -632,6 +633,7 @@
    * @private
    */
   turnToDraggable() {
+    phet.log && phet.log( 'Turning to Draggable' );
     this.numberOfGrabs++;
 
     this.grabbable = false;
Index: balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonNode.js b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonNode.js
--- a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonNode.js	(revision 0974e75a34a802a74a921496fe508d215abb4eb2)
+++ b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonNode.js	(date 1635263273247)
@@ -128,6 +128,7 @@
 
     // Finish a drag interaction by updating the Property tracking that the balloon is dragged and resetting velocities.
     const endDragListener = () => {
+      console.log( 'endDragListener' );
       model.isDraggedProperty.set( false );
       model.velocityProperty.set( new Vector2( 0, 0 ) );
       model.dragVelocityProperty.set( new Vector2( 0, 0 ) );
Index: scenery/js/input/Input.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/input/Input.js b/scenery/js/input/Input.js
--- a/scenery/js/input/Input.js	(revision 89b51f05e4bb4c7988912ca2f6e21273a4c6a030)
+++ b/scenery/js/input/Input.js	(date 1635263621578)
@@ -1193,10 +1193,10 @@
    * @param {Event} event
    */
   mouseMove( point, event ) {
-    sceneryLog && sceneryLog.Input && sceneryLog.Input( `mouseMove(${Input.debugText( point, event )});` );
-    sceneryLog && sceneryLog.Input && sceneryLog.push();
+    // sceneryLog && sceneryLog.Input && sceneryLog.Input( `mouseMove(${Input.debugText( point, event )});` );
+    // sceneryLog && sceneryLog.Input && sceneryLog.push();
     this.mouseMoveAction.execute( point, event );
-    sceneryLog && sceneryLog.Input && sceneryLog.pop();
+    // sceneryLog && sceneryLog.Input && sceneryLog.pop();
   }
 
   /**

We get the mouse down before we get the blur from the draggable, so it is moused and then blurred/released. I am not sure exactly how to fix this, but presumably this effects all grab drag interactions, other sims may just not be putting such important logic in the release callback. I'll keep thinking about this.

[zepumph]

@jessegreenberg, this fixes the bug, but I wonder if it makes too many assumptions. Is the presslistener the only way the mouse interactions with the Grab/Drag type? Is it possible that it could get in a bad state from some multitouch problem?

Index: js/accessibility/GrabDragInteraction.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/GrabDragInteraction.js b/js/accessibility/GrabDragInteraction.js
--- a/js/accessibility/GrabDragInteraction.js	(revision f39c2f7bf6f3df23021ca8978ab33a48cd5cdbbf)
+++ b/js/accessibility/GrabDragInteraction.js	(date 1635264169418)
@@ -490,7 +490,12 @@
         // if successfully dragged, then make the cue node invisible
         this.updateVisibilityForCues();
       },
-      blur: () => this.releaseDraggable(),
+      blur: () => {
+
+        if ( !this.pressListener.isPressedProperty.value ) {
+          this.releaseDraggable();
+        }
+      },
 
       // TODO: this is not called right now, see https://github.com/phetsims/scenery-phet/issues/693
       focus: () => {

[jbphet]

The sound design team reviewed this issue in today's meeting and we decided that, since there is not a clear and quick fix, this should not be blocking for the 1.5 release of BASE. The use case is unlikely to be frequently encountered by our users, since it is specific to Firefox and involves switching between keyboard nav and mouse, and the recovery is fairly easy, i.e. let go of the balloon and pick it up again.

@emily-phet said she would talk with @jessegreenberg and @zepumph to determine whether this should be fixed soon on master or made part of a larger effort to improve the behavior when switching between input modalities.

[zepumph]

I understand that sentiment, and agree with it in theory. I think though that if @jessegreenberg signs off on the patch above (in #553 (comment)), and it can be tested by QA, then we are done. Is it worth at least that much effort? The fix itself, if effective, is very very simple, and would be an easy cherry pick.

[jessegreenberg]

Thanks for investigating this @zepumph.

Yes, the PressListener is the only way to access the GrabDragInteraction with mouse/touch. I can't think of anything related to multitouch that would be problematic here. I tried to think through how this might work with iOS VoiceOver but I couldn't think of any problems with this. I tested on iOS 15 with VoiceOver and verified that GrabDragInteraction still worked.

This was an improvement but there was another case this is still an issue, regardless of platform:

1. Charge the balloon (because it is easier to see the problem)
2. Pick up the charged balloon with keyboard.
3. (Without clicking anywhere else in the sim) click on the balloon with a mouse to pick it up, leave mouse pressed.
4. Press spacebar while the mouse is pressed. This will turn the balloon back into a grabbable, but the balloon's DragListener is still pressed for dragging.

I thought about moving the !this.pressListener.isPressedProperty.value to releaseDraggable. But we need to be able to release while actively dragging sometimes. Ideally, we would be able to interrupt the DragListener in this case but GrabDragInteraction doesn't have knowledge of the DragListener. Maybe that needs to change.

[jbphet]

Over Slack, @jessegreenberg also said:

If it was decided that publishing with this bug is OK, I would probably recommend proceeding without this fix. I think we should work on it a bit more before putting the change in RC SHAs.

Based on @jessegreenberg's review and recommendation, I'm going to move forward with publishing the next RC off of the 1.5 branch without this fix. If there is a breakthrough before publication, we can consider working this in.

[jessegreenberg]

I made phetsims/scenery-phet#710 to work on this generally since it isn't specific to BASE (though BASE probably has the most obvious case since the balloon can move on its own).

[jbphet]

@jessegreenberg - Since it sounds like this issue will be addressed in common code, and we've decided not to have the problem block the 1.5 release of BASE, can this issue just be closed, or at least reduced in priority?

[jessegreenberg]

Indeed, reducing priority since this was published with this behavior.