WavelengthSlider problems with pointerAreas

[pixelzoom]

(1) There are no options for setting touchArea and mouseArea for the tweaker (arrow) buttons.

(2) There are no options for setting touchArea and mouseArea for the slider thumb.

(3) There is a very odd options.pointerAreasOverTrack that is undocumented and has this behavior. When pointerAreasOverTrack:false, the slider thumb's touchArea and mouseArea is completely below the track, e.g.:

When pointerAreasOverTrack:true, the touchArea and mouseArea for the thumb overlap the track, e.g:

Option pointerAreasOverTrack was added by @aaronsamuel137 in @9aae2c89441849b3ef3b5d3c7c4f0c05d2a4dfa7. bending-light and color-vision are the current clients. Unclear why it was added, or whether it's really needed. It certainly interferes with the "click in track" feature.

[pixelzoom]

I was the author of the Java version of color-vision, and there was never a need for the pointerAreasOverTrack feature. @samreid Since you inherited bending-light, could you comment on this feature? (Issue (3) above.)

[pixelzoom]

Ah... There is some ability to change thumb touchArea. But it's done before the options extend call, and is not documented in the options hash:

49 var thumbTouchAreaXDilation = options.thumbTouchAreaXDilation || 0.5 * thumbWidth;
50 var thumbTouchAreaYDilation = options.thumbTouchAreaYDilation || 0.5 * thumbHeight;

[pixelzoom]

(4) Buggy vertical touchArea computation.

This line:

50 var thumbTouchAreaYDilation = options.thumbTouchAreaYDilation || 0.5 * thumbHeight;

... is probably a bug, and should be 0.25 * thumbHeight. The y dilation is shifted, so this actually results in a huge touch area (the full height of the thumb) below the thumb. E.g. in scenery-phet demo:

[pixelzoom]

(5) touchArea shape is computed using Shape.rect instead of using localBounds, dilate and shift functions. This results in an unnecessarily complicated bit of code:

    // compute mouse/touch areas, extend up to top of track if pointerAreasOverTrack is true
    var bounds = shape.bounds.copy();
    if ( pointerAreasOverTrack ) {
      this.touchArea = Shape.rectangle( bounds.minX - touchAreaXDilation, bounds.minY - trackHeight, bounds.width + 2 * touchAreaXDilation, bounds.height + 2 * touchAreaYDilation + trackHeight );
      this.mouseArea = Shape.rectangle( bounds.minX, bounds.minY - trackHeight, bounds.width, bounds.height + trackHeight );
    }

    // don't extend above the thumb so that we don't encroach on slider track if pointerAreasOverTrack is false
    else {
      this.touchArea = Shape.rectangle( bounds.minX - touchAreaXDilation, bounds.minY, bounds.width + 2 * touchAreaXDilation, bounds.height + 2 * touchAreaYDilation );
    }

[pixelzoom]

(6) Computation of pointer areas is done inside of WavelengthSlider.Thumb. It would be much easier (see 5) to do this outside of Thumb, in the WavelengthSlider constructor.

(7) No way to turn off touchArea dilation for the thumb.

[samreid]

I was the author of the Java version of color-vision, and there was never a need for the pointerAreasOverTrack feature. @samreid Since you inherited bending-light, could you comment on this feature? (Issue (3) above.)

I don't recall any specific requests for this feature from the design team, it seems like pointerAreasOverTrack: true was inherited by copy-paste from an existing WavelengthSlider example.

[pixelzoom]

@ariel-phet requested the pointerAreasOverTrack feature for color-vision in phetsims/color-vision#63, due to a usability concern raised in interviews.

I agree with the usability concern. But I don't think this was addressed in a particularly good way. My proposal:

Instead of extending the thumb's pointer areas into the track, the track should support "click and drag" (consistent with HSlider). This should be the default behavior, and pointerAreasOverTrack option should be removed.

@ariel-phet @samreid Do you see any problem with this? Or may I proceed?

[samreid]

It sounds reasonable to me, but let's hear from @ariel-phet before proceeding.

[ariel-phet]

@pixelzoom agreed, please proceed

[pixelzoom]

Here's the check list of what needs to be done:

• make it possible to set touchArea dilation to 0 for thumb
• add options for mouseArea dilation of thumb
• fix computation of pointer areas for thumb so that areas are not so tall
• add options for touchArea and mouseArea dilation of tweaker buttons
• change computation of pointer area shapes to use localBounds, dilatedXY, shift
• set thumb pointer areas outside of Thumb constructor
• clean up constructor for Thumb inner type
• add "click and drag" feature to track, ala HSlider
• delete pointerAreasOverTrack option
• rename param wavelength to wavelengthProperty, since it's a Property
• test color-vision and bending-light
• fix memory leak in ValueDisplay inner type, it needs to unlink from valueProperty

[pixelzoom]

Some of the things in inner type Thumb that IDEA was complaining about were due to incorrect documentation of parameters in kite.Shape. See phetsims/kite#62.

[pixelzoom]

Because ArrowButton has default no-zero pointer area dilation, I was forced to compute touchArea and mouseArea for the tweaker buttons even if their dilation option values were zero. This demonstrates the problem with having non-zero pointer area defaults in common components -- it interferes with composition, and forces us to compute pointer areas that we wouldn't otherwise need.

[samreid]

It looks like you have been making swift progress here, so perhaps I should have asked sooner: what is your opinion of generalizing HSlider so it can support WavelengthSlider?

[pixelzoom]

WavelengthSlider is a pretty specialized control, and sufficiently different that I don't think generalizing (or extending) HSlider would work out. And it's going to get even more different when I tackle #211 (add support for UV and IR wavelengths).

[pixelzoom]

In light of phetsims/sun#251 (comment), options for pointer areas should be changed to pixels instead of percentages. Review clients to see if this adversely affects them.

[pixelzoom]

Changing pointer area options from percentages to pixels moved to #266.

[pixelzoom]

@samreid Would you mind reviewing this? I know it's a lot of stuff. The only really critical issue is to confirm that WavelengthSlider now behaves like HSlider; that is, you can click and drag in the track.

[samreid]

I tested the functionality on Mac Chrome with the WavelengthSlider in Bending Light screen 3 and it seemed good with respect to clicking and dragging in the track.

I noticed the require statements were alphabetized, do you have an automated tool for this? @andrewadare may want to hear about it too, if it exists.

I skimmed the change set and did not see any concerns.

[pixelzoom]

@andrewadare @samreid Sorry to report that sorting require statements is currently a manual process. I tend to enter them sorted to begin with, so don't have to do manual sort very often.