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

NVDA reads screen summary when exiting a dialog #318

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device Dell

Operating System Win10

Browser Firefox

Problem description For https://github.com/phetsims/qa/issues/868, every time I exit a dialog with the escape key or when focus is on the 'x' NVDA starts to read the sim description. It will say "Friction is an interactive sim. It changes as you play with it. There is a play area and a control" I don't typically test NVDA, so feel free to close if it is working as expected.

Steps to reproduce

  1. Turn on NVDA
  2. Tab to and select either the Preferences or Keyboard dialog
  3. Press 'esc' or space bar if 'x' has focus.

Visuals

https://user-images.githubusercontent.com/87318828/210674297-56a97aa5-47ac-4af3-911b-48dc3c2fe3cc.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: {}
zepumph commented 1 year ago

That is indeed a bug, it should focus and read the name of the preferences nav bar button. I don't see this happening in Ratio and Proportion most likely because of the multi-screen-ness of it. There is logic that manually focuses the beginning of the screen summary, but I wouldn't expect it to be running like this in a single screen sim. I'll take a look.

zepumph commented 1 year ago

So the document.activeElement is the close button until this line, and then afterwards it is the document body.

https://github.com/phetsims/joist/blob/dfdaa4a1b162c08e9f0f1008485a65c4e56b2f03/js/Sim.ts#L852

I also thought that it could have to do with this code accidentally getting triggered before the focusOnHideNode was called but that isn't occurring: https://github.com/phetsims/joist/blob/681c26a5e7de896071887c0023936a023dc34eb9/js/ScreenView.ts#L132-L133

zepumph commented 1 year ago

I'm kinda not sure what to do here, though @jessegreenberg may have better ideas about work around. Here is a patch with 4 console logs, as far as I know this code is run synchronously, so the body has focus for just a few miliseconds, but NVDA picks up the change and starts speaking as soon as it is out of the dialog.

---
Index: joist/js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/Sim.ts b/joist/js/Sim.ts
--- a/joist/js/Sim.ts   (revision 209e7076ccf0d6a046d3655719e3779856341601)
+++ b/joist/js/Sim.ts   (date 1672953854932)
@@ -849,7 +849,9 @@
         this.setPDOMViewsVisible( true );
       }
     }
+    console.log( document.activeElement );
     this.topLayer.removeChild( popup );
+    console.log( document.activeElement );
   }

   private resizeToWindow(): void {
Index: sun/js/Popupable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/Popupable.ts b/sun/js/Popupable.ts
--- a/sun/js/Popupable.ts   (revision 0e4f7479857c7ea4b97ec7ba79b4070fc174f9c5)
+++ b/sun/js/Popupable.ts   (date 1672954685861)
@@ -147,7 +147,10 @@

       // return focus to the Node that had focus when the Popupable was opened (or the focusOnHideNode if provided)
       if ( this._nodeToFocusOnHide && this._nodeToFocusOnHide.focusable ) {
+        console.log( document.activeElement );
+
         this._nodeToFocusOnHide.focus();
+        console.log( document.activeElement );
       }
     }

I also commended out the focus call to the nodeToFocusOnHide and noticed that the original description is exactly the same, so this isn't an interrupting problem: starting with "out of region" and ending with "Play Area and Control"

zepumph commented 1 year ago

If I was being the laziest I could be in regards to this issue, I'd make the argument that we do still EVENTUALLY hear the right thing the first half sentence of the screen summary. But I don't think I'm ready to call it a wont fix until I understand better why this is happening on a single screen sim and not a multi-screen sim.

zepumph commented 1 year ago

With this patch, I add another screen to Friction and it 'fixes' the bug. So there is something about it being a single screen sim:

Subject: [PATCH] use isTRangedProperty to allow UnitConversionProperty to access non-ranged Properties, https://github.com/phetsims/axon/issues/425
---
Index: js/friction-main.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/friction-main.js b/js/friction-main.js
--- a/js/friction-main.js   (revision 8db46b39685efe2e28f39b320d1dad2aaba06323)
+++ b/js/friction-main.js   (date 1672956433436)
@@ -43,13 +43,24 @@

   // Create and start the sim
   const screenTandem = Tandem.ROOT.createTandem( 'frictionScreen' );
+  const screenTandem2 = Tandem.ROOT.createTandem( 'friction2Screen' );
   new Sim( FrictionStrings.friction.titleStringProperty, [
     new Screen( () => new FrictionModel( LAYOUT_BOUNDS.width, LAYOUT_BOUNDS.height, screenTandem.createTandem( 'model' ) ),
       model => new FrictionScreenView( model, screenTandem.createTandem( 'view' ) ), {
         backgroundColorProperty: new Property( '#fff' ),
         tandem: screenTandem,
+        name: new Property( 'hi' ),
+        createKeyboardHelpNode: () => new FrictionKeyboardHelpContent()
+      }
+    ),
+    new Screen( () => new FrictionModel( LAYOUT_BOUNDS.width, LAYOUT_BOUNDS.height, screenTandem2.createTandem( 'model' ) ),
+      model => new FrictionScreenView( model, screenTandem2.createTandem( 'view' ) ), {
+        backgroundColorProperty: new Property( '#fff' ),
+        tandem: screenTandem2,
+        name: new Property( 'hi' ),
         createKeyboardHelpNode: () => new FrictionKeyboardHelpContent()
       }
     )
+
   ], simOptions ).start();
 } );
\ No newline at end of file

I checked again and it doesn't have anything to do with focus changing, from the screen view visibleProperty or elsewhere. The console logs from https://github.com/phetsims/friction/issues/318#issuecomment-1372814785 are the same: Close button, body, body, Nav bar button.

zepumph commented 1 year ago

Furthermore, if you take a multiscreen sim and add the ?screens=1 query parameter, it is just a single screen, but this bug doesn't occur. In that mode there is no phet.joist.sim.homeScreen, and also no PDOM markup for the nav bar screen buttons.

zepumph commented 1 year ago

@jessegreenberg and I found that this isn't caused by https://github.com/phetsims/joist/blob/c8f4618b401faf103b0b96fc8feece7f20bb4b75/js/HighlightVisibilityController.ts#L94-L120

zepumph commented 1 year ago

Ok. So the base issue brought here was that we wish we didn't hear anything but the newly focused item. That is not possible. NVDA will say "out of region" and then the "top" item in the newly visible PDOM when closing the dialog. Then it will next say the newly focused button.

The weirdness of this bug because that for single screen sims with names that are just a single word, the top level h1 is ignored, and the first screen summary sentance-ish is spoken. For all other cases where the h1 is multiple words, includes a comma or emdash, etc, NVDA picks that to speak when closing the dialog.

@jessegreenberg are ready to close this as a won't fix, as we aren't going to change the title of this sim to be multiple words. I also don't see an easy way to submit a bug report to them. Closing