Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WavelengthSlider, HSlider and Accessibility #355

Closed
samreid opened this issue Mar 8, 2018 · 8 comments
Closed

WavelengthSlider, HSlider and Accessibility #355

samreid opened this issue Mar 8, 2018 · 8 comments

Comments

@samreid
Copy link
Member

samreid commented Mar 8, 2018

At today's wave interference meeting, we noticed that WavelengthSlider (which we will need for Wave Interference) does not extend or leverage HSlider (which is already accessible). Should we rework WavelengthSlider so it uses HSlider, to benefit from its a11y? Or should we add new a11y support for it? Something else?

@jessegreenberg
Copy link
Contributor

We should be able to make WavelengthSlider accessible by mixing in the AccessibleSlider trait in sun.

@samreid
Copy link
Member Author

samreid commented Mar 8, 2018

@ariel-phet said this would be a good thing for me to learn how to do, assigned to me.

@jessegreenberg
Copy link
Contributor

Sounds good @samreid. I think the WavelengthSlider should behave and be implemented similarly to NumberControl because it has the left/right tweaker buttons. Let me know if we should pair on this sometime soon.

@samreid
Copy link
Member Author

samreid commented Mar 12, 2018

@jessegreenberg can you describe how I can test the result? Will I use keyboard navigation and/or a screen reader for testing? Is this documented somewhere?

@samreid samreid assigned jessegreenberg and unassigned samreid Mar 12, 2018
@jessegreenberg
Copy link
Contributor

@samreid testing with a keyboard alone should be sufficient for this. Sorry, no formal documentation for adding a11y related stuff yet. We will want to verify that the slider has these features, listed in the top of AccessibleSlider.js

/**
 * - Arrow keys increment/decrement the slider by a specified step size.
 * - Holding shift with arrow keys will increment/decrement by alternative step size, usually smaller than default.
 * - Page Up and Page Down increments/decrements value by an alternative step size, usually larger than default.
 * - Home key sets value to its minimum.
 * - End key sets value to its maximum.
*/

These should get added by mixing in AccessibleSlider.

We probably want the focus highlight around the thumb, not the whole slider. You could mix AccessibleSlider directly into the thumb to get this automatically. Or you could create a unique focus highlight for the WavelengthSlider and translate it with the thumb.

@samreid
Copy link
Member Author

samreid commented Mar 26, 2018

I mixed in AccessibleSlider to WavelengthSlider in the preceding commit. I followed the pattern in HSlider and things went smoothly.

  • @jessegreenberg can you please review and test the changes?
  • I noticed that Home/End on my keyboard did not change the values on any of the sliders? Can you verify it is still working? Or perhaps a problem with my keyboard?
  • This code seemed odd:
    // a11y - custom focus highlight that surrounds and moves with the thumb
    this.focusHighlight = new FocusHighlightFromNode( thumb );

Should it be marked @public (a11y)? Also, how does assigning it as an object property get it added to the scene graph properly? Does this have to happen before the initializeAccessibleSlider call? What happens if you set it afterwards? Why isn't it an option to the initializeAccessibleSlider call? Where is that attribute documented?

If you want to use Wave Interference as the test harness, you will need to select the "Light" scene to see the wavelength slider.

@jessegreenberg
Copy link
Contributor

Changes looks great to me @samreid. I tested in color-vision and wave-interference it is working great.

I noticed that Home/End on my keyboard did not change the values on any of the sliders? Can you verify it is still working? Or perhaps a problem with my keyboard?

Home/End are working for me in Windows 10 Chrome and macOS 10.13.3 Safari. Home/End on both platforms are "fn + left arrow" and "fn + right arrow" respectively. Is that what you are using?

This code seemed odd...Should it be marked @public (a11y)? Also, how does assigning it as an object property get it added to the scene graph properly? Does this have to happen before the initializeAccessibleSlider call? What happens if you set it afterwards? Why isn't it an option to the initializeAccessibleSlider call? Where is that attribute documented?

This is actually an HTML5 setter, not an object property. The setter is documented in scenery/Accessibility.js. Since it is a setter for Node.js (through the Accessibility trait), it can be specified through options in mutate(), Node.call(), or through the setter after Node.call().

@samreid
Copy link
Member Author

samreid commented Mar 28, 2018

Home/End are working for me in Windows 10 Chrome and macOS 10.13.3 Safari. Home/End on both platforms are "fn + left arrow" and "fn + right arrow" respectively. Is that what you are using?

I'm using a USB keyboard. Home/End seem to be working now (not sure what was wrong before). Your explanation about seems reasonable--it seems we can set that before or after the initializeAccessibleSlider call. Closing.

@samreid samreid closed this as completed Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants