phetsims / friction

"Friction" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/friction
GNU General Public License v3.0
4 stars 6 forks source link

Mobile VO doesn't read all alerts #317

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device iPad 9th generation

Operating System iPadOS 16.1.1

Browser safari

Problem description For https://github.com/phetsims/qa/issues/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

https://user-images.githubusercontent.com/87318828/210623961-fb824ed8-9566-44bd-83d0-3a009a5dc7a2.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 commented 1 year ago

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 commented 1 year ago

Is there a way to debug positionInPDOM with bounding rectangles?

jessegreenberg commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.