Grab drag cue bug

[zepumph]

While working on phetsims/scenery-phet#869.

To reproduce:

1. Buoyancy first screen
2. tab to first block, press space, move it with arrows.
3. tab to second block while still having the first block grabbed
4. The bug is that the second block will show the grab cue for that first highlight, but tab + shift-tab back to the second block will correctly hide it.

[zepumph]

Using this patch, along with ?sceneryLog=Input, I am able to confirm that when you tab from a grabbed block to another block, that new block doesn't get the focus event. I believe that is because the whole PDOM is redrawn. When I cancel out the swapping of tag names such that we don't need to redraw, it fixes the problem.

@jessegreenberg, I would like to discuss this with you to see if we should get creative in GrabDragInteraction, or try to better understand focus in PDOM peer here. This bug would apply to any spot in phet code when a tab triggers PDOM content to change and the PDOM to redraw. Have we encountered this before?

Subject: [PATCH] use listener+default options instead of custom option, https://github.com/phetsims/scenery-phet/issues/869
---
Index: js/accessibility/grab-drag/GrabDragInteraction.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/grab-drag/GrabDragInteraction.ts b/js/accessibility/grab-drag/GrabDragInteraction.ts
--- a/js/accessibility/grab-drag/GrabDragInteraction.ts	(revision 73421a747332de12595886220d333d9b8cc5c5c8)
+++ b/js/accessibility/grab-drag/GrabDragInteraction.ts	(date 1725575595955)
@@ -412,12 +412,15 @@
       },
 
       focus: () => {
+        console.log( 'focus idle' );
         this.updateVisibilityForCues();
 
         this.onGrabButtonFocusEmitter.emit();
       },
 
-      blur: () => this.updateVisibilityForCues()
+      blur: () => {
+        console.log( 'blur idle' );
+      }
     };
 
     // Keep track of all listeners to swap out grab/drag functionalities.
@@ -450,10 +453,16 @@
       },
 
       // release when focus is lost
-      blur: () => this.grabDragModel.release(),
+      blur: () => {
+        console.log( 'blur grabbed' );
+        this.grabDragModel.release();
+      },
 
       // if successfully dragged, then make the cue node invisible
-      focus: () => this.updateVisibilityForCues()
+      focus: () => {
+        console.log( 'focus grabbed' );
+        this.updateVisibilityForCues();
+      }
     } );
 
     // Update the visibility of dragging cues whenever keyboard dragging keys release (keyup), bug fix for https://github.com/phetsims/scenery-phet/issues/868
@@ -580,7 +589,12 @@
 
     // update the PDOM of the node
     const nodeOptions = interactionState === 'idle' ? this.idleStateOptions : this.grabbedStateOptions;
-    this.node.mutate( nodeOptions );
+
+    this.node.mutate( {
+      ...nodeOptions,
+      tagName: 'button',
+      containerTagName: 'div'
+    } );
     assert && this.grabDragModel.enabledProperty.value && assert( this.node.focusable, 'GrabDragInteraction node must remain focusable after mutation' );
 
     const listenersToAdd = interactionState === 'idle' ? this.listenersWhileIdle : this.listenersWhileGrabbed;

[jessegreenberg]

Great find on the re-rendering, I think that is correct. The focus event is being blocked by BrowserEvents.blockFocusCallbacks:

https://github.com/phetsims/scenery/blob/f2c577900fdc87799e206b75137301dedd5ebd6e/js/input/BrowserEvents.js#L343-L345

[jessegreenberg]

@zepumph worked to track down what is going on. The problem is that this line prevents focus from being restored correctly after the PDOM update:

https://github.com/phetsims/scenery/blob/92e8bc694d4c1d91c36d519ccf517a84f46ff6c0/js/accessibility/FocusManager.ts#L241-L243

This change was added in phetsims/scenery#1296 (though it now lives in FocusManager).

Basically the PDOMPointer is set up to null out the FocusManager.pdomFocus in between each blur/focus pair. So when tabbing from (a) to (b), the FocusManager acts like it went from (a) to null to (b).

The original issue was created to support PhET-iO playback. But it continues to describe cases of changing the PDOM/moving focus that should be supported. Id like to try removing the workaround and making sure that all these cases still work.

[jessegreenberg]

@zepumph and I paired on this again. We identified that it was buggy for PDOMTree to manipulate focus while scenery is actively responding to focus and blur events. We added a flag that is true while focus/blur events are being dispatched and prevents PDOMTree from changing focus when this is true.

I tested the following with this change

