Refactor interaction handlers to never unbind their event listeners

[mcwhittemore]

mapbox-gl-js version: 0.21.0

Steps to Trigger Behavior

https://jsbin.com/havivaruwe/edit?html,output

1. map.doubleClickZoom.disable();
2. on double click
   • map.fitBounds(...)
   • map.doubleClickZoom.enable();
3. zoom back out
4. on double click
   • map.fitBounds(...)

Expected Behavior

I expect the fit bounds call to bring me to the bounds

Actual Behavior

the double click zoom happens.

[mourner]

I suppose this happens because disable removes the standard doubleclick listener, and enable adds it back after the existing handler, and the first one takes the priority. A possible solution would be to not remove the handler on disable, but rather set a flag so that it's a noop.

[lucaswoj]

When I run this example:

1. the first double click zooms to the bounds
2. subsequent double clicks invoke the DoubleClickZoom interaction handler

Looking at the code, this is what I'd expect to happen. You are binding two interaction handlers to the double click event and the more recently bound one takes precedence.

If you would like subsequent double clicks to zoom to the bounds, don't enable the DoubleClickZoom interaction handler. If you would like subsequent double clicks to run the DoubleClickZoom interaction handler, disable the custom dblclick listener.

[mcwhittemore]

@lucaswoj - I know this is old, but I missed your comment above.

I see how you are reading the code and if you assume that map.on('dblclick', () => {}) works like map.onclick = () => {} than I agree with what you've said. That said, this isn't how I expect events to work. I expect all event handlers to be fired by an event unless another handler prevents propagation.

[mcwhittemore]

@lucaswoj and I talked this over a bit today. I was thinking that the user-provided callback was being overwritten with the doubleClickZoom callback. This is not true. The problem here stems from the order of operations. As both doubleClickZoom and the user-provided callback change the camera the second event to run overrides the first. This can be solved by users by Always disabling doubleClickZoom when they are creating dblclick handler that affects the camera.

[mourner]

@mcwhittemore see my comment above.

[mcwhittemore]

@mourner - Yea, I was of the same mind as your comment, but @lucaswoj proved that this wasn't the case and that what is happening instead is that the order the handlers are being run makes it seem like one of the handlers was never run.

[mourner]

@mcwhittemore Yeah, but order of events is exactly what I meant. After dblclick is removed by this line and then added back, it starts to happen after the user dblclick, not before it. We can make it retain the order by not actually removing dblclick listener when calling disable, but rather checking enabled state in the listener.

[mcwhittemore]

@mourner - gotcha.

I think it's wiser to have the default interaction handlers run last rather than first. This way the user can make a dynamic interaction such as if the user clicked on a shape zoom to the bounds of that shape, else zoom in. This would require being able to disable the handler once (via stop propagation?).

Right now we do this in Draw by disabling the handler and then enabling it via a setTimeout.

[lucaswoj]

Another solution that may work well for the GL Draw use case is adding a EventData#preventInteractionHandlers API (like the DOM Event#preventDefault API). This would require some changes to Evented. See #3290 for more.

Per discussion in #3290, we may need to resolve #3290 along side resolving this issue.

[mcwhittemore]

I'm a fan of the preventInteractionHandlers option. This would require that these handlers always be run last.