Skip to content
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

Mobile VO doesn't read all alerts #317

Closed
Nancy-Salpepi opened this issue Jan 4, 2023 · 6 comments
Closed

Mobile VO doesn't read all alerts #317

Nancy-Salpepi opened this issue Jan 4, 2023 · 6 comments

Comments

@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Jan 4, 2023

Test device
iPad 9th generation

Operating System
iPadOS 16.1.1

Browser
safari

Problem description
For phetsims/qa#868, when I double tap to grab the zoomed in book, I don't hear alerts like "Grabbed," "Lightly of physics book," or "Released." There is also a slight delay before I can actually move the Zoomed-in book. I don't see this with the non Zoomed-in book.

Steps to reproduce

  1. Turn on VO
  2. Swipe to the zoomed-in book
  3. Double tap to grab and move finger around

Visuals

frictionMobileVO.mov
Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Friction‬ URL: https://phet-dev.colorado.edu/html/friction/1.6.0-dev.28/phet/friction_all_phet.html Version: 1.6.0-dev.28 2022-12-20 18:30:08 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/108.0.0.0 Safari/537.36 Language: en-US Window: 1475x780 Pixel Ratio: 1.7999999523162842/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
@jessegreenberg
Copy link
Contributor

It is possible the double-tap gesture is sending events to a place that is not clickable in the sim. setPDOMTransformSourceNode may help with this.

@zepumph
Copy link
Member

zepumph commented Jan 5, 2023

Is there a way to debug positionInPDOM with bounding rectangles?

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jan 5, 2023

I don't think so unfortunately. You could hack PDOMSiblingStyle such that the elements are visible. The black rectangle with VO should also be the rectangle we care about if you have access to VO.

positionInPDOM will use the bounds of whatever has the tagName for the focusable zoomed in book, if you can draw a rectangle around that Node.

And it is possible the problem is something else entirely :/

@zepumph
Copy link
Member

zepumph commented Jan 5, 2023

I think you are totally on the right track, the center of the currently positioned node won't hit the visual magnified book:
image

I used this patch to get this view:

Subject: [PATCH] fdsa
---
Index: js/accessibility/pdom/ParallelDOM.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/ParallelDOM.ts b/js/accessibility/pdom/ParallelDOM.ts
--- a/js/accessibility/pdom/ParallelDOM.ts	(revision b5b1ed98b9546e6ee9c48f1394cc7a88203623d4)
+++ b/js/accessibility/pdom/ParallelDOM.ts	(date 1672959367007)
@@ -131,7 +131,7 @@
 import PhetioObject, { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js';
 import UtteranceQueue from '../../../../utterance-queue/js/UtteranceQueue.js';
 import { TAlertable } from '../../../../utterance-queue/js/Utterance.js';
-import { Node, PDOMDisplaysInfo, PDOMInstance, PDOMPeer, PDOMTree, PDOMUtils, scenery, Trail } from '../../imports.js';
+import { Node, PDOMDisplaysInfo, PDOMInstance, PDOMPeer, PDOMTree, PDOMUtils, Rectangle, scenery, Trail } from '../../imports.js';
 import { Highlight } from '../../overlays/HighlightOverlay.js';
 import optionize from '../../../../phet-core/js/optionize.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
@@ -2573,6 +2573,13 @@
   public setPositionInPDOM( positionInPDOM: boolean ): void {
     this._positionInPDOM = positionInPDOM;
 
+    if ( positionInPDOM ) {
+      setTimeout( () => {
+        // insertChild for z order
+        this.parent.insertChild( 0, Rectangle.bounds( this.bounds, { stroke: 'red' } ) );
+      }, 30 );
+    }
+
     for ( let i = 0; i < this._pdomInstances.length; i++ ) {
       this._pdomInstances[ i ].peer!.setPositionInPDOM( positionInPDOM );
     }

@zepumph
Copy link
Member

zepumph commented Jan 5, 2023

I directly tested dev.28 and immediately master with the above change and heard a very positive change! @Nancy-Salpepi can you please test on master and feel free to close if all is well. Furthermore note that the black VO highlight now mimics the focus higlight in the sim. In the future seeing those as different on mobile VO should be noted as a potential problem for many interactive elements, especially "custom" ones like books and rulers (in gravity force lab, another spot where this fix has been applied).

Thanks!@

@Nancy-Salpepi
Copy link
Author

Things look good on master. While testing I noticed another small bug, which I will post an issue for now. Sorry I didn't catch it the last time. I think I was too distracted with this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants