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

Add Voicing an Interactive Description to ToggleSwitch #883

Closed jessegreenberg closed 1 year ago

jessegreenberg commented 1 year ago

This is related to work that @marlitas is doing in Preferences. It will be easier to refactor out the ToggleSwitch from PreferencesToggleSwitch if Voicing/Interactive Description code is implemented right in ToggleSwitch so that clients don't have to re-implement that part of the control. And it is the right direction for a11y to move this common code into the component.

This work will involve

jessegreenberg commented 1 year ago

Add a toggledEmitter so that we can respond to the toggle change in a custom way when we need to.

I realized I did not need to do this to support the special case for Voicing. Keeping the context response in a listener on the Property is working well for VoicingPanelSection and there is no need to add an Emitter.

@zepumph would you mind reviewing these changes? We discussed adding the Emitter to ToggleSwitch together, want to confirm you are OK skipping this part.

zepumph commented 1 year ago

Add a toggledEmitter so that we can respond to the toggle change in a custom way when we need to. . . . OK skipping this part.

Sounds good.

It also seems important to note https://github.com/phetsims/joist/issues/842, where in that issue PreferencesToggleSwitch was deleted and ToggleSwitch was directly made the replacement.

I am confused about https://github.com/phetsims/joist/commit/b760924d94da1c11f519f0524e89c61a0e67d45b#diff-abbd30dc99a117ae11bba20f33ae50f746f38d0ce4f7df5e0be34d17fd299947R92-R101, and it doesn't look like it was transferred to ToggleSwitch. Is this code important at all? A light explanation would be nice, but I don't need all the context if you feel good.

Thanks for doing this work.

jessegreenberg commented 1 year ago

Thank you for reviewing!

I have used a11yLabel in a couple places, ComboBox, ToggleSwitch, VoicingPanelSection. I like a11yName better, Ill rename each to that. As you said, we are blocked from using accessibleName at the moment.

I am confused about...

Thanks! Those were options specific to PreferencesTogleSwitch but I don't see any usages at the moment? I reviewed usages within the PreferencesDialog/VoicingToolbarItem and components look correct so Ill go ahead and remove these options.

Back to @zepumph to confirm follow-up changes are OK.

zepumph commented 1 year ago

Wonderful, thanks.

jessegreenberg commented 1 year ago

OK, thanks!