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

Disable drag rotate if touch detected #3457

Closed
wants to merge 1 commit into from

Conversation

ChrisBellew
Copy link

@ChrisBellew ChrisBellew commented Oct 25, 2016

A long press touch is interpreted by the browser as a right click,
which causes the drag rotate to start a rotation. Subsequent touches
perform the rotation, which causes the map to erratically rotate
around. If a touch start event is detected, just disable the feature.

As recommended by @bhousel in #2204

A long press touch is interpreted by the browser as a right click,
which causes the drag rotate to start a rotation. Subsequent touches
perform the rotation, which causes the map to erratically rotate
around. If a touch start event is detected, just disable the feature.
this._enabled = false;
}

_onTouchStart() {
this.disable();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we ever need to re-enable drag rotate after a touchstart event?

Copy link

Choose a reason for hiding this comment

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

Yes, on devices that have both touch screen and mouse (e.g Windows Surface Book and almost all modern Windows 10 laptops), drag rotate should be enabled when mouse interaction starts.

This can be tested on desktop chrome using the following steps:

  1. Use toggle device toolbar in desktop Chrome and switch to "Nexus 6P". Long press.
  2. In the same debugging session, tap on toggle device toolbar to switch back to regular desktop chrome. Dragpan rotate should work (at least after first mouse click instead of being permanently disabled).

@lucaswoj
Copy link
Contributor

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page

@ChrisBellew
Copy link
Author

ChrisBellew commented Oct 31, 2016

My opinion is no. There is no meaningful way to interact with it in its
current form, especially since it causes such erratic behavior after it is
activated. Tapping feels like selection/panning/movement, pinching like
zooming and long press for more information/options. Can't think of another
way besides having a separate menu?

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 1, 2016

I see. We're using the existence of the touchstart event as an indication that we're on a touchscreen device and therefore disable any handlers that are mouse-specific. That makes sense to me 👍.

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 1, 2016

Some devices may have both touch events AND mouse events (i.e. the Chromebook Pixel). We can support those devices seamlessly if we enable the DragRotateHandler when a touchend event is encountered.

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 1, 2016

This may necessitate using a mechanism other than enable / disable so that we don't override user settings for this interaction handler. A private _isTouchInteraction property should work.

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 2, 2016

Launch Checklist

  • re-enable DragRotateHandler after a touchend event
  • ensure user calls to DragRotateHandler#enable / #disable do not interfere with this PR's behaviour

@jfirebaugh
Copy link
Contributor

Hey @ChrisBellew -- I'm in the process of cleaning out our PR queue and it looks like there hasn't been any activity on this one recently. If you're still interested in coming back to it, please let us know and we'll be happy to help you get it to completion. Until then, I'm going to close it out. Thanks again for your contributions!

@jfirebaugh jfirebaugh closed this Jun 28, 2017
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.

4 participants