a11y bloat in Slider

[pixelzoom]

Prior to adding a11y to HSlider, it was 393 lines of code. It’s now 632 lines, a 60% increase.

Before a11y, 5/3/17: https://github.com/phetsims/sun/blob/ed3e260b55d430c2980c854f27a4296f04ed6ebb/js/HSlider.js
Current, 10/25/17:
https://github.com/phetsims/sun/blob/f309a64160ae1802ba57a1924cc4b5c25ded6c19/js/HSlider.js

And though I was the author of HSlider, I’d have a difficult time modifying/enhancing it now.

Is this indicative of the impact of adding a11y?
Can we separate a11y and non-a11y functionality?

[pixelzoom]

Slack developer channel:

@jessegreenberg Typically a11y changes are much smaller, HSlider is an atypical case that required a lot of work. We try to let the browser natively handle the keyboard interaction so we don't have to add much, but that wasn't possible for HSlider. (edited)

@pixelzoom Do you think it would be beneficial (or possible) to have more separation of a11y and non-a11y functionality in HSlider?

@jessegreenberg Yes I think it is possible and beneficial for the reason you described.

[pixelzoom]

HSilder is apparently atypical. Let's investigate separating ally and non-ally features in HSlider.

[pixelzoom]

Tasks for 10/26/17 developer meeting:
• make all developers aware of a11y bloat, be on the lookout for it
• brainstorm about how to encapsulate a11y features in HSlider specifically
• decide whether what is done for HSlider should be applied anywhere else

[jessegreenberg]

Would a "traits" pattern work? (Not mixin because of phetsims/scenery#700). Implementation for accessibility could be separated into another file called something like AccessibleSlider. Then HSlider would look like this.

//  in the constructor
this.initializeAccessibleSlider( options );

// after inherit,  HSlider composed with AccessibleSlider
AccessibleSlider.compose( HSlider );

[pixelzoom]

10/26/17 dev meeting:
JO: Trait not appropriate here. Traits usually used for reusable code. Probably relying on internal details of HSlider, so pulling out into a trait is difficult
SR: a11y should remain in HSlider. consider factoring out accessible listener, possibly as an inner type?
CM: do a11y "step" features play nice with other features (snap, etc.)

@jessegreenberg will look at factoring out accessible listener.
@jonathanolson will review a11y changes to HSlider when @jessegreenberg is done.

[jessegreenberg]

Trait not appropriate here. Traits usually used for reusable code.

Actually, this trait could be reused outside of HSlider. From phetsims/scenery-phet#341 the keyboard interaction for NumberControl is meant to behave exactly like HSlider, so it would be great if a trait for a11y to introduce slider functionality there. It might even be used by anything that can move in 1-D if we want those things to behave just like sliders.

I would still like to look into separating into a trait.

[jessegreenberg]

I moved almost all a11y code out of HSlider and into a trait called AccessibleSlider that can be used for HSlider and other Nodes. It is now mixed into HSlider, as well asNumberControl and the draggable objects in inverse-square-law-common. Making this a trait already seems very beneficial. @jonathanolson can you please review?

The main implementation is in https://github.com/phetsims/sun/blob/master/js/accessibility/AccessibleSlider.js. In particular, I would love your thoughts on its location (created a new folder sun/js/accessibility) and the signature for initializeAccessibleSlider.

[pixelzoom]

The issue is now old enough that it no longer pertains to HSlider. It now pertains to the base class, Slider. Updating title accordingly.

[zepumph]

AccessibleSlider has been working well for us for 5 years now. The file has been thoroughly modified and improved upon in that time. I think this issue is ready to be closed. @pixelzoom is there still bloat in Slider that you would like to change?

[pixelzoom]

This was addressed via the AccessibleSlider trait. Closing.