• GrabDragInteraction in BASE
• GrabDragInteraction in Friction
• ComboBox in my-solar-system (known to be a case of PDOM re-render + focus change)
• Dialog, PhetMenu
• greenhouse-effect
• ?fuzzBoard tests with aqua

We are worried this problem will happen for other events too. For example, if a keydown event on a button re-renders it, will it still receive a click event? Ill test that next and report what I find here.

[jessegreenberg]

To test the keydown/click issue, I added this to ResetAllButton and I was happy to see that the click event was coming through nicely. Tested in Chrome and FF.

    this.addInputListener( {
      keydown: () => {
        this.innerContent = null; // to avoid assertion
        
        this.tagName = 'input';
        this.inputType = 'button';
        this.accessibleName = Math.random() + '';
        this.tagName = 'button';
      }
    } );

I added logging to make sure the PDOM is being re-rendered, and I verified that the element for the ResetAllButton was different before/after pdomContentChange.

Full patch:

Subject: [PATCH] Interrupt mouse/keyboard when one form of input starts
---
Index: scenery-phet/js/buttons/ResetAllButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/buttons/ResetAllButton.ts b/scenery-phet/js/buttons/ResetAllButton.ts
--- a/scenery-phet/js/buttons/ResetAllButton.ts	(revision 73421a747332de12595886220d333d9b8cc5c5c8)
+++ b/scenery-phet/js/buttons/ResetAllButton.ts	(date 1725661192584)
@@ -122,6 +122,17 @@
       } );
     } );
 
+    this.addInputListener( {
+      keydown: () => {
+        this.innerContent = null; // to avoid assertion
+
+        this.tagName = 'input';
+        this.inputType = 'button';
+        this.accessibleName = Math.random() + '';
+        this.tagName = 'button';
+      }
+    } );
+
     const keyboardListener = KeyboardListener.createGlobal( this, {
       keyStringProperties: ResetAllButton.RESET_ALL_HOTKEY_DATA.keyStringProperties,
       fire: () => this.pdomClick(),
Index: scenery/js/accessibility/pdom/PDOMTree.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/pdom/PDOMTree.js b/scenery/js/accessibility/pdom/PDOMTree.js
--- a/scenery/js/accessibility/pdom/PDOMTree.js	(revision 972264781c72d5f4f20ba596bedf7fe7847dbce8)
+++ b/scenery/js/accessibility/pdom/PDOMTree.js	(date 1725661347450)
@@ -197,6 +197,8 @@
 
     // For now, just regenerate the full tree. Could optimize in the future, if we can swap the content for an
     // PDOMInstance.
+
+    const elementBefore = focusedNode?._pdomInstances[ 0 ].peer.primarySibling;
     for ( i = 0; i < parents.length; i++ ) {
       const parent = parents[ i ];
 
@@ -204,17 +206,23 @@
       pdomTrailsList.push( pdomTrails );
 
       PDOMTree.removeTree( parent, node, pdomTrails );
+      console.log( 'removing tree' );
     }
 
     // Do all removals before adding anything back in.
     for ( i = 0; i < parents.length; i++ ) {
+      console.log( 'rebuilding tree' );
       PDOMTree.addTree( parents[ i ], node, pdomTrailsList[ i ] );
     }
+    const elementAfter = focusedNode?._pdomInstances[ 0 ].peer.primarySibling;
+
+    console.log( 'elements are teh same:', elementBefore === elementAfter );
 
     // An edge case is where we change the rootNode of the display (and don't have an effective parent)
     for ( i = 0; i < node._rootedDisplays.length; i++ ) {
       const display = node._rootedDisplays[ i ];
       if ( display._accessible ) {
+        console.log( 'rebuilding tree' );
         PDOMTree.rebuildInstanceTree( display._rootPDOMInstance );
       }
     }

This is good news!

I am sure there are other cases to worry about but this helps me rest easier.

I tried adding a unit test for this but could not think of a way to simulate a keydown event that sends both a keydown and click event at the same time. The result was dispatching a click event after the re-render, which should certainly work.

@zepumph reassigning this back to you. What else do you think should be done/tested for this issue?

[zepumph]

Excellent patch! I expanded upon it to get a notion of what I was wondering about. Basically that long and the short of it is that we need to be SOOOOO CAREFUL when updating PDOM content during input listeners, because it can totally change the way the browser dispatches the next event. I wrote notes directly in the patch, but let me know if you want to discuss more synchronously. Should we try to add this into documentation somehow? Not sure how to do so in a way that we will actually remember or care in the future, but it seems like a scary gotcha.

Subject: [PATCH] Interrupt mouse/keyboard when one form of input starts
---
Index: scenery-phet/js/buttons/ResetAllButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/buttons/ResetAllButton.ts b/scenery-phet/js/buttons/ResetAllButton.ts
--- a/scenery-phet/js/buttons/ResetAllButton.ts	(revision 73421a747332de12595886220d333d9b8cc5c5c8)
+++ b/scenery-phet/js/buttons/ResetAllButton.ts	(date 1725752433275)
@@ -122,6 +122,73 @@
       } );
     } );
 
