-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tapping paragraph instead of button disables Mobile VO #1631
Comments
Here is an error from @terracoda 's phone, tethered to her Mac: And in the debug version: |
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. |
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. |
@KatieWoe is there anything else you would like to test for this? |
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. |
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. |
@marlitas let me know that the This MR involves 3 sims that support iOS VoiceOver (according to the website).
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. |
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
Visuals
IMG_5261.MOV
The text was updated successfully, but these errors were encountered: