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

Fixed #2784 #5274

Merged
merged 3 commits into from
Oct 2, 2017
Merged

Fixed #2784 #5274

merged 3 commits into from
Oct 2, 2017

Conversation

aparajita
Copy link
Contributor

Previously, a zoom initiated by a double tap on a touchscreen device would stop as soon as the user lifted his/her finger from the second tap. This was due to the map's mousedown handler calling map.stop().

This PR sets a flag when the user initiates a double click/tap, which is then checked before calling map.stop() in the map's mousedown handler.

mousedown events are still received and passed to any handlers during a user-initiated zoom animation, but they will not stop the zoom.

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

Hi @aparajita thank you very much for your contribution!

I have verified that this fix works on my mobile device. In addition to my comment, do you have time to add a test that would catch this bug?

@@ -46,7 +46,10 @@ module.exports = function bindHandlers(map: Map, options: {}) {
}

function onMouseDown(e: MouseEvent) {
map.stop();
if (!map._isUserDoubleClick) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of adding this property to the map object, you could create a DoubleClickZoomHandler#isActive method as we do in with some of the other handles (ex. https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/handler/box_zoom.js#L52), so you could call map.doubleClickZoom.isActive() for this conditional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll switch to using isActive. As for the test, there aren't any existing tests for the DoubleClickZoomHandler, and I'm kind of a little too busy to figure out how to write one to catch this specific bug. Better someone who knows the test framework well should do it.

@mollymerp
Copy link
Contributor

Thanks for the update @aparajita. I started writing a test but it turns out that the TouchEvent constructor is not implemented in JSDOM, so I'm not sure if writing a meaningful test is possible at this time.

@jfirebaugh do you have other ideas for testing this?

src/ui/map.js Outdated
@@ -244,6 +244,7 @@ class Map extends Camera {
_refreshExpiredTiles: boolean;
_hash: Hash;
_delegatedListeners: any;
_isUserDoubleClick: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this flag anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mollymerp
Copy link
Contributor

@aparajita once you remove the unneeded flag on the map object, I will merge this as is and open a follow-up ticket on testing.

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

thank you @aparajita !

@mollymerp mollymerp merged commit dc7dbc3 into mapbox:master Oct 2, 2017
Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

thank you @aparajita !

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.

2 participants