Add an "EventData#preventInteractionHandlers" API

[ntilwalli]

This is based on the latest Mapbox GL JS (v0.25.1). I've posted a demo repo which has one file (index.html) which when loaded in Chrome fully replicates the issue. That file is an almost complete replica of: https://www.mapbox.com/mapbox-gl-js/example/drag-a-point/

The only differences are the places where map.dragPan.enable() and map.dragPan.disable() are called. Instead of calling those functions during mousemove, I call them during mouseup and mousedown, but that should work fine too...

Repro file at: https://github.com/ntilwalli/mapboxDragPanIssue

Steps to Trigger Behavior

1. Load the index.html file from the repo
2. Try dragging the circle (it won't work)
3. Try dragging a second time (it will work)

Expected Behavior

Dragging circle should always work

Actual Behavior

Dragging circle only works from second attempt onwards

[lucaswoj]

JSBin-ified example: http://jsbin.com/mobicocaxe/edit?html,console,output

[lucaswoj]

@ntilwalli The behaviour you are seeing is due to the order in which the event listeners are run.

The first time that mouse.dragPan.disable() is called, the dragPan interaction has already begun and cannot be aborted.

The second time that mouse.dragPan.disable() is called, the event listeners have been reordered such that the dragPan interaction has not yet begun and disabling the interaction prevents it from beginning.

We are looking to make the event order more stable in #2891. This fix will make all calls to mouse.dragPan.disable() behave like the first call to mouse.dragPan.disable().

What you're looking is some equivalent of event#preventDefault for GL JS event handlers. I think this would be a good addition to our API 👍

As a temporary workaround, I suggest calling mouse.dragPan.disable(); mouse.dragPan.enable(); right after the map loads.

[ntilwalli]

@lucaswoj thanks for the response. The workaround you proposed works, but if I understand your comment and #2891 correctly it will stop working and the proper approach will be to use the enhancement you've proposed to preventInteractionHandlers.... Is my understanding correct?

[lucaswoj]

@ntilwalli Yes, your understanding is correct. We will address your use case before we merge #2891 😄

[lucaswoj]

💭 Idea: We could add a new method to Eventedspecifically for binding interaction handler listeners i.e. Evented#onInteractionHandler. This would allow all implementation of EventData#preventInteractionHandlers to exist within Evented.

[jfirebaugh]

I think preventInteractionHandlers is the right approach, though I'd prefer to just call it preventDefault. This suggests the symmetry between map events and DOM events: you can call preventDefault on map events to prevent the default behavior of the map, just like you can call preventDefault on DOM events to prevent the default DOM behavior.

It looks like this could be implemented with cooperation between bindHandlers and the handlers themselves, without changes to Evented. Instead of having each handler bind native event listeners to the canvas container, the base event listeners that bindHandlers sets up would:

1. Fire the relevant map event (as they do now)
2. Check if the default was prevented
3. If so, do nothing further. If not, explicitly call relevant methods on relevant handlers, in a defined order.

This would address #2419, #2891, #2237 (in a cleaner way than #4528), #4297, #6047, #6132, maybe #2204, and provide a clean base for implementing a drag example that works for both mouse and touch input (#3017).

[jfirebaugh]

Adding mutable state to events turns out to be much more difficult than anticipated.

• To document preventDefault() properly, we need to convert MapMouseEvent to a full-fledged class, which means that the signature of Evented#fire must change, which requires a sweeping change everywhere fire is used. Fine, I did this, I think it improves the codebase.
• Using instances with mutable state means you can no longer just util.extend yourself a new event object with additional properties whenever it seems convenient:
  • When propagating events to the "evented parent", you need to add the "evented parent data" to the existing event object, rather than creating a new one.
  • This code needs to do... something. I'm not even sure what. But it can't util.extend up a new object, because then e.preventDefault() from a delegated event handler doesn't work.