+    /**
+     * Case 1: Turn from button to input type=button. You get the click event enter, but not from space, since it changed
+     * PDOM state before keyup, which is likely when the click would be triggered by the browser.
+     */
+    const case1 = () => {
+      this.tagName = 'input';
+      this.inputType = 'button';
+    };
+
+    /**
+     * Case 2: Turn from button to focusable div. You do not get a click event for enter or space (because divs don't
+     * get clicks I guess?)
+     */
+    const case2 = () => {
+      this.focusable = true;
+      this.tagName = 'div';
+    };
+
+
+    /**
+     * Case 3: Turn from button to focusable div and then to input type=button. You get a click event for enter but
+     * still not for space. So the click will come if by the end of the keydown you are still button like (but not
+     * fully, see case4 below about ariaRole).
+     *
+     * NOTE: In this case the PDOMTree logging shows that the elements aren't the same, but we still can get a click event.
+     */
+    const case3 = () => {
+      this.focusable = true;
+      this.tagName = 'div';
+      this.tagName = 'input';
+      this.inputType = 'button';
+    };
+
+
+    /**
+     * Case 4: See if ariaRole=button will send the click event. It does not (what?!?!) Rude.
+     */
+    const case4 = () => {
+      this.focusable = true;
+      this.tagName = 'div';
+      this.ariaRole = 'button';
+    };
+
+    this.addInputListener( {
+      keydown: () => {
+        this.innerContent = null; // to avoid assertion
+
+        this.ariaLabel = 'hi';
+        console.log( '\n\nbegin keydown' );
+
+        case1();
+        // case2();
+        // case3();
+        // case4();
+
+        console.log( 'end keydown' );
+      },
+      click: () => {
+        console.log( 'click' );
+      },
+      keyup: () => {
+        console.log( 'begin keyup' );
+        this.tagName = 'button';
+        console.log( 'end keyup' );
+      }
+    } );
+
     const keyboardListener = KeyboardListener.createGlobal( this, {
       keyStringProperties: ResetAllButton.RESET_ALL_HOTKEY_DATA.keyStringProperties,
       fire: () => this.pdomClick(),
Index: scenery/js/accessibility/pdom/PDOMTree.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/pdom/PDOMTree.js b/scenery/js/accessibility/pdom/PDOMTree.js
--- a/scenery/js/accessibility/pdom/PDOMTree.js	(revision b3926dad8b7ffe947acb9ed041807a1facb18e35)
+++ b/scenery/js/accessibility/pdom/PDOMTree.js	(date 1725751235250)
@@ -197,6 +197,8 @@
 
     // For now, just regenerate the full tree. Could optimize in the future, if we can swap the content for an
     // PDOMInstance.
+
+    const elementBefore = focusedNode?._pdomInstances[ 0 ].peer.primarySibling;
     for ( i = 0; i < parents.length; i++ ) {
       const parent = parents[ i ];
 
@@ -204,17 +206,23 @@
       pdomTrailsList.push( pdomTrails );
 
       PDOMTree.removeTree( parent, node, pdomTrails );
+      console.log( 'removing tree' );
     }
 
     // Do all removals before adding anything back in.
     for ( i = 0; i < parents.length; i++ ) {
+      console.log( 'rebuilding tree' );
       PDOMTree.addTree( parents[ i ], node, pdomTrailsList[ i ] );
     }
+    const elementAfter = focusedNode?._pdomInstances[ 0 ].peer.primarySibling;
+
+    console.log( 'elements are teh same:', elementBefore === elementAfter );
 
     // An edge case is where we change the rootNode of the display (and don't have an effective parent)
     for ( i = 0; i < node._rootedDisplays.length; i++ ) {
       const display = node._rootedDisplays[ i ];
       if ( display._accessible ) {
+        console.log( 'rebuilding tree' );
         PDOMTree.rebuildInstanceTree( display._rootPDOMInstance );
       }
     }

[zepumph]

Doc added. Closing