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

Handle basic touch interactions #675

Merged
merged 6 commits into from
Mar 20, 2017
Merged

Handle basic touch interactions #675

merged 6 commits into from
Mar 20, 2017

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Mar 8, 2017

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.

manthey added 2 commits March 8, 2017 09:08
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.
@manthey
Copy link
Contributor Author

manthey commented Mar 8, 2017

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-io
Copy link

codecov-io commented Mar 8, 2017

Codecov Report

Merging #675 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            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
Impacted Files Coverage Δ
src/action.js 100% <ø> (ø) ⬆️
src/mapInteractor.js 96.12% <100%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27814ce...e317bf9. Read the comment docs.

@manthey
Copy link
Contributor Author

manthey commented Mar 8, 2017

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.

@aashish24
Copy link
Member

@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,
Copy link
Member

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)?

Copy link
Contributor Author

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,
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

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)) {
Copy link
Member

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]});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does pointers do?

Copy link
Contributor Author

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.

@aashish24
Copy link
Member

Mostly looks reasonable to me.. will try on my touch device as well.

@aashish24
Copy link
Member

@manthey I guess we can pop up widgets on touch events? (for e.g. when we select a path or item)

@manthey
Copy link
Contributor Author

manthey commented Mar 15, 2017

We still handle push touches as mouse clicks (not as special touch events). So, showing widgets or selecting something on a click will work.

@aashish24
Copy link
Member

Cool, thanks @manthey

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +2

@manthey manthey merged commit 8501fb8 into master Mar 20, 2017
@manthey manthey deleted the touch-interactions branch March 20, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants