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

Add touch pan blocker to gesture handling for touch devices #11116

Merged
merged 26 commits into from Oct 19, 2021

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Oct 12, 2021

This PR implements the new map option gesture handling (see: PR #11029) for touch devices by requiring two fingers to touch pan. Touch actions affected by gesture handling is touch pan. Actions that involve tapping gestures (i.e. tap zoom) and actions that already require two fingers (e.g. pinch-to-zoom) are unaffected by implementing gesture handling.

Possible trouble areas: Stress test panning with two fingers to see if the alert flickers. Touch panning vertically with two fingers may occasionally trigger tilting or zooming map instead of panning. To address the issue with touch pitch (aka tilting the map) affecting touch panning with two fingers, touch pitch requires three fingers if gestureHandling is set to true.

To implement gesture handling to map set to true in the map constructor:

var map = window.map = new mapboxgl.Map({
    container: 'map',
    zoom: 12.5,
    center: [-77.01866, 38.888],
    style: 'mapbox://styles/mapbox/streets-v10',
    gestureHandling: true
});

Video demo:

RPReplay_Final1634082501.MOV

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • 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>Add touch device capability for gesture handling (require two fingers to touch pan)</changelog>

@avpeery avpeery linked an issue Oct 13, 2021 that may be closed by this pull request
@avpeery avpeery marked this pull request as draft October 13, 2021 04:39
@avpeery avpeery marked this pull request as ready for review October 14, 2021 19:58
Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

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

The feature itself looks good to me. 👏 My questions:

Is the behavior of wheel and one-finger touches independently controllable? Or are we okay with one switch for both? (IMO: one is probably fine? Maybe give a quick thought to future-proofing in the event that someone comes up with a good reason to separate the two?)

Is there are more descriptive name than gestureHandling? Almost something like capturePageScrolling: true | false. interceptPageScrolling? blockPageScrolling? My issue with gestureHandling is that even if I remember the name of the parameter and what it does, I don't know what true or false mean except to try them both and then remember which is which.

src/ui/handler/touch_pan.js Show resolved Hide resolved
@avpeery
Copy link
Contributor Author

avpeery commented Oct 18, 2021

@rreusser Thanks for taking a look!

Is the behavior of wheel and one-finger touches independently controllable? Or are we okay with one switch for both? (IMO: one is probably fine? Maybe give a quick thought to future-proofing in the event that someone comes up with a good reason to separate the two?)

It is not independently controllable. Setting gestureHandling activates both wheel and one finger touch control. I think one switch for both is OK, but open to being independently controllable if there are strong opinions.

Is there are more descriptive name than gestureHandling? Almost something like capturePageScrolling: true | false. interceptPageScrolling? blockPageScrolling? My issue with gestureHandling is that even if I remember the name of the parameter and what it does, I don't know what true or false mean except to try them both and then remember which is which.

Great point. Two alternatives would be to either change the term to cooperativeGestureHandling (boolean), or keep gestureHandling and use enum values such as something like "cooperative" and "greedy" or "accommodating" and "grabby"

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

This looks great! Only found the one issue while testing

src/css/mapbox-gl.css Outdated Show resolved Hide resolved
src/ui/handler/touch_pan.js Show resolved Hide resolved
Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

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

LGTM! 👏 🎉

src/ui/handler/touch_pan.js Outdated Show resolved Hide resolved
Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com>
@avpeery avpeery merged commit 18b8a5f into main Oct 19, 2021
@avpeery avpeery deleted the avpeery/two-fingers-to-move branch October 19, 2021 22:30
@avpeery avpeery linked an issue Oct 19, 2021 that may be closed by this pull request
katydecorah pushed a commit that referenced this pull request Oct 20, 2021
* main:
  Add touch pan blocker to gesture handling for touch devices (#11116)
  Address accessibility issues (#11064)
  add support for non-mercator projections (#11124)
  Image fallback expressions within paint properties (#11049)
  Replaces EPSG:4326 with OGC:CRS84 in GL JS `LngLat` doc (#11072)
  Add globe view support to heatmap shaders (#11120)
  Exclude flaky test (#11118)
  consistify YOUR_MAPBOX_ACCESS_TOKEN as placeholder string (#11113)
  Allow adding multiple layers to `map.on()` event handler (h/t @omerbn) (#11114)
  render-test-flakiness:clear worker storage (#11111)
  upgrade to supercluster v7.1.4, earcut v2.2.3, vt-pbf v3.1.3, geojson-rewind v0.5.1 (#11110)
  Added v1.13.2 changelog entry (#11108)
  One weird JSON.parse() trick (#11098)
  Fixed doc usage of map.getCenter (#11093)
  s̶y̶m̶b̶o̶l̶-̶c̶l̶i̶p̶ dynamic-filtering with `pitch` and `distance-from-camera` expressions (#10795)
  Update link to transpiling guide (#11096)
  Cherry pick 2.5.1 changelog (#11099)
  Fix an iOS15 issue where Safari tab bar interrupts panning (#11084)
  Fix conditional check for isFullscreen to accommodate Safari (#11086)
  Render tests for #11041 (#11070)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable pinch-to-zoom, disable scroll zooming Support multitouch two-finger panning
3 participants