Disabling and enabling the box zoom interaction messes it up

[averas]

Discovered this while working on #2173

mapbox-gl-js 0.15.0:

Steps to Trigger Behavior

1. Wait for the map to load map.on("load", ...)
2. Disable box zoom map.boxZoom.disable();
3. Enable box zoom map.boxZoom.enable();
4. Perform a box zoom

Demonstration at http://codepen.io/anon/pen/LNNqrP

Expected Behavior

That it works as before disable/enable.

Actual Behavior

Map gets stuck dragging right after the box zoom, as if the mouse button is down even though it is not.

This may be related to OSX and have a similar root cause of #1888

[averas]

I believe the problem here is that the current implementation of interaction handlers depends on initialization order. When the handlers are initially enabled in the order defined in interaction.js their respective event listeners are also registered in "correct" order. However, later on, when disable() and enable() are invoked, the required event listener order chain is broken.

This could be adressed by making enable() and disable() "smarter" in how they register/unregister their listeners, or probably better, the overall interaction model has to be refactored to somehow make initialization order inconsequential.

Personally I would say that this is a quite serious issue as it more or less means that disabling and enabling of interactions will yield unexpected results for a user of the API.

Also worth mentioning that this is only relevant for interactions that base their behaviour upon the active state of others.

cc: @lucaswoj @bhousel

[averas]

... another possible solution is actually to let disabled interaction handlers keep their event listeners registered, but ignore them. That way the order of the initially registered event listeners would remain intact.

[averas]

@lucaswoj @jfirebaugh Any opinions on my last suggested option of letting listeners persist for disabled handlers but making them noops? Feels a bit like a workaround of the real problem, but at the same time, implementing arbitrary handler initialization order may be complicated and create more inter-handler-dependencies.

@mourner Any good experiences from how leaflet does it here?

[mourner]

@averas in Leaflet, I don't recall any situations where interactions would depend on the order they were enabled. We need to figure out the core issue here. Why is it order-dependent in the first place?

[averas]

Why is it order-dependent in the first place?

In this particular case (there may be more), there is a check in drag_pan.js such as:

   _ignoreEvent: function (e) {
...
        if (map.boxZoom && map.boxZoom.isActive()) return true;
...
}

This implicitly requires that the boxZoom listener is called before the dragPan listener so that the active property has been set before the evaluation is done. As I described above, the initial initialization of handlers in interaction.js ensures this order, but the listeners then get out of order when disable/enable are called later on.

[mourner]

Oh, I see. Yeah, that's a tricky issue. I think we should find another way to bail out of a handler, not using isActive. Maybe introduce a method that would accept an event and return true if the handler agrees to process it? Then you could check in one handler if an event should be processed by a different handler without relying on it being active at the time.

[lucaswoj]

I can confirm that this bug is still present in master.

[jasonpepper]

My workaround for this is to disable both the dragPan and the doubleClickZoom, and then re-enable them all in the following order: boxZoom, doubleClickZoom, dragPan. I'm not sure if the re-enabling doubleClickZoom is necessary, but that is the order they all started up in originally.

[kristfal]

AFAIK, the 'best practice' for adding / removing event listeners is to remove listeners on DOM-teardowns (when removing a DOM element), but otherwise persist the listener.

In addition, event listeners can't be removed while they are active (this affects touch track and mouse drag listeners), meaning that some listeners need a different disable mechanism anyway. Given the low amount of listeners invoked by mapbox-gl, I don't think any noticeable memory or performance issues will appear if they persist either.

All in all, I think persisting listeners would be both simpler to implement and give more control of handling input events. @mourner what are your thoughts on this?

[mourner]

@kristfal yes, that's exactly what I have suggested in #2891. I think we should move towards that direction.

[kristfal]

@mourner Thanks for the confirmation, didn't see that issue.

I'll sketch out a more detailed proposal and tie all this together in a summarized ticket when I have time.

On top of my head (let me know if you think this is the wrong direction):

If we want to support a few of the 'advanced' touch guestures that are supported in mapbox-gl-native, we'll need some kind of 'gesture recognizer' logic (similar to what hammer.js and a few other gesture libs I've checked are using). This recognizer should be responsible for delegating events to the correct handler, instead the current pattern where each handler have to check and set the state of other handlers on input.

This recognizer pattern would represent a major rewrite of the existing logic, so I'd prefer to first focus on fixing the most critical current issues (number one would be to retain listeners, then iron out the edge cases that have already been identified).