phetsims / greenhouse-effect

"Greenhouse Effect" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

NVDA reads Surface Albedo slider followed by "Invalid Entry" #388

Closed KatieWoe closed 7 months ago

KatieWoe commented 8 months ago

Test device Samsung Operating System Win 11 Browser Firefox Problem description For https://github.com/phetsims/qa/issues/1033 When you focus on the Surface Albedo slider, it will read the current value then "Invalid Entry"

Visuals

https://github.com/phetsims/greenhouse-effect/assets/41024075/3cb64318-9573-4d74-b1bf-88eee4371c14

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: ‪Greenhouse Effect‬ URL: https://phet-dev.colorado.edu/html/greenhouse-effect/1.3.0-dev.3/phet/greenhouse-effect_all_phet.html Version: 1.3.0-dev.3 2024-01-23 21:58:15 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:122.0) Gecko/20100101 Firefox/122.0 Language: en-US Window: 1536x731 Pixel Ratio: 1.25/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 Vendor: Mozilla (ANGLE (Intel, Intel(R) HD Graphics Direct3D11 vs_5_0 ps_5_0)) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}
jbphet commented 8 months ago

I've done some investigation and have a few clues into why this is happening, though I don't have a solution yet. The problem occurs in Firefox, but not in Chrome. I examined the PDOM element where the problem occurs and compared it to the one for the nearby Solar Intensity slider, where the problem does not occur. I tried changing several attributes of the element using the Firefox developer tools, and the one thing that seemed to make the problem go away was to change the attribute that said step="0.009000000000000001" to step="0.009". It would appear that a floating point rounding error is leading to a long string that describes the step value, and that long string in turn is hitting a bug in either Firefox or NVDA. Since the same long value for the step attribute is seen in Chrome (I double checked this), it seems more likely that it's a bug of some kind in Firefox.

We could problem fix it by doing some rounding where this step value is calculated.

jbphet commented 8 months ago

@jessegreenberg helped me find the place in the common code where the step size was being set to this long value in the PDOM. It's in AccessibleValueHandler._updateSiblingStepAttribute, and the code in question is there to work around some issues where the step sizes that PhET sometimes uses for sliders weren't always working well with screen readers, so a step attribute is set somewhat arbitrarily. It looks like this:

      // step is too small relative to full range for VoiceOver to receive input, fall back to portion of
      // the max value as a workaround
      if ( stepValue / mappedLength < 1e-5 ) {
        stepValue = mappedMax / 100;
      }

      this.setPDOMAttribute( 'step', stepValue );

In the case where this is leading to the long value, the mappedMax value is 0.9, and if I go into the JS console in Chrome and ask it for 0.9 / 100 I see this:

image

Sheesh. Thanks binary math.

So, I think we need to set the number of significant figures for these values to a reasonable number of digits. I found a Slack Overflow article that suggested something like this (I'm mapping the suggestion to our particular case):

stepValue = Number( ( mappedMax / 100 ).toPrecision( 8 ) );

I'll give this approach a try.

jbphet commented 8 months ago

I wanted to verify that using toPrecision wouldn't turn very small values into zeros, so I wrote the following test code:

  const x = 0.9/100;
  console.log( `x = ${x}` );
  const xWithPrecision = Number( x.toPrecision( 10 ) );
  console.log( `xWithPrecision = ${xWithPrecision}` );
  const y = 0.000000000000000001;
  console.log( `y = ${y}` );
  const yWithPrecision = Number( y.toPrecision( 10 ) );
  console.log( `yWithPrecision = ${yWithPrecision}` );

The first part is to duplicate the situation we're seeing, the second is to test using a very small value. The output is below.

x = 0.009000000000000001
xWithPrecision = 0.009
y = 1e-18
yWithPrecision = 1e-18

I also verified the the documentation for the toPrecision method indicates that it is using significant figures, and it is. I'm now convinced that this is a reasonable approach.

jbphet commented 8 months ago

This problem can be duplicated on the currently published version of Fourier: Making Waves, Wave Packet screen, 3 of the four sliders in the control panel on the right.

jbphet commented 8 months ago

@jessegreenberg - I committed a fix for this, please review. This change does not fix the problem for the Fourier sim, so I think it may be indicative of a larger problem where any long numeric values in the PDOM are an issue for Firefox and screen readers. Once you've reviewed this, let's talk about logging issues for the other things and possibly contacting Mozilla about what we're seeing here.

jessegreenberg commented 7 months ago

Thanks @jbphet - this looked great. Over in https://github.com/phetsims/sun/issues/873 I learned about the any value for the step attribute. See https://github.com/phetsims/sun/issues/873#issuecomment-1930917430. If that works OK I think we can remove this workaround entirely. Lets see how testing goes in https://github.com/phetsims/qa/issues/1035.

jessegreenberg commented 7 months ago

https://github.com/phetsims/qa/issues/1035 did not go well, but an alternative in https://github.com/phetsims/sun/issues/873 is ready for review that still fixes this issue.

jbphet commented 7 months ago

Review was done in https://github.com/phetsims/sun/issues/873. This is now ready for testing by QA and can be closed when they've verified that it's fixed.

KatieWoe commented 7 months ago

Looks ok to me