-
Notifications
You must be signed in to change notification settings - Fork 75
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
Handle basic touch interactions #675
Conversation
This uses hammerjs as an optional dependency. It is only loaded if the touch support is detected in the browser OR the alwaysTouch option is set. By default, it handles pan and rotate events, where rotate events include zoom (pinch) actions. If hammerjs is not available, touch actions are not supported. Actions are configured through our general action mechanism, so that they could require different gestures, though I suspect anything different will require some work. There is currently no touch support for selecting a rectangular region. Since 1-touch is pan and 2-touch is zoom/rotate, this would either require using 3-touch or some sort of tapping, I would think. This still needs tests.
This has been tested on an Samsung/Android phone and on a Windows touch-screen laptop in Chrome and Firefox. It needs to be tested on iOS devices (and anywhere else we want to support). |
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
==========================================
+ Coverage 91.9% 92.02% +0.12%
==========================================
Files 84 84
Lines 8749 8857 +108
==========================================
+ Hits 8041 8151 +110
+ Misses 708 706 -2
Continue to review full report at Codecov.
|
I just tried to this is IE 11 on a Windows laptop with a touch screen, and touch events are not handled (HammerJS's website doesn't handle them either). HammerJS says rotations don't work in IE 11, but should in Edge. |
@manthey starting the review...this is a cool feature.. |
/* The minimum angle of rotation (in radians) before the | ||
* geo_action.zoomrotate action will allow rotation. Set to 0 to always | ||
* include rotation. */ | ||
zoomrotateMinimumRotation: 5.0 * Math.PI / 180, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be zoomRotate* (R capital)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't camel cased other actions (zoomselect, for instance). Since this is related to the zoomrotate action, I thought not capitalizing the R made more sense (but I'm hardly convinced of this).
* geo_action.zoomrotate action will reverse the rotation direction. | ||
* This helps reduce chatter when zooms and pans are combined with | ||
* rotations. */ | ||
zoomrotateReverseRotation: 4.0 * Math.PI / 180, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering why 4.0 for reverse and 5.0 for other way round?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first value (5) is for initial rotation to prevent accidental rotation when you are trying to just zoom or just pan. The reverse value (4) is when you are rotating to debounce rotating back and forth -- it should be less than or equal to the initial rotation threshold. I tried a smaller number, but sometimes when panning and rotating, the map would jitter in rotation (because in a two-finger pan, the two touch points don't necessarily move in perfect synch). I settled on 4 as working. Feel free to play with these values and suggest other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the info. I will try playing with some other numbers if i get a chance.
src/mapInteractor.js
Outdated
Math.PI * 3) % (Math.PI * 2) - Math.PI; | ||
/* If we reverse direction, don't rotate until some threshold is | ||
* exceeded. This helps prevent rotation bouncing while panning. */ | ||
if (deltaTheta && (deltaTheta * (m_state.lastRotationDelta || 0) >= 0 || Math.abs(deltaTheta) >= m_options.zoomrotateReverseRotation)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we wrap these long lines?
interactor.simulateEvent( | ||
'panstart', {touch: true, center: {x: 20, y: 20}}); | ||
interactor.simulateEvent( | ||
'hammer.input', {touch: true, center: {x: 30, y: 20}, pointers: [1]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does pointers do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simulates the number of fingers down. A two-touch gesture where you then release one figure will result in a following one-touch gesture unless we stop it. The pointers is a list of those touches, but we don't care what is in them, just how many there are, so for the test it can be any list of the appropriate length.
If you don't do this, a two-touch zoom where you let up on one figure first will result in a pan which isn't desired.
Mostly looks reasonable to me.. will try on my touch device as well. |
@manthey I guess we can pop up widgets on touch events? (for e.g. when we select a path or item) |
We still handle push touches as mouse clicks (not as special touch events). So, showing widgets or selecting something on a click will work. |
Cool, thanks @manthey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +2
This uses hammerjs as an optional dependency. It is only loaded if the touch support is detected in the browser OR the alwaysTouch option is set. By default, it handles pan and rotate events, where rotate events include zoom (pinch) actions. If hammerjs is not available, touch actions are not supported.
Actions are configured through our general action mechanism, so that they could require different gestures, though I suspect anything different will require some work.
There is currently no touch support for selecting a rectangular region. Since 1-touch is pan and 2-touch is zoom/rotate/pan, this would either require using 3-touch or some sort of tapping, I would think. Alternately, we could turn off 1-touch pan and use that. We should probably support a two-click rectangle in annotations as well as a drag-rectangle. Any of those changes can be put in a different PR.