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

touchArea API for HSlider #202

Closed
pixelzoom opened this issue Oct 8, 2015 · 15 comments
Closed

touchArea API for HSlider #202

pixelzoom opened this issue Oct 8, 2015 · 15 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

From 10/8/15 dev meeting notes:

JO: Standard pattern for Mouse/Touch area customization for controls? (See phetsims/curve-fitting#72)

How to set touchArea for private subcomponents of a UI component is a general problem. We discussed this in the context of HSlider, where @jonathanolson has a need to change the thumb's touchArea.

We considered making the thumbNode public, but that's a slippery slope.

The consensus was to
• create dot.Insets
• add options.thumbTouchAreaInsets (or similar name)
• apply options.thumbTouchAreaInsets to the default thumb only; if a custom thumb is provided, the client is responsible for setting the touchArea, and this option is ignored.

@jonathanolson
Copy link
Contributor

Should we create the mouseArea part too?

@pixelzoom
Copy link
Contributor Author

I can't recall ever having to increase mouseArea for a slider thumb.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 30, 2016

Following conventions used elsewhere in sun, the options should be:

thumbTouchAreaXDilation: 0,  // {number} horizontal dilation of thumb touchArea
thumbTouchAreaYDilation: 0,  // {number} vertical dilation of thumb touchArea
thumbMouseAreaXDilation: 0,  // {number} horizontal dilation of thumb mouseArea
thumbMouseAreaYDilation: 0,  // {number} vertical dilation of thumb mouseArea

But there is a slight problem with the touchArea. It currently defaults to some percentage of the thumb's dimensions. Here's the relevant code in HSlider:

114  thumbNode.touchArea = Shape.rectangle( ( -thumbNode.width / 2 ) - dx, ( -thumbNode.height / 2 ) - dy, thumbNode.width + dx + dx, thumbNode.height + dy + dy ); 

If we set the defaults to zero (or any specific value, for that matter), then this affect all clients of HSlider. So perhaps we should use 'null' as the default value, which then sets defaults based on thumb dimensions. E.g.:

option = _.extend( {
  //...
  thumbTouchAreaXDilation: null,  // {number|null} horizontal dilation of thumb touchArea
  thumbTouchAreaYDilation: null,  // {number|null} vertical dilation of thumb touchArea

}, options );

// set defaults based on thumbNode dimensions
if ( options.thumbTouchAreaXDilation === null ) {
  options.thumbTouchAreaXDilation = 0.5 * thumbNode.width;
}
if ( options.thumbTouchAreaYDilation === null ) {
  options.thumbTouchAreaYDilation = 0.25 * thumbNode.height;
}

@pixelzoom
Copy link
Contributor Author

We'll also have to decide whether these options apply only to the default thumb, or to a custom thumb provided via options.thumbNode.

@veillette
Copy link
Contributor

for the record, forces-and-motion-basics, masses-and-springs and molecules-and-light uses a custom thumbNode,

@pixelzoom
Copy link
Contributor Author

@veillette Do they use a custom thumbNode because they require a different "look" for the thumb, or as a workaround for changing the thumb's touchArea?

@veillette
Copy link
Contributor

Currently if options.thumbNode is non-null, then the touchArea is the responsibility of the client.
Most clients don't bother with touchArea though.

In any case, I didn't want to change the behavior,

option = _.extend( {
  //...
  thumbTouchAreaXDilation: null,  // {number|null} horizontal dilation of thumb touchArea
  thumbTouchAreaYDilation: null,  // {number|null} vertical dilation of thumb touchArea

}, options );


    // thumb touch area
    if ( !options.thumbNode ) {
      if ( !options.thumbTouchAreaXDilation ) {
        options.thumbTouchAreaXDilation = 0.5 * thumbNode.width;
      }
      if ( !options.thumbTouchAreaYDilation ) {
        options.thumbTouchAreaYDilation = 0.25 * thumbNode.height;
      }
      // touch area dilated along X and Y directions
      thumbNode.touchArea = thumbNode.bounds.dilatedXY( options.thumbTouchAreaXDilation, options.thumbTouchAreaYDilation );
    }

The commit is here c5dbf7c (I got the wrong issue number)

In this way there is no changes in behavior for all clients of HSlider, even with a custom thumbNode.

Assigning this commit to @pixelzoom for review.

@veillette
Copy link
Contributor

Sorry I never addressed your question.
Forces and motion basics and Molecules and light get a different thumb look.
Masses and springs is under construction. I don't see any reason why one would want to use a custom thumbNode for their case. it doesn't change the touchArea.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 1, 2016

I inspected the changes made by @veillette, they look OK, but I don't have time to regression test sims right now (trying to meet another deadline). @ariel-phet You may want to have someone test drive a few sims.

His change does enable @veillette to proceed with curve-fitting (see phetsims/curve-fitting#76). But I don't think the change is sufficient to close this issue. Work that still needs to be done:

(1) Implement similar option for mouseArea. (PhET decided to support both touchArea and mouseArea in all common code.)

(2) Apply touchArea and mouseArea uniformly to but the default and custom thumbNode (i.e., options.thumbNode).

@veillette
Copy link
Contributor

I see what you are hinting at.
One could create a

var thumbNode= new HSlider.ThumbNode( new Property( true ), {
        thumbSize: new Dimension2( 7.5, 15 ),
        thumbFillEnabled: '#00b3b3',
        thumbFillHighlighted: '#00e6e6'
      } )
thumbNode.touchArea= thumbNode.bounds.dilatedXY(5,10);

  var myHSlider = new HSlider( property, range, {
      thumbNode:  thumbNode
    } );

That never occured to me.
That would also work for the purpose of the curve-fitting.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 1, 2016

I wasn't hinting at that at all. HSlider.ThumbNode constructor is documented as @private.

@ariel-phet
Copy link

@veillette feel free to create a testing "task" for regression testing slider changes and assign to @phet-steele

@ariel-phet
Copy link

@pixelzoom this issue has been sitting fallow for awhile, it seems like there is some work left to do and some regression testing that would be good. Do you mind making a test task for @phet-steele since our QA resources are not too strained at the moment? And perhaps we can polish off this issue (not necessarily you) after this testing?

@ariel-phet ariel-phet removed their assignment Oct 26, 2016
@ariel-phet
Copy link

@pixelzoom sorry, looks like that testing was already done....do you feel like tackling the remaining work, if not, please assign back to me.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 31, 2016

This was already addressed via 4c3ea17 for #254. Both touchArea and mouseArea are supported, and the defaults are in pixels:

      thumbTouchAreaXDilation: 11,
      thumbTouchAreaYDilation: 11,
      thumbMouseAreaXDilation: 0,
      thumbMouseAreaYDilation: 0,

Closing.

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

4 participants