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

fix pinching while touching markers #9683

Merged
merged 2 commits into from
May 14, 2020
Merged

Conversation

ansis
Copy link
Contributor

@ansis ansis commented May 13, 2020

fix #9675

When touches touch the markers instead of the canvas they have different targets. Gestures were using touchEvent.targetTouches as the list of relevant touches. This broke multi-finger gestures that touch markers.

The fix switches to touchEvent.touches and filters out touches who's target is not a child element of the map's canvas container.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix multi-finger gestures that touch markers.</changelog>

ansis added 2 commits May 12, 2020 16:37
Previously we were using e.targetTouches as the list of touches relevant
to the map. It contains only the touches that first touched the same
target. Touches that hit a marker have a different target.

This fixes uses e.touches and filters out touches who's target is not a
child of the map.

fix #9675
@ansis ansis added the bug 🐞 label May 13, 2020
@ansis ansis requested a review from karimnaaji May 13, 2020 16:11
@karimnaaji karimnaaji mentioned this pull request May 13, 2020
7 tasks
@karimnaaji
Copy link
Contributor

Do you think it would be useful to create a page with a high density of randomly placed markers, and add it as part of our release testing here? (making sure we would inevitably touch one while doing a gesture with a density similar to where the issue was shown https://nomadlist.com/map).

We already have markers in the list of pages but I have the feeling it would be harder to test that fix from there.

@ansis
Copy link
Contributor Author

ansis commented May 14, 2020

Do you think it would be useful to create a page with a high density of randomly placed markers, and add it as part of our release testing here?

Yep. The existing marker page might work if you intentionally try to test for this

Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

Sounds good!

@karimnaaji karimnaaji merged commit 8ff1303 into master May 14, 2020
@karimnaaji karimnaaji deleted the fix-touch-target-9675 branch May 14, 2020 17:21
karimnaaji pushed a commit that referenced this pull request May 14, 2020
* fix multi-finger gestures touching markers

Previously we were using e.targetTouches as the list of touches relevant
to the map. It contains only the touches that first touched the same
target. Touches that hit a marker have a different target.

This fixes uses e.touches and filters out touches who's target is not a
child of the map.

fix #9675

* update tests and add regression test
karimnaaji pushed a commit that referenced this pull request May 14, 2020
* fix multi-finger gestures touching markers

Previously we were using e.targetTouches as the list of touches relevant
to the map. It contains only the touches that first touched the same
target. Touches that hit a marker have a different target.

This fixes uses e.touches and filters out touches who's target is not a
child of the map.

fix #9675

* update tests and add regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pinch-to-zoom broken when first touch is on a marker on touch devices
2 participants