Tapping paragraph instead of button disables Mobile VO #1631

KatieWoe · 2024-05-08T19:24:21Z

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

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

Visuals

IMG_5261.MOV

jessegreenberg · 2024-05-08T19:31:58Z

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

More info:

And in the debug version:

jessegreenberg · 2024-05-08T20:09:34Z

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 · 2024-05-08T20:31:42Z

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.

…see #1631

jessegreenberg · 2024-05-08T20:55:04Z

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

KatieWoe · 2024-05-08T20:56:38Z

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 · 2024-05-08T20:58:35Z

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 · 2024-05-08T21:21:48Z

@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.

zepumph · 2025-01-31T18:09:08Z

I'm going to add this into phetsims/perennial#430. I am going to cherry pick

92e8bc6

into the latest version of the published sims:

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

zepumph · 2025-01-31T21:14:53Z

All three of these sims are quite different from the commit. I would like to look at this with @jessegreenberg before trying to apply these.

So far:

92e8bc6 did not apply

I created locally 24672b4904a84a2e46a943d0db26d460df847c55 which ratio and proportion applied.
GFLB I got as far as this patch, but it is just too different to proceed at this time.

Subject: [PATCH] isElementUnderPDOM can return false if the element is the PDOM root, see https://github.com/phetsims/scenery/issues/1631

(cherry picked from commit 92e8bc694d4c1d91c36d519ccf517a84f46ff6c0)
(cherry picked from commit 24672b4904a84a2e46a943d0db26d460df847c55)

# Conflicts:
#	js/input/Input.js
---
Index: js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/display/Display.js b/js/display/Display.js
--- a/js/display/Display.js	(revision 90fea3138c516450ad5a004ed54489d64a76b800)
+++ b/js/display/Display.js	(date 1738357913057)
@@ -850,6 +850,25 @@
     return this._accessible;
   }
 
