phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Screen readers indicate changes being made to dimmed Extra Sounds Checkbox #895

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air (m1 chip) and Dell

Operating System macOS 13.0.1/Win10

Browser safari/chrome

Problem description For https://github.com/phetsims/qa/issues/868 and https://github.com/phetsims/qa/issues/874, screen readers (VO and NVDA) don't indicate that the Extra Sounds Checkbox is dimmed. When the dimmed checkbox has focus, pressing space results in the screen reader stating that the checkbox is checked/unchecked even though those changes didn't occur.

Comparing it to Ratio and Proportion 1.2.0-rc.3, when the Ratio Lock Checkbox is dimmed, Voiceover indicates that it is dimmed and pressing space doesn't result in VO uttering that the state of the checkbox had changed.

Steps to reproduce

  1. Turn on VO or NVDA
  2. Go to the Audio Tab in Preferences Dialog
  3. Turn Sounds Off
  4. Tab to Extra Sounds and press space--Screenreader will indicate that extra sounds are on

Visuals

https://user-images.githubusercontent.com/87318828/210408533-1aee2e1b-5269-45b4-9921-751e7572e0f9.mov

jessegreenberg commented 1 year ago

Confirmed. It looks like the disabled state is not propagating to descendants. Not an issue for ratio-and-proportion "Lock" checkbox because enabledProperty is getting set directly on the component.

jessegreenberg commented 1 year ago

Two problems. 1) PDOM links to inuptEnabledProperty instead of enabledProperty to set attributes for marking HTML elements as disabled. 2) It only listens for local changes. So if enabled is set on an ancestor Node in the scene graph, descendants will not update correctly.

The above is a temporary workaround to fix this particular case and unblock sims but it is a general problem that will keep coming up.

jessegreenberg commented 1 year ago

I opened https://github.com/phetsims/scenery/issues/1514 as the main issue to track this one. That is where the real fix should happen. @zepumph are the needs for this issue met and can this one be closed?

zepumph commented 1 year ago

Hey @Nancy-Salpepi! @jessegreenberg found another, better work around, and it was working great on NVDA. Can you please double check on master that things sound good and disabled on VO/mac?

Nancy-Salpepi commented 1 year ago

@zepumph @jessegreenberg if I turn off Sounds, I can no longer check the Extra Sounds Checkbox. However, going up one more in the hierarchy, if I turn off Audio Features, I can still check the disabled Extra Sounds Checkbox.

jessegreenberg commented 1 year ago

@Nancy-Salpepi what do you mean by one up in the hierarchy? Can you tell what the virtual cursor is on?

I went back to our first workaround in the above commits. Can you please try again on master?

zepumph commented 1 year ago

@matthew-blackman and I are on the case! We see that toggling all audio off shows the same issue. Commit coming shortly.

zepumph commented 1 year ago

Ok. @matthew-blackman and I were able to add the container disabled by the AudioPreferencesPanel as well. Thanks for the report! I'm going to head to cherry picking now.

zepumph commented 1 year ago

Ok, feel free to close if confirmed fixed in the next RAP spot check.

Nancy-Salpepi commented 1 year ago

In rc.4 I can no longer check/uncheck the Extra Sounds Checkbox when Sounds or Audio Features have been toggled off. Yay! Wanted to note: -VO doesn't say "dimmed" or "unavailable" when toggles or checkboxes are grayed out. -With Audio Features off, NVDA indicates that Voicing and Sounds are unavailable, but doesn't indicate extra sounds are also. @zepumph , I will let you decide if that is OK.

THIS IS WITH NVDA: https://user-images.githubusercontent.com/87318828/210680828-d3541db5-6e38-46a7-bbfd-b88e04cfb419.mov

Nancy-Salpepi commented 1 year ago

Here is the VO VIDEO for the above comment. You can see there is no indication that these options are not currently accessible. In the sim itself, when options are grayed out, VO says "dimmed."

https://user-images.githubusercontent.com/87318828/210872823-11398936-1a20-4f7f-bffa-e7e4eae700ec.MOV

zepumph commented 1 year ago

Thanks @Nancy-Salpepi. That is really helpful. The important thing to note for me is that you can't active or interact with that the disabled elements in VO. If you do, @Nancy-Salpepi said that it just repeats the same thing you hear on focus.

I also confirmed the NVDA behavior above with @Nancy-Salpepi, but then afterward I noticed that I think this is NVDA's fault. Following these steps, I can successfully get NVDA to note the "extra sounds" checkbox as "unavailable"

  1. open https://phet-dev.colorado.edu/html/ratio-and-proportion/1.2.0-rc.4/phet/ratio-and-proportion_all_phet.html
  2. Tab to open the preferences
  3. Tab to audio panel
  4. turn off audio
  5. tab to extra sounds checkbox
  6. Hear: "Extra Sounds check box unavailable not checked"

Then if you continue to mess with and toggle the audio and sound on/off switches, you can get NVDA into a state where the disabled checkbox is spoken as "clickable".

Furthermore, here is what the aria-disabled spec says in https://www.w3.org/TR/wai-aria-1.1/#aria-disabled:

The state of being disabled applies to the current element and all focusable descendant elements of the element on which the aria-disabled attribute is applied.

From this, I feel like further work isn't necessary for us for NVDA or VO support.

@jessegreenberg I feel also like this may not even warrant a bug report to either. It feels too specific and a bit weird, but it doesn't effect the input even behavior. How do you feel about closing this issue?

jessegreenberg commented 1 year ago

I think it says "clickable" because of the listener we add to the element when it is disabled:

this.setPDOMAttribute( 'onclick', enabled ? '' : 'return false' );

I had the exact same thought about aria-disabled and how it applies to descendents. Glad to see that in the spec. Setting the attribute on the element directly may be a workaround but I think we are doing the right thing according to the spec (except for maybe the onclick listener which is a little wacky).

jessegreenberg commented 1 year ago

And I am fine closing this issue!

phet-dev commented 1 year ago

Reopening because there is a TODO marked for this issue.

jessegreenberg commented 1 year ago

I replaced the TODO for the actual common code work that might improve this, https://github.com/phetsims/scenery/issues/1514. Closing.