KatieWoe
commented
3 years ago Doesn't seem to happen in Safari
Open KatieWoe opened 3 years ago
KatieWoe
commented
3 years ago Doesn't seem to happen in Safari
jbphet
commented
3 years ago 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)A couple of other notes:
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
commented
3 years ago 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
commented
3 years ago Here is the logging that shows what @jbphet suspected:
User interaction:
[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 balloonNodeApply this patch for the logging:
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
commented
3 years ago @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: () => {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
commented
3 years ago I understand that sentiment, and agree with it in theory. I think though that if @jessegreenberg signs off on the patch above (in https://github.com/phetsims/balloons-and-static-electricity/issues/553#issuecomment-952088845), 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
commented
3 years ago 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
commented
3 years ago 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
commented
3 years ago I made https://github.com/phetsims/scenery-phet/issues/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
commented
2 years ago @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
commented
2 years ago Indeed, reducing priority since this was published with this behavior.
Test device Dell Operating System Win 11 Browser Firefox (Not the same on Chrome) Problem description For https://github.com/phetsims/qa/issues/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
Visuals
Troubleshooting information: