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

[WIP] Refactor gesture handling & improve gesture support on mobile #9223

Closed
wants to merge 9 commits into from

Conversation

vakila
Copy link

@vakila vakila commented Jan 23, 2020

This PR refactors the input gesture handling architecture to reduce bugs & improve extensibility. It also adds support for touch gestures to pan, zoom, rotate, and pitch the map.

This is work in progress; API(s) & gesture handling behavior is still subject to change.

Overview

Previously, individual handlers representing arguably-related groups of interactions (e.g. TouchZoomRotateHandler) managed both gesture detection and the resulting map animation simultaneously, which led to several problems including:

  • brittle & difficult-to-extend handlers & APIs,
  • duplication of logic shared by multiple handlers,
  • bugs due to multiple handlers competing for control of the map, and
  • a painful and/or tedious developer experience when attempting to customize interactivity.

The new architecture aims to better separate concerns between gesture detection and the animation of the map in response to these gestures. Furthermore, we now have a mechanism for managing conflicts between different gesture handlers, and controlling their hierarchy and mutual exclusivity.

HandlerManager

The module src/ui/handler_manager.js defines a HandlerManager object, accessed through the map as map.handlers. The manager:

  • Stores an ordered hierarchy of potentially-competing Handler objects (see below), with builtin handlers attached by default
  • Provides access to individual handlers through named properties, e.g. map.handlers.touchPitch
  • Provides .add() and .remove() methods to allow overriding builtin handlers and/or attaching custom handlers extending the Handler class
  • Exposes the list of available handlers dynamically through the .list() method
  • Attaches map listeners for input events (e.g. touchstart, mouseup), allows enabled handlers to process these events, and receives "recommendations" for how to update the map from applicable handlers (see below)
  • Decides which updates to apply, merging compatible handlers' effects and resolving any conflicts between competing handlers
  • Performs the actual map animation, at most once per input event
  • Fires "output" events from the map (e.g. movestart, zoomstart) as needed

Eventually it will:

  • Accept high-level interactive option(s) and enable/disable handlers accordingly
  • Provide additional convenience methods to quickly modify multiple handlers, e.g. map.handlers.disableTouch()

Handler classes

Handler objects are responsible for processing relevant input events by calculating desired updates to the map & returning information describing those updates to the manager.

The Handler base class (src/ui/handler/handler.js) provides methods common to all handlers, e.g. .enable() and .disable().

Custom handlers can be implemented by extending the base Handler class and implementing event-processing methods for any relevant input events (e.g. .mouseup(e), .keydown(e)). Handlers are ideally small and single-purpose, focused on only one type of interaction.

Upon detecting an input event (e.g. touchmove), the manager looks for a method with the same name on any enabled handlers (e.g. touchZoom.touchmove(e)), calling each such method with the original input event.

Handler event-processing methods may return an object specifying which map updates should be applied (e.g. transform: { zoomDelta: -1 } , and which output events should be fired (e.g. events: [ 'zoomstart', 'zoom']).

If a handler's method for a detected input event (e.g. .touchmove(e)) returns such an object to the manager, the manager may apply the specified update(s) to the map and fire the specified event(s), after all handlers have had a chance to process the input event and any conflicts have been resolved.

See the TouchPanHandler, TouchZoomHandler, TouchRotateHandler, and TouchPitchHandler classes in src/ui/handler/touch.js for examples.

Changes to current API

Under the new architecture, the map's former mixed-concern handlers (e.g. map.touchZoomRotate) no longer exist as such. The old handler classes are either replaced or ported to the new architecture, and the old bind_handlers module has been removed in favor of the high-level control provided by the HandlerManager.

To avoid breaking changes, the current API can be shimmed to map existing properties/methods to the new handler architecture (e.g. map.touchZoomRotate.disableRotation() mapped to map.touchRotate.disable()).

Next steps

  • [in progress] More documentation
  • [in progress] Port old handlers to new single-concern handlers (handlers in italics should be straightforward to port from the old architecture)
    • touchPan
    • touchRotate
    • touchZoom
    • touchPitch
    • tapZoom
    • quickZoom
    • mousePan
    • mouseRotate
    • mousePitch
    • scrollZoom
    • dblclickZoom
    • boxZoom
    • keyboard
  • Toggle (and rename?) container CSS classes (e.g. mapboxgl-touch-zoom-rotate) from manager
  • Expand high-level API (map.handlers.disableRotation() etc)
  • Shim existing handler API, adding deprecation warnings (can be skipped if this ends up in major release)
  • Fix flow & test errors
  • Once this is merged, fix Disable drag rotation on NavigationControl compass #9036 and check for similar (mis)uses of old handlers

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

Anjana Vakil added 7 commits January 22, 2020 19:01
HandlerManager:
- Keeps track of all handlers
- Provides high-level handler utility methods
- Listens for input events on the map
- Processes input events through all handlers
- Receives & collates handlers' recommendations
  for map transformations, resolving conflicts
- Applies map transformations as appropriate
- Fires output (movement) events as appropriate
- Add TouchHandler & MultiTouchHandler base classes
- Implement TouchPan, TouchZoom, TouchRotate, TouchPitch Handlers
- Add touch handlers to manager by default
- Add & update tests
@vakila vakila added feature 🍏 refactoring 🏗️ breaking change ⚠️ Requires a backwards-incompatible change to the API api 📝 labels Jan 23, 2020
@kkaefer kkaefer added this to the release-b milestone Jan 27, 2020
@kkaefer kkaefer modified the milestones: release-baltic, release-c Feb 12, 2020
@ansis ansis mentioned this pull request Mar 2, 2020
17 tasks
@mourner
Copy link
Member

mourner commented Mar 2, 2020

Continued in #9365

@mourner mourner closed this Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API feature 🍏 refactoring 🏗️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable drag rotation on NavigationControl compass
3 participants