+  /**
+   * Returns true if the element is in the PDOM. That is only possible if the display is accessible.
+   * @public
+   * @param element
+   * @param allowRoot - If true, the root of the PDOM is also considered to be "under" the PDOM.
+   */
+  isElementUnderPDOM( element, allowRoot ) {
+    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 );
+  }
+
   /**
    * Implements a workaround that prevents DOM focus from leaving the Display in FullScreen mode. There is
    * a bug in some browsers where DOM focus can be permanently lost if tabbing out of the FullScreen element,
Index: js/input/Input.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/input/Input.js b/js/input/Input.js
--- a/js/input/Input.js	(revision 90fea3138c516450ad5a004ed54489d64a76b800)
+++ b/js/input/Input.js	(date 1738358075695)
@@ -1100,8 +1100,17 @@
       return domEvent[ TARGET_SUBSTITUTE_KEY ][ PDOMUtils.DATA_TRAIL_ID ];
     }
     else {
-      assert && assert( domEvent.target instanceof window.Element );
-      return domEvent.target.getAttribute( PDOMUtils.DATA_TRAIL_ID );
+      // assert && assert( domEvent.target instanceof window.Element );
+      // return domEvent.target.getAttribute( PDOMUtils.DATA_TRAIL_ID );
+      const target = domEvent.target;
+      if ( target && target instanceof window.Element && this.display.isElementUnderPDOM( target, false ) ) {
+        const trailIndices = target.getAttribute( PDOMUtils.DATA_TRAIL_ID );
+        assert && assert( trailIndices, 'should not be null' );
+        return PDOMInstance.uniqueIdToTrail( this.display, trailIndices );
+      }
+      else{
+        // TODO:?????
+      }
     }
   }
 
@@ -1604,7 +1613,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.isTargetUnderPDOM( event.target ) ) {
+    if ( this.display.isElementUnderPDOM( event.target, true ) ) {
       return;
     }
 
@@ -1640,7 +1649,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.isTargetUnderPDOM( event.target ) ) {
+    if ( this.display.isElementUnderPDOM( event.target, true ) ) {
       return;
     }

zepumph · 2025-01-31T21:17:37Z

I edited my progress out of the maintenance state, and can easily get it back with this:

  // https://github.com/phetsims/scenery/issues/1631
  // Fixes: https://github.com/phetsims/scenery/commit/92e8bc694d4c1d91c36d519ccf517a84f46ff6c0
  // await m.createPatch( 'scenery', 'https://github.com/phetsims/scenery/issues/1631', 'iOSVoiceOver')
  // await m.addNeededPatch( 'ratio-and-proportion', '1.2', 'iOSVoiceOver');
  // await m.addNeededPatch( 'john-travoltage', '1.6', 'iOSVoiceOver');
  // await m.addNeededPatch( 'gravity-force-lab-basics', '1.1', 'iOSVoiceOver');
  // await m.addPatchSHA( 'iOSVoiceOver', '92e8bc694d4c1d91c36d519ccf517a84f46ff6c0')
  // await m.addPatchSHA( 'iOSVoiceOver', '24672b4904a84a2e46a943d0db26d460df847c55')

…see #1631 (cherry picked from commit 92e8bc6)

zepumph · 2025-02-04T20:41:29Z

I could not reproduce this problem on GFL:B and JT. Only for RAP. I'm proceeding with the RC deploy for that sim.

zepumph · 2025-02-04T21:53:52Z

Ok. RC has been deployed.

For QA Testing: Please make sure that the above bug is not reproducible. The easiest way I found it could be reproduced was to swipe to the description of the home screen button (like in the video). Please test the production version of the sim on phet.colorado.edu as you go also to make sure you can see that there is indeed a problem there, and the fix in the RC. I used an ipad with safari browser.

KatieWoe · 2025-02-05T18:22:43Z

RaP looks fixed in the MR after a quick check.

zepumph · 2025-02-10T18:06:51Z

RaP has been pushed to production. Closing

KatieWoe added the type:bug label May 8, 2024

KatieWoe assigned jbphet and jessegreenberg May 8, 2024

jessegreenberg added the dev:description label May 8, 2024

This was referenced May 8, 2024

RC Test: Balancing Act 1.3.0-rc.1 phetsims/qa#1084

Closed

Dev Test: Greenhouse Effect 1.3.0-dev.11 phetsims/qa#1080

Closed

jessegreenberg added a commit that referenced this issue May 8, 2024

isElementUnderPDOM can return false if the element is the PDOM root, …

92e8bc6

…see #1631

jessegreenberg assigned KatieWoe and unassigned jbphet and jessegreenberg May 8, 2024

jessegreenberg added the dev:maintenance-release label May 8, 2024

jessegreenberg assigned jessegreenberg and unassigned KatieWoe May 8, 2024

jessegreenberg mentioned this issue May 23, 2024

Latest Mobile VO fails to read out responsive descriptions phetsims/a11y-research#198

Open

marlitas added dev:description and removed dev:description labels Oct 21, 2024

zepumph mentioned this issue Jan 31, 2025

A batch maintenance release is needed phetsims/perennial#430

Closed

zepumph assigned zepumph and unassigned jessegreenberg Jan 31, 2025

zepumph pushed a commit that referenced this issue Feb 4, 2025

isElementUnderPDOM can return false if the element is the PDOM root, …

3b1b9bd

…see #1631 (cherry picked from commit 92e8bc6)

zepumph added a commit to phetsims/ratio-and-proportion that referenced this issue Feb 4, 2025

updated dependencies.json for phetsims/scenery#1631

5779a53

zepumph added a commit to phetsims/ratio-and-proportion that referenced this issue Feb 4, 2025

Bumping version to 1.2.14-rc.1, phetsims/scenery#1631

fc2cb7c

zepumph mentioned this issue Feb 4, 2025

Maintenance Release Testing phetsims/qa#1201

Closed

zepumph added a commit to phetsims/ratio-and-proportion that referenced this issue Feb 8, 2025

Bumping version to 1.2.14, phetsims/scenery#1631

726e9ee

zepumph closed this as completed Feb 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tapping paragraph instead of button disables Mobile VO #1631

Tapping paragraph instead of button disables Mobile VO #1631

KatieWoe commented May 8, 2024

jessegreenberg commented May 8, 2024 •

edited

Loading

jessegreenberg commented May 8, 2024

jessegreenberg commented May 8, 2024 •

edited

Loading

jessegreenberg commented May 8, 2024

KatieWoe commented May 8, 2024

jessegreenberg commented May 8, 2024

jessegreenberg commented May 8, 2024

zepumph commented Jan 31, 2025

zepumph commented Jan 31, 2025

zepumph commented Jan 31, 2025

zepumph commented Feb 4, 2025

zepumph commented Feb 4, 2025

KatieWoe commented Feb 5, 2025

zepumph commented Feb 10, 2025

Tapping paragraph instead of button disables Mobile VO #1631

Tapping paragraph instead of button disables Mobile VO #1631

Comments

KatieWoe commented May 8, 2024

jessegreenberg commented May 8, 2024 • edited Loading

jessegreenberg commented May 8, 2024

jessegreenberg commented May 8, 2024 • edited Loading

jessegreenberg commented May 8, 2024

KatieWoe commented May 8, 2024

jessegreenberg commented May 8, 2024

jessegreenberg commented May 8, 2024

zepumph commented Jan 31, 2025

zepumph commented Jan 31, 2025

zepumph commented Jan 31, 2025

zepumph commented Feb 4, 2025

zepumph commented Feb 4, 2025

KatieWoe commented Feb 5, 2025

zepumph commented Feb 10, 2025

jessegreenberg commented May 8, 2024 •

edited

Loading

jessegreenberg commented May 8, 2024 •

edited

Loading