generalize HSlider to support horizontal and vertical orientations

[pixelzoom]

We currently have HSlider, whose implementation is specific to horizontal orientation. Several sims need a vertical slider, e.g. graphing-quadratics, curve-fitting, capacitor-lab-basics, color-vision. Those sims take an HSlider and rotate it -90 degrees. The tick mark features are unusable, and the sim must decorate the slider with ticks using a combination of hacks (e.g. see GRAPHING_QUADRATICS/CoefficientSlider).

Ideally, we should do something like we did with LayoutBox, HBox and VBox. That is, have base class Slider, with convenience subclasses HSlider and VSlider.

Assigning to @ariel-phet to prioritize.

[pixelzoom]

CAPACITOR_LAB_BASICS/BatteryNode uses another type of hack. It creates the tick labels rotated 90 degrees, uses the tick features of HSlider, then rotates the HSlider -90 degrees.

[pixelzoom]

I converted GRAPHING_QUADRATICS/CoefficientSlider to use the approach used in CAPACITOR_LAB_BASICS/BatteryNode. It seems like this would be relatively easy to support in a Slider, HSilder, VSlider hierarchy, like I described in #380 (comment).

The development steps would be something like:

• move HSlider to Slider, add orientation option, rotate slider and tick labels when orientation === 'vertical'
• create HSlider convenience type that sets orientation: 'horizontal'
• create VSlider convenience type that sets orientation: 'vertical'

And there may be addition things required to address phet-io and a11y dependencies on "HSlider" name.

[pixelzoom]

8/23/18 dev meeting:

Assigned to me.

For vertical sliders, want options for whether labels are on left vs right, and whether the tick extends out on both left and right sides.

a11y: Aria orientation option in AccessibleSlider. Make sure arrow keys work.

[pixelzoom]

Conclusions from Slack discussion (expand Details below):

• OK to rename things with prefix "HSlider" to prefix "Slider" (HSlider, HSliderIO, HSliderThumb, HSliderTrack, HSliderTrackIO)
• orientation does not need to be public to PhET-iO
• set ariaOrientation and propagate to AccessibleSlider via initializeAccessibleSlider

Chris Malley [1:23 PM]
I’m working on #380, adding support for vertical sliders. As part of this work, HSlider will be renamed to Slider, and HSlider and VSlider will be convenience subtypes of Slider (similar to how HBox and VBox relate to LayoutBox). Other types prefixed with “HSlider” will be renamed to have “Slider” prefix — ie, HSliderIO, HSliderThumb, HSliderTrack, and HSliderTrackIO will become SlideIO, SliderThumb, SliderTrack and SliderTrackIO respectively. Does that sound reasonable, or will this cause PhET-iO problems? (edited)

I could also consider leaving HSliderTrack and HSliderThumb named as is, since they draw track and thumb respectively in horizontal orientation.
Slider will continue to construct a slider in horizontal orientation by default, and rotate itself and tick labels if orientation: 'vertical' is specified.

Sam Reid [1:26 PM]
That sounds reasonable to me, but would be good to check with Michael too

Michael Kauzmann [1:29 PM]
I don't see a reason that the orientation will need to be public to PhET-iO, so I think that we can get away safely with SliderIO. There are considerations for a11y though, albeit minor ones. Perhaps we can get away with just having VSlider pass in the proper ariaOrientation to AccessibleSlider.js

Chris Malley [1:44 PM]
Setting ariaOrientation should not be the responsibility of the convenience classes. Slider is not abstract (it can be instantiated), so Slider needs to pass the correct ariaOrientation to AccessibleSlider based on options.orientation.

Michael Kauzmann [1:45 PM]
sounds good to me.

Chris Malley [1:45 PM]
What are the values for ariaOrientation?
Hmmm… I see AccessibleSlider.mixInto( Slider ); How does Slider pass anything into AccessibleSlider? Should AccessibleSlider set ariaOrientation based on this.orientation // public (read-only) of Slider?
Or this.orientation // public (read-only, a11y)

Michael Kauzmann [1:48 PM]
see initializeAccessibleSlider
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-orientation_attribute should be the same values as this.orientation (edited)

Chris Malley [1:49 PM]
I see. So options are propagated to AccessibleSlider. Can’t AccessibleSlider then look at options.orientation?

Michael Kauzmann [1:49 PM]
sounds good to me.

Chris Malley [1:50 PM]
Anything else related to a11y that needs to be added/changed?

Michael Kauzmann [1:51 PM]
The most robust way would probably be to remove the ariaOrientation option and instead just use the "orientation" option. But there may be other usages.

Chris Malley [1:52 PM]
Remove from where?

Michael Kauzmann [1:53 PM]
from AccessibleSlider's options, because the option should just be "orientation" to match Slider's. Then internally it can set the ariaOrientation to be the same as the AccessilbeSlider's options.orientation.

Chris Malley [1:55 PM]
Yes, agreed, it should not be in AccessibleSlider’s options. There should be an assertion that it is not in options, followed by:
assert && assert( options.ariaOrientation == undefined, ‘…’ );
assert && assert( options.orientation !== undefined, ‘…’ );
options.ariaOrientation == options.orientation; (edited)

Michael Kauzmann [1:56 PM]
Sounds very nice. Thank you for doing this.

Anything else related to a11y that needs to be added/changed?

I don't think so, I would just assign over to @jessegreenberg, who wrote most of it, to review when complete.

Chris Malley [1:57 PM]
OK, thanks for the help.

[pixelzoom]

Changes in the above commits:

