Focus sometimes missing when moving an object with keyboard nav

[Nancy-Salpepi]

Test device
MacBook Air M1 chip

Operating System
14.5

Browser
Safari 17.5

Problem description
For phetsims/qa#1121, on the Distribute and Balance Point screens, the focus highlights for keyboard nav will fail to appear after moving a chocolate bar or a soccer ball with alt input, then moving the same/different bar/ball with the mouse and then switching back to keyboard navigation.

Steps to reproduce

1. On the Distribute screen, add the max amount of people
2. Tab to and grab one of the chocolate bars on the notepad and move it to another pile
3. While that bar still has keyboard focus, use the mouse to grab the same bar or a different one and move it to another pile
4. Press the HOME,END or arrow keys-- they will move the original bar.
5. Follow similar steps on the Balance Point screen.

Visuals

focusChocolate.mp4

[Nancy-Salpepi]

This behavior doesn't always happen on the Balance Point screen and I can't quite figure out what I may be doing differently.

[marlitas]

I'm going to take a stab at this. @jessegreenberg I'll let you know if I'll need your help.

[marlitas]

I think this is now being caught by an assertion that was turned on after some work I did yesterday in phetsims/soccer-common#7. Great news, because we have a clear starting point to search from.

[marlitas]

@jessegreenberg and I worked on some behavior questions for groupSortInteraction today that ended up solving this issue. However I did notice some inconsistencies in where the sim was focusing after a mouse even.

JG and I committed code that would reset keyboard state and fire a blur event whenever we received a mouse down event. Our understanding is that upon the user hitting tab the next item in the pdom would be focused (in the case of group sort, the first number spinner). However, I noticed that sometimes tab would lead me to the number spinner, and sometimes it would focus on the group again.

@jessegreenberg would love your thoughts on how buggy this feels to you? It seems strange but I'm not sure how broken it feels.

Screen.Recording.2024-07-31.at.3.57.18.PM.mov

[jessegreenberg]

I took a brief look. To consistently reproduce:

• If you click on a chocolate bar before it is grabbed by keyboard, focus will return to it when you press tab.
• If you click on a chocolate bar while it is grabbed by keyboard, focus will move to the spinner when you press tab.

I confirmed that in both cases, we successfully call blur from phetsims/scenery-phet@5fa5a31. Because of that, this should not be an issue and the behavior should be correct.

This may be happening because of a DOM state change triggered by GroupSortInteractionView, if something causes the PDOM elements to re-render (like changing the accessibleName) the browser might behave differently after the blur. Or it may be happening because of phetsims/joist#750 (comment). I didn't investigate further and recommend continuing without fixing.

Back to you @marlitas.

[marlitas]

Thanks @jessegreenberg. This is ready to cherry pick!

[Nancy-Salpepi]

The original issue is fixed so I am going to close this one.
A new issue related to this can be found at #343

[Nancy-Salpepi]

Sorry but I need to reopen.
Here is one way to get it to happen reliably:

1. Turn on interactive highlights
2. With the pointer, grab the highlighted bar and hold it in the air
3. While the bar is still grabbed, tab to and grab the next available bar (bar on top of left stack)
4. With alt input move that second bar to the stack on the right but do not release
5. Move the first bar with the pointer outside of the outer highlight
6. place the bar on the stack above the one that was grabbed using alt input
7. press the arrow keys

stillbroken3.mp4

[marlitas]

@jbphet and I were surprised to find that a keyboard event was not cancelling a pointer. We noticed that you can do also move the same element with both keyboard and mouse at the same time as shown in the published version of Faraday's Electromagnetic Lab. We are curious if this is expected behavior that individual sims need to handle.

faradays-keyboard.mov

[jessegreenberg]

Discussed with @marlitas. Indeed, this is a problem for several sims but we have generally been deferring issues related to using mouse and keyboard simultaneously. @marlitas described that this can create serious problems for GrabDragInteraction and this sim, so it would be ideal if it can be fixed.

I will take a look at GroupSortInteractionView to see if there is a way we can interrupt the pointer-related drag listeners when keys are pressed. I will also run the problem by @jonathanolson to see if he has any thoughts about fixing this more deeply in scenery (I was nervous about generally interrupting listeners whenever a key or tab is pressed, that could prevent mouse+keyboard behavior we might want in the future).

@marlitas will also check with design leads about whether this needs to be fixed before publishing.

We will check in again on Monday.

[jessegreenberg]

OK, I have a couple of ideas. I noticed that in the unbuilt version, an assertion is thrown before the bug in #329 (comment) which is helpful.

@marlitas @jbphet can you please review?

1. You may be able to put model.resetInteractionState(); at the top of the focus listener in GroupSortInteractionView. That makes the assertion failure go away. This is a simple change. But I don't know enough about GroupSortInteraction to say whether it is working correctly.

2. We can give GroupSortInteractionModel emitters to reset keyboard and mouse listener when necessary. GroupSortInteractionView can handle the interruption for keyboard input. But it will be up to clients to implement interruption for mouse/touch. This is a similar idea to setMouseSortedGroupItem. Here is a patch with this. Tested lightly but will need some further review and cleanup for disposal and such.

