Tapping paragraph instead of button disables Mobile VO

[KatieWoe]

Test device
iPad
Operating System
iPadOS 17.4.1
Browser
Safari
Problem description
Found during phetsims/qa#1080.
Mobile VO may stop responding to button presses if you try to press a paragraph description rather than a button. This makes using the sim from that point on impossible
Steps to reproduce

1. If you go to the home screen, swipe right until you get to one of the buttons for a screen
2. Swipe right one more time to the description of the button
3. Try to press the button.
4. Focus disappears and further button presses aren't possible.
5. This impacts published sims such as RaP that are marked as supporting Mobile VO

Visuals

IMG_5261.MOV

[jessegreenberg]

Here is an error from @terracoda 's phone, tethered to her Mac:

More info:

And in the debug version:

[jessegreenberg]

Code in question is here:

      const target = domEvent.target;
      if ( target && target instanceof window.Element && this.display.isElementUnderPDOM( target ) ) {
        const trailIndices = target.getAttribute( PDOMUtils.DATA_PDOM_UNIQUE_ID );
        assert && assert( trailIndices, 'should not be null' );
        return PDOMInstance.uniqueIdToTrail( this.display, trailIndices! );
      }

This shows that every element in the PDOM has the attribute.

So I believe, that that the only way we can hit this is if the target is the root of the PDOM.

[jessegreenberg]

isElementUnderPDOM uses element.contains( otherElement ) which returns true if the element === otherElement. I believe that is the only way we could see this problem if all elements have attribute for PDOMUtils.DATA_PDOM_UNIQUE_ID. This patch makes isElementUnderPDOM more selective.

Subject: [PATCH] Update usages of KeyboardListener and KeyboardDragListener after changes from https://github.com/phetsims/scenery/issues/1570
---
Index: js/display/Display.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/display/Display.ts b/js/display/Display.ts
--- a/js/display/Display.ts	(revision a0c37f52fc88810fb6bcdce1f31df03ab09b6ad8)
+++ b/js/display/Display.ts	(date 1715199696664)
@@ -938,9 +938,20 @@
 
   /**
    * Returns true if the element is in the PDOM. That is only possible if the display is accessible.
+   * @param element
+   * @param allowRoot - If true, the root of the PDOM is also considered to be "under" the PDOM.
    */
-  public isElementUnderPDOM( element: Element ): boolean {
-    return this._accessible && this.pdomRootElement!.contains( element );
+  public isElementUnderPDOM( element: Element, allowRoot: boolean ): boolean {
+    if ( !this._accessible ) {
+      return false;
+    }
+
+    const isElementContained = this.pdomRootElement!.contains( element );
+    const isNotRootElement = element !== this.pdomRootElement;
+
+    // If allowRoot is true, just return if the element is contained.
+    // Otherwise, also ensure it's not the root element itself.
+    return allowRoot ? isElementContained : ( isElementContained && isNotRootElement );
   }
 
   /**
Index: js/accessibility/FocusManager.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/FocusManager.ts b/js/accessibility/FocusManager.ts
--- a/js/accessibility/FocusManager.ts	(revision a0c37f52fc88810fb6bcdce1f31df03ab09b6ad8)
+++ b/js/accessibility/FocusManager.ts	(date 1715199538536)
@@ -201,7 +201,7 @@
         const display = displays[ i ];
 
         const activeElement = document.activeElement as HTMLElement;
-        if ( display.isElementUnderPDOM( activeElement ) ) {
+        if ( display.isElementUnderPDOM( activeElement, false ) ) {
           const uniqueId = activeElement.getAttribute( PDOMUtils.DATA_PDOM_UNIQUE_ID )!;
           assert && assert( uniqueId, 'Event target must have a unique ID on its data if it is in the PDOM.' );
 
Index: js/input/Input.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/input/Input.ts b/js/input/Input.ts
--- a/js/input/Input.ts	(revision a0c37f52fc88810fb6bcdce1f31df03ab09b6ad8)
+++ b/js/input/Input.ts	(date 1715199538557)
@@ -1124,7 +1124,7 @@
   public getRelatedTargetTrail( domEvent: FocusEvent | MouseEvent ): Trail | null {
     const relatedTargetElement = domEvent.relatedTarget;
 
-    if ( relatedTargetElement && this.display.isElementUnderPDOM( relatedTargetElement as HTMLElement ) ) {
+    if ( relatedTargetElement && this.display.isElementUnderPDOM( relatedTargetElement as HTMLElement, false ) ) {
 
       const relatedTarget = ( domEvent.relatedTarget as unknown as Element );
       assert && assert( relatedTarget instanceof window.Element );
@@ -1154,7 +1154,7 @@
     }
     else {
       const target = domEvent.target;
-      if ( target && target instanceof window.Element && this.display.isElementUnderPDOM( target ) ) {
+      if ( target && target instanceof window.Element && this.display.isElementUnderPDOM( target, false ) ) {
         const trailIndices = target.getAttribute( PDOMUtils.DATA_PDOM_UNIQUE_ID );
         assert && assert( trailIndices, 'should not be null' );
         return PDOMInstance.uniqueIdToTrail( this.display, trailIndices! );
@@ -1607,7 +1607,7 @@
     // if the event target is within the PDOM the AT is sending a fake pointer event to the document - do not
     // dispatch this since the PDOM should only handle Input.PDOM_EVENT_TYPES, and all other pointer input should
     // go through the Display div. Otherwise, activation will be duplicated when we handle pointer and PDOM events
-    if ( this.display.isElementUnderPDOM( context.domEvent.target as HTMLElement ) ) {
+    if ( this.display.isElementUnderPDOM( context.domEvent.target as HTMLElement, true ) ) {
       return;
     }
 
@@ -1636,7 +1636,7 @@
     // if the event target is within the PDOM the AT is sending a fake pointer event to the document - do not
     // dispatch this since the PDOM should only handle Input.PDOM_EVENT_TYPES, and all other pointer input should
     // go through the Display div. Otherwise, activation will be duplicated when we handle pointer and PDOM events
-    if ( this.display.isElementUnderPDOM( context.domEvent.target as HTMLElement ) ) {
+    if ( this.display.isElementUnderPDOM( context.domEvent.target as HTMLElement, true ) ) {
       return;
     }
 

I made a build with it and asked @KatieWoe or @terracoda to confirm that this fixes it. If it does Ill commit the change.

[jessegreenberg]

@KatieWoe is there anything else you would like to test for this?

[KatieWoe]

Not at the moment, but if it get's a maintenance release it should probably get some more focus for those sims before going out.

[jessegreenberg]

Ah, right. Thanks for the reminder, lets consider a maintenance release. Since so few sims support iOS VoiceOver, it may not be too time consuming.

[jessegreenberg]

@marlitas let me know that the dev:maintenance-release label puts this issue on a project board to track MR requests.

This MR involves 3 sims that support iOS VoiceOver (according to the website).

• ratio-and-proportion
• gravity-force-lab-basics
• john-travoltage

Something about iOS VoiceOver changed, and an unfortunate assumption that we made about event targets is no longer true. The sim will crash if the user "clicks" on readable content in the PDOM. The fix is small and the sim list is small enough that I would not use the batch MR process. I estimate 4 hours to apply the patch in each scenery branch, verify, rebuild, see through QA spot check.