• renamed Slider to HSlider
• added orientation option to Slider
• added HSlider and VSlider convenience types
• renamed HSliderIO, HSliderThumb, HSliderTrack, HSliderTrackIO to have "Slider" prefix
• addressed ariaOrientation for AccessibleSlider
• used VSlider in graphing-quadratics
• added VSlider to sun demo

[samreid]

Normally WebStorm can track history across file renames. However for HSlider.js => Slider.js, it is failing to follow the history:

This is confusing because git log --follow does seem to follow the history. Perhaps --follow works based on contents?

~/github/sun/js$ git log --follow Slider.js
commit fd6daf25e543b73f9a19dce2f53e3bf927b8b476 (HEAD -> master, origin/master, origin/HEAD)
Author: Chris Malley <cmalley@pixelzoom.com>
Date:   Fri Aug 31 15:37:08 2018 -0600

    Slider, HSlider, VSlider hierarchy, https://github.com/phetsims/sun/issues/380
    
    Signed-off-by: Chris Malley <cmalley@pixelzoom.com>

commit e69d4bf1a8d8bcb822e9a47e6b5bbb1fd585d9a0
Author: samreid <reids@colorado.edu>
Date:   Fri Jun 8 12:51:42 2018 -0600

    Replaced '// phet-io modules' with '// iphetio', see https://github.com/phetsims/phet-io/issues/1331

commit 359470fff422ee7efecbefc6d8313150dae1a741
Author: denz1994 <denz1994@gmail.com>
Date:   Tue May 22 09:27:33 2018 -0400

    HSlider labels made so they aren't pickable. Found during https://github.com/phetsims/masses-and-springs/issues/274.

...

I typically find it most useful to be able to use the WebStorm history features when looking back in time, I'm not sure what is different in this case, but I thought I should point it out as soon as possible.

[samreid]

In the GitHub diff page showing the diff, it is treating Slider.js as a new file (if I'm reading this correctly):

[pixelzoom]

I'm not sure what happened here. I used WebStorm to rename the file. And for any file that was renamed, I noted it's original name in the JSdoc at the top of the file. What do you suggest?

[samreid]

It looks like something went wrong with the rename. Perhaps WebStorm had a git failure during that step. Can that step be re-done so we can easily access the history? Easy access to the history for this file seems particularly important because it crosses a11y and phet-io and is a widely used component.

[pixelzoom]

There was no git failure indicated by WebStorm. Perhaps it got confused because the file was renamed, and then a new file named HSlider was created. I have no idea how to this can be "re-done".

[samreid]

Basically, you could try:

1. Revert head to the SHA before your changes
2. Commit the renaming of HSlider.js => Slider.js as one commit (don't push yet)
3. Commit your other changes
4. Push all commits

EDIT: changed would => could
EDIT: changed could => could try

[pixelzoom]

#380 (comment) says:

For vertical sliders, want options for whether labels are on left vs right, and whether the tick extends out on both left and right sides.

After completing the initial work (see #380 (comment)), I noticed that all sims with vertical sliders (see screenshots in #385 (comment)) have their labels on the left, and tick marks that point only to the left. That is consistent with the current implementation. So I'm going to propose that we don't make any additional enhancements related to tick marks until they are actually needed. @ariel-phet Since you specified the above requirements, does this sound reasonable to defer?

[pixelzoom]

The issue of restoring history for Slider.js was moved to #389.

[pixelzoom]

To restore history for some files that we renamed, some commits were reverted (see above) and then reapplied (see #389).

[pixelzoom]

@ariel-phet Assuming that deferring enhancements is acceptable (see #380 (comment)), please assign someone to review this work.

[ariel-phet]

@pixelzoom deferring enhancements is fine (if enhancements are needed, they are likely to come in graphing quadratics anyhow, but we can wait to see if that is actually necessary/desirable)

@zepumph I feel like you are in a good position for this review, being very familiar with a few vertical sliders.

[pixelzoom]

@zepumph This issue got a little confused with file renaming and restoring git history (addressed in #389). So let me attempt to clarify and point you to what needs to be reviewed.

The majority of the commits are related to renaming for files to have "Slider" prefix instead of "HSlider" prefix. Feel free to skim these, but they are pretty straightforward.

The primary change to Slider.js is shown in 81a0a69#diff-19fdbe803307bf2e93fb8ff42915ff18. In that commit, orientation: 'vertical' results in the slider being rotated -Math.PI/2, and tick labels rotated in the opposite direction (+Math.PI/2).

Finally, HSlider and VSlider were created as convenient types. Reviewing those in master HEAD should be sufficient.

Let me know if you have any questions.

[zepumph]

@pixelzoom everything you have done above looks great.

I tested usages and adjusted the rotation in my working copy to see how it was working.

I saw that for a11y documentation, there was still an HSlider.md, so I renamed it and passed it off for review in phetsims/binder#22

For phet-io, I wondered if the orientation of the slider is something that the phet-io data stream or api should have. Currently we don't have a need for Nodes in the state, so I think this is fine for now.

I also looked around at HSlider and VSlider usages, and it looks very good. Thanks @pixelzoom.

[pixelzoom]

Thanks for the review @zepumph.

@zepumph said:

For phet-io, I wondered if the orientation of the slider is something that the phet-io data stream or api should have. Currently we don't have a need for Nodes in the state, so I think this is fine for now.

In the Slack details in #380 (comment), @zepumph said:

I don't see a reason that the orientation will need to be public to PhET-iO, so I think that we can get away safely with SliderIO.

So I didn't make any PhET-iO related changes. If we decide it's needed in the future, it seems easy enough to add.

Closing.