Slider problems with vertical orientation

[pixelzoom]

Inspired by phetsims/fourier-making-waves#175 ...

Slider (the base class for sliders) is implemented for a horizontal slider. You can create a vertical slider using either VSlider, or Slider with orientation: 'vertical'. But when creating a vertical slider, there are a bunch of dimensions that need to be swapped. VSlider does this for you:

    if ( options.trackSize !== undefined ) {
      options.trackSize = options.trackSize.swapped();
    }
    if ( options.thumbSize !== undefined ) {
      options.thumbSize = options.thumbSize.swapped();
    }
    swapObjectKeys( options, 'thumbTouchAreaXDilation', 'thumbTouchAreaYDilation' );
    swapObjectKeys( options, 'thumbMouseAreaXDilation', 'thumbMouseAreaYDilation' );

But what if you need to create a subclass of Slider that support horizontal and vertical orientations? Then you're in trrouble. The above code should be in Slider, not in VSlider. It should be invoked when options.orientation === Orientation.VERTICAL.

[pixelzoom]

In the above commits, I moved the dimension-swapping code from VSlider to Slider.

The only sim affected by this was Fourier; there were no other cases where Slider was being instantiated with orientation: Orientation.VERTICAL, all other vertical sliders are created using VSlider. In Fourier, this allowed me to specify pointer areas "normally" for a vertical slider.

But there is still an issue that is not addressed. In Fourier, I'm using the thumbNode and trackNode options to provide a custom thumb and track (see AmplitudeSlider.js). For orientation: Orientation.VERTICAL, Slider indiscriminately rotates all of its "part" by 45 degrees:

    const sliderPartsNode = new Node( { children: sliderParts } );
    if ( options.orientation === Orientation.VERTICAL ) {
      sliderPartsNode.rotation = VERTICAL_ROTATION;
    }

So in Forueir, I have to create my custom thumb and track in the horizontal orientation, and then Slider rotates them. This makes for some confusing client code, like this:

    // Here we specify our thumb and track dimensions in the vertical orientation, then swap them.
    // This is because Slider rotates the thumb and track -90 degrees when orientation is vertical.
    // So we'll create our custom thumb and track in horizontal orientation, and Slider will rotate
    // them into vertical orientation. Pretty gross, eh?
    const thumbSize = new Dimension2( THUMB_WIDTH, THUMB_HEIGHT ).swapped();
    const trackSize = new Dimension2( TRACK_WIDTH, options.trackHeight ).swapped();

Ideally, Slider should only rotate the parts that it has created. I'll work on that next.

[pixelzoom]

Ideally, Slider should only rotate the parts that it has created. I'll work on that next.

I couldn't untangle this. The best I could do is clean things up in Fourier, so that there's less code that has to be aware of the fact that Slider rotates things. I'm still working on that in phetsims/fourier-making-waves#175.

@jonathanolson could you please review what I've done here so far? (... which is just https://github.com/phetsims/sun/commit/53706aa5fef79d0805840dbd7e4a04b3a555ef9e) I'm inclined to stop here for now, since this is going to take a lot of time, and Fourier is the only sim currently using a vertical slider with custom thumb and track. Let me know if you have other ideas.

[jonathanolson]

Change looks great to me, and seems like a fine place to stop at. I'll leave open but unassigned, thanks!

[pixelzoom]

As I discovered in https://github.com/phetsims/fourier-making-waves/issues/175, trying to create your vertical slider with custom thumb and track has lots of problems. It was necessary to create my thumb and track (and update the track, in my case) in the horizontal orientation, which makes for some ugly code. And while https://github.com/phetsims/sun/commit/53706aa5fef79d0805840dbd7e4a04b3a555ef9e moved the responsibility for swapping pointer-area dimensions into Slider, those options cannot apparently be used with a custom thumb. So I had to resort to handling my own pointer areas, and swapping their dimensions.

I've removed the "high priority" label because it's no longer blocking Fourier, and @jonathanolson has reviewed my changes. But this should definitely be cleaned up "soon". It shouldn't be this difficult to customize a vertical slider.

And note that whoever works on this will need to take a close look at all existing vertical sliders, including Fourier's AmplitudeSlider.

[chrisklus]

This change broke any VSlider instances that don't provide options.trackSize or options.thumbSize, see https://github.com/phetsims/energy-forms-and-changes/issues/414.

Since those options values are now swapped in Slider instead of VSlider, the default values (specified in the horizontal orientation) are swapped along with any provided options (specified in vertical orientation).

Sounds like something like this was expected to happen based on comments in https://github.com/phetsims/sun/issues/717#issuecomment-924371910. Perhaps this elevates priority of the remaining work for this issue (though it is not blocking me, I was just doing maintenance work on EFAC for https://github.com/phetsims/chipper/issues/1177). I haven't tried to find how may sims are affected by this issue because i couldn't think of a relatively easy way to do so.