Subject: [PATCH] Interrupt mouse/keyboard when one form of input starts
---
Index: mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts b/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts
--- a/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts	(revision 431ad8b359b09fd8c719b6c77c6f2f8d68ea352e)
+++ b/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts	(date 1724080946791)
@@ -116,6 +116,11 @@
       tandem: options.tandem.createTandem( 'dragListener' )
     } );
 
+    // When the mouse input is interrupted, stop dragging the candy bar.
+    groupSortInteractionModel.interruptMouseInputEmitter.addListener( () => {
+      this.dragListener.interrupt();
+    } );
+
     this.addInputListener( this.dragListener );
 
     this.candyBar.positionProperty.link( position =>
Index: scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts b/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts
--- a/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts	(revision 8124a3a8728a7647543e585b2b9636f085074b62)
+++ b/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts	(date 1724081963236)
@@ -65,6 +65,7 @@
 import IOType from '../../../../../tandem/js/types/IOType.js';
 import ReferenceIO from '../../../../../tandem/js/types/ReferenceIO.js';
 import NullableIO from '../../../../../tandem/js/types/NullableIO.js';
+import Emitter from '../../../../../axon/js/Emitter.js';
 
 type SelfOptions<ItemModel> = {
 
@@ -124,6 +125,12 @@
   // input (not from keyboard/group sort input), but is important for cue showing.
   public readonly hasMouseSortedGroupItemProperty = new BooleanProperty( false );
 
+  // When a new input method starts, you may need to interrupt the other. GroupSortInteractionView handles interruption
+  // for keyboard input. You will need to add a listener to the interruptMouseInputEmitter to interrupt your
+  // implementation of mouse/touch input.
+  public readonly interruptKeyboardInputEmitter = new Emitter();
+  public readonly interruptMouseInputEmitter = new Emitter();
+
   // Whether any group item has yet been sorted to a new value, even if not by the "group sort" interaction. This
   // Property should be used to control the mouseSortCueVisibleProperty. The mouse sort cue does not need to be shown
   // if a keyboard sort has occurred (because now the user knows that the group items are sortable).
@@ -276,6 +283,9 @@
     this.hasKeyboardSelectedGroupItemProperty.dispose();
     this.mouseSortCueVisibleProperty.dispose();
     this.hasGroupItemBeenSortedProperty.dispose();
+
+    this.interruptKeyboardInputEmitter.dispose();
+    this.interruptMouseInputEmitter.dispose();
     super.dispose();
   }
 }
Index: scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts b/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts
--- a/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts	(revision 8124a3a8728a7647543e585b2b9636f085074b62)
+++ b/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts	(date 1724080687636)
@@ -198,6 +198,8 @@
     const focusListener = {
       focus: () => {
 
+        model.interruptMouseInputEmitter.emit();
+
         // It's possible that getGroupItemToSelect's heuristic said that there is nothing to focus here
         if ( selectedGroupItemProperty.value === null ) {
           selectedGroupItemProperty.value = options.getGroupItemToSelect();
@@ -223,6 +225,8 @@
       },
       down: () => {
 
+        model.interruptKeyboardInputEmitter.emit();
+
         // When the group is clicked, reset the interaction state to stop keyboard input logic. We only support
         // one mode of input at a time.
         model.resetInteractionState();
@@ -358,6 +362,15 @@
       }
     } );
 
+    const interruptListener = () => {
+      deltaKeyboardListener.interrupt();
+      grabReleaseKeyboardListener.interrupt();
+    };
+    model.interruptKeyboardInputEmitter.addListener( interruptListener );
+    model.disposeEmitter.addListener( () => {
+      model.interruptKeyboardInputEmitter.removeListener( interruptListener );
+    } )
+
     if ( options.numberKeyMapper ) {
       const numbersKeyboardListener = new KeyboardListener( {
         fireOnHold: true,
@@ -380,8 +393,15 @@
         }
       } );
       primaryFocusedNode.addInputListener( numbersKeyboardListener );
+
+      const interruptNumbersListener = () => {
+        numbersKeyboardListener.interrupt();
+      };
+      model.interruptKeyboardInputEmitter.addListener( interruptNumbersListener );
+
       this.disposeEmitter.addListener( () => {
         primaryFocusedNode.removeInputListener( numbersKeyboardListener );
+        model.interruptKeyboardInputEmitter.removeListener( interruptNumbersListener );
         numbersKeyboardListener.dispose();
       } );
     }

[marlitas]

@jessegreenberg thank you for the investigation and work here. I think it will be valuable to determine wether this is something GroupSortInteraction should handle in general and I added it as a discussion point for dev meeting. The team has decided that this is not blocking for MSaB and we will proceed by ignoring any dual mouse and keyboard bugs with QA looking out for the following:

• Bugs that crash the sim or cause a frozen state.
• Bugs that do not resolve themselves without a reload of the sim.
• Bugs that do not resolve themselves without a reset of the sim.

QA, if any of the above occur please flag that as it is much more critical and may need to be addressed.

Thanks!

[Nancy-Salpepi]

I'm not seeing any of the scenarios in #329 (comment).
Closing.