phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

NVDA says "invalid entry" in Firefox when long numeric strings are present #873

Closed jbphet closed 5 months ago

jbphet commented 6 months ago

In https://github.com/phetsims/greenhouse-effect/issues/388 it was reported that Firefox and NVDA on Windows was sometimes saying "invalid entry" as part of its description of a slider. I tracked this down to a long string for a numeric value being present in the PDOM, specifically for the step value, and when the length of the string was reduced the problem went away. This is essentially a workaround for a problem in Firefox, and we should consider reporting the problem to them.

In the process of exploring this I found that it was not specific to Greenhouse Effect. I was able to duplicate the issue on the published version of Fourier: Making Waves when navigating to the 3 lower sliders on the "Wave Packet" screen. Changing the length of the step value did not fix the problem in Fourier, but inspection with the Firefox developer tools indicates several other long strings. Here's a screenshot:

image

I suspect that limiting the length of some or all of these values will stop NVDA from saying "invalid value".

For reference, Firefox version is 122.0.1, NVDA 2023.3.3, Fourier 1.0.17, OS is Windows 11.

jbphet commented 6 months ago

The following patch makes the problem go away. I'm not suggesting this as a fix, just further demonstrating the nature of the problem and showing that it really is related to numeric string lengths.

```diff Subject: [PATCH] experimental patch for "invalid entry" problem --- 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 b1cdc5afbd6763c9a74e4ba5126554253dfd4799) +++ b/js/accessibility/pdom/ParallelDOM.ts (date 1707245481735) @@ -2461,6 +2461,16 @@ assert && providedOptions && assert( Object.getPrototypeOf( providedOptions ) === Object.prototype, 'Extra prototype on pdomAttribute options object is a code smell' ); + if ( typeof value === 'number' && value.toString().length > 10 ) { + + // Limit precision to prevent weird behavior. + console.log( '--------------------' ); + console.log( `attribute = ${attribute}` ); + console.log( `value (before mod) = ${value}` ); + value = value.toPrecision( 8 ); + console.log( `value (after mod) = ${value}` ); + } + const options = optionize()( { // {string|null} - If non-null, will set the attribute with the specified namespace. This can be required ```
jbphet commented 6 months ago

Assigning to @jessegreenberg, since he is the main author for the PDOM code, and @pixelzoom, because he was the primary author of Fourier.

pixelzoom commented 6 months ago

I don't think this problem should have a sim-specific solution. It's a PDOM problem. So I'm going to unassign myself. Please reassign me if you disagree.

jessegreenberg commented 6 months ago

Thanks for diving into this @jbphet. Do you think the toPrecision fix you used in https://github.com/phetsims/greenhouse-effect/issues/388 could be used for these values as well? In the above commit I factored out toPrecision so it is used everywhere a value is set as an attribute in the PDOM. It sounds OK in greenhouse-effect and I am not hearing 'invalid entry" when testing in fourier. Can you please review?

jessegreenberg commented 6 months ago

Here is the description of the step attribute from MDN:

The step sets the stepping interval when clicking up and down spinner buttons, moving a slider left and right on a range, and validating the different date types. If not explicitly included, step defaults to 1 for number and range, and 1 unit type (minute, week, month, day) for the date/time input types. The value must be a positive number - integer or float — or the special value any, which means no stepping is implied and any value is allowed (barring other constraints, such as min and max).

...

If any is not explicitly set, valid values for the number, date/time input types, and range input types are equal to the basis for stepping - the min value and increments of the step value, up to the max value, if specified.

So... I think we should try the any value for step, and see if screen readers like that better.

EDIT: A comment in the code says

We tried to use the any attribute which is valid according to DOM specification but screen readers generally don't support it. See https://github.com/phetsims/sun/issues/413.

this attribute is only added because it is required to receive accessibility events on all browsers

And another comment in old git history says

// HTML requires that the value be evenly divisible by the step size to receive 'input' events, which is critical to work with mobile AT like VoiceOver

Its worth testing now to see if this has improved 6 years later.

jessegreenberg commented 6 months ago

Using any sounds great with NVDA. I am most worried about iOS VoiceOver based on comments above. I don't have access to a device with that right now, so I made https://github.com/phetsims/qa/issues/1035 to test.

jessegreenberg commented 6 months ago

From https://github.com/phetsims/qa/issues/1035, the any attribute is not supported by iOS VoiceOver and makes the slider unusable. Ill revert https://github.com/phetsims/sun/commit/a90b5d39cea71b74606049cb4e1dfd711164a70e.

jessegreenberg commented 6 months ago

Alright, I am proposing the above commit which uses a platform check to do some extra work on iOS Safari. This is a bummer but here is my thinking:

1) We use the any attribute everywhere except for iOS Safari, where we calculate the step value. This way we can use the simple any value for most platforms and avoid adding lots of complicated workarounds like https://github.com/phetsims/sun/commit/42be9030ee3141740b564e7a13c270e5ae54fb03. 2) We know that the step calculation sounds nice for iOS VoiceOver which doesn't say "invalid" like NVDA does. 3) We should submit a bug report to Apple to support the 'any' attribute and maybe someday we can remove this.

@jbphet can you please review? Does this seem reasonable to you?

jbphet commented 5 months ago

I tested this on Firefox on my Win 11 system and verified that the problem was no longer occurring in Greenhouse Effect and Fourier. Looking over the code, the approach seems reasonable, although I agree that it's unfortunate to have to put platform-specific code in here. Here are a few additional thoughts:

Also note that I made a few minor editorial changes to the comments in this code.

jessegreenberg commented 5 months ago

OK, thanks for the review!

I'd suggest adding a TODO...

Done, I made https://github.com/phetsims/a11y-research/issues/191 for the TODO and also to submit a bug report to Apple.

Have QA test this if they haven't already.

OK, sounds good - I made https://github.com/phetsims/qa/issues/1040

and also to logging one with Mozilla

Sounds good, I made https://github.com/phetsims/a11y-research/issues/192. I assigned this one to both of us for whoever can get to it first.

Ill leave this open until https://github.com/phetsims/qa/issues/1040 is closed.

jessegreenberg commented 5 months ago

https://github.com/phetsims/qa/issues/1040 has been confirmed so this one can be closed. We will follow up on the bug reports in the new issues. Closing.