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

add ability to shift pointer areas for buttons #500

Closed
pixelzoom opened this issue Apr 16, 2019 · 13 comments
Closed

add ability to shift pointer areas for buttons #500

pixelzoom opened this issue Apr 16, 2019 · 13 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 16, 2019

In phetsims/unit-rates#212, we identified a need to shift the pointer area for a RectangularButtonView. In that case, it's the ExpandCollapseButton for AccordionBox.

If we need arbitrary shifting, we could add this to RectangularButtonView:

touchAreaXShift: 0,
touchAreaYShift: 0,
mouseAreaXShift: 0,
mouseAreaYShift: 0,

and the code that sets the pointer areas becomes:

// set pointer areas
this.touchArea = button.localBounds.dilatedXY( options.touchAreaXDilation, options.touchAreaYDilation ).shifted( options.touchAreaXShift,  options.touchAreaYShift );
this.mouseArea = button.localBounds.dilatedXY( options.mouseAreaXDilation, options.mouseAreaYDilation ).shifted( options.mouseAreaXShift,  options.mouseAreaYShift );

If we don't need arbitrary shifting and want to keep the pointer areas aligned with the button, then the API might be:

touchAreaXAlign: 'center', // {string} left, right, center
touchAreaYAlign: 'center', // {string} top, bottom, center

and the code to set the pointer areas would be significantly more complicated.

And depending on what API we choose, we should probably add something similar to RoundButtonView.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 16, 2019

Summary of @zepumph suggestion on Slack:

touchAreaShift: {Vector2}
mouseAreaShift: {Vector2}

The current API uses {number} for touchAreaXDilation et. al. So while I appreciate the fact that using Vector2 requires fewer options (2 vs 4), it's not in the spirit of the existing API (which would be costly to change) and requires allocation of a Vector2.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 16, 2019

Since I can easily imagine a designer asking to "expand the touch area a bit below and more above" (which is what we'd probably want to do in phetsims/unit-rates#212), I think that the touchAreaXShift et. al. API is probably the way to go. The downside is that the shift values would be dependent on the dilation values. E.g.

new ExpandCollapseButton( ..., {
  touchAreaXDilation: 10,
  touchAreaYDilation: 9,
  touchAreaYShift: -3 // to have 6 below and 12 above
} );

@jonathanolson
Copy link
Contributor

A generalization (that wouldn't include as many options) would presumably be a function that returns the mouse/touch area. It could take in the as-of-yet-constructed node, e.g.:

new ExpandCollapseButton( ..., {
  touchAreaGenerator( node ) {
    return node.localBounds.dilatedXY( 10, 9 ).shifted( -3, -3 );
  }
} );

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 24, 2019

@jonathanolson's suggestion above is indeed very general. But...

  • 4 existing RectangularButtonView options will become obsolete: touchAreaXDilation, touchAreaYDilation, mouseAreaXDilation, mouseAreaXDilation. Keeping those options and doing something sensible with them will complicate the implementation. Tracking them down in client code and replacing them will be expensive.

  • The most common case (symmetric dilation) will be more complicated for the client to specify, e.g.:

new RectangularPushButton( ..., {
  touchAreaXDilation: 10, 
  touchAreaYDilation: 5
  }
} );

... would become:

new RectangularPushButton( ..., {
  touchAreaGenerator( node ) {
    return node.localBounds.dilatedXY( 10, 5 );
  }
} );

@samreid
Copy link
Member

samreid commented Apr 24, 2019

Would it read any clearer with arrows?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 24, 2019

Do you mean arrow functions? Still seems heavy for the typical case:

new RectangularPushButton( ..., {
  touchAreaGenerator: node => node.localBounds.dilatedXY( 10, 5 )
} );

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 24, 2019

Occurrences of the RectangularButtonView options that would be obsoleted by @jonathanolson's proposal. Unlikely that all of these are for RectangularButtonView, but all of them would need to be examined.

touchAreaXDilation 161
touchAreaYDilation 153
mouseAreaXDilation 44
mouseAreaYDilation 35

And since we'd presumably want to do something similar for RoundButtonView:

touchAreaDilation 37
mouseAreaDilation 3

@samreid
Copy link
Member

samreid commented Apr 25, 2019

I'm not sure how to balance the flexibility of 1 function vs the ease of 4 variables which are often all we need. I'd like to discuss further in the dev meeting.

@pixelzoom
Copy link
Contributor Author

4/25/19 dev meeting:

I'll proceed with new options for RectangularButtonView and RoundButtonView. We can change in the future if needed. The options are:

touchAreaXShift: 0,
touchAreaYShift: 0,
mouseAreaXShift: 0,
mouseAreaYShift: 0,

@pixelzoom
Copy link
Contributor Author

This is blocked until we resolve #502.

@pixelzoom
Copy link
Contributor Author

Done for round and rectangular push buttons. Demonstrated in sun demo application, see annotated screenshot below.

@ariel-phet please assign someone to review.

screenshot_1174

@ariel-phet
Copy link

@chrisklus can you review?

@chrisklus
Copy link
Contributor

@pixelzoom looks good. The code changes make sense and I tested the shifted pointer areas with a mouse and simulated touch device. Back to you.

@chrisklus chrisklus assigned pixelzoom and unassigned chrisklus May 8, 2019
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

5 participants