OpenLayers.Control.Navigation with documentDrag enabled removes all document events.

[mgleahy]

Currently, the OpenLayers.Handler.Drag class used by the Navigation control uses the OpenLayers.Events.destroy() method when the documentDrag option is enabled after a user drags the map outside of the map element's boundaries.

This destroy() method has the result of removing all events for the element in question (in this case, the document element). As a result, other controls that rely on document events (e.g., the KeyboardDefaults control) will stop working once this has happened at least once.

The patch that I'll attach to this ticket switches tactics in the Drag class, such that it uses the OpenLayers.Events.un() method to unregister the only the events used by the Navigation control, and instead of creating the documentEvents object each time the user leaves the map, it keeps it and just calls the on() un() methods as users move the mouse out of the map when dragging.

[mgleahy]

Patch to fix OpenLayers.Handler.Drag

[mgleahy]

Note: this patch will apply to both 2.10 and trunk...but the documentDrag option currently is not functional in trunk.

[ahocevar]

Wouldn't it be better to use OpenLayers.Event.observe/stopObserving instead of creating/destroying an Events instance, like in Handler.Keyboard?

[ahocevar]

If Event.observe is not an option, it would be better to create the Events instance in the constructur, and register the events in mouseout.

[mgleahy]

From your note, were you suggesting something like:

OpenLayers.Event.observe(document,'mousemove',this.mousemove);

then later:

OpenLayers.Event.stopObserving(document,'mousemove',this.mousemove);

If so, I'm not having success experimenting with that approach at the moment. Is it different enough from from the Events.on() and Events.un() functions for registering/unregistering the event functions to have a significant impact? How would you recommend using the observe/stopObserving approach, relative to the changes in the patches I've posted?

I'll attach a new patch that creates the Events object in the initialize function as you suggest, which accomplishes more or less the same thing, but creates the Events object before it is needed, instead of at the time that the user first leaves the map extent (as Drag.js was originally written).

[ahocevar]

This seems to be a regression caused by r10873.

I am attaching a patch that uses Event.observe/stopObserving. As mgleahy stated correctly, this is not as straightforward as it may seem, but it pays off: creating and destroying Events instances is relatively expensive compared to just observing events. My patch also moves the code for adding the observers to a separate addDocumentEvents method.

The document-drag.html example still works as expected, and tests still pass. Thanks for any review.

[mgleahy]

Looks like that patch works for me. Thanks for the help.

[erilem]

I'll review this. Sorry for the regression.

[erilem]

I'm a bit confused.

At least in FF, documentDrag does not work in the current trunk (r10948), see  http://www.openlayers.org/dev/examples/document-drag.html. This isn't a regression caused by [10873] but by [10871].

But if I understand correctly, this particular ticket isn't about fixing documentDrag. It is about Events:destroy removing all event listeners for the element attached to the Events instance. Shouldn't we fix Events:destroy for that?

[mgleahy]

You are correct - the initial problem that I described is related to the Events:destroy problem.

The note about documentDrag not working on trunk was just a note to explain the applicability of the patch I had submitted, which I could only verify with the 2.10 release.

[erilem]

Replying to mgleahy:

You are correct - the initial problem that I described is related to the Events:destroy problem.

See 2941.patch . This patch aims to show what I've been thinking of as a potential fix, it is absolutely untested.

[ahocevar]

@erilem: Sorry I blamed r10873 for this regression, and sorry for my r10871 actually causing it. In 2941.patch , shouldn't you be stopObserving "dragstart" rather than "drag"? Other than that, your patch makes sense.

My patch reduces a lot of overhead and improves fast dragging out of the map, independent of the other issue. So I'd still like to get a review for openlayers-2941.patch .

[erilem]

Replying to ahocevar:

@erilem: Sorry I blamed r10873 for this regression, and sorry for my r10871 actually causing it. In 2941.patch , shouldn't you be stopObserving "dragstart" rather than "drag"? Other than that, your patch makes sense. My patch reduces a lot of overhead and improves fast dragging out of the map, independent of the other issue. So I'd still like to get a review for openlayers-2941.patch .

So we have three different things to deal with:

• the documentDrag regression (I currently have no idea how to fix that one)
• the issue reported by this particular ticket (2941.patch )
• your overhead removal patch (openlayers-2941.patch )

[ahocevar]

Replying to erilem:

So we have three different things to deal with: * the documentDrag regression (I currently have no idea how to fix that one)

This is fixed by my patch.

* the issue reported by this particular ticket (2941.patch ) * your overhead removal patch (openlayers-2941.patch )

[erilem]

Replying to ahocevar:

Replying to erilem:

So we have three different things to deal with: * the documentDrag regression (I currently have no idea how to fix that one)

This is fixed by my patch.

I thought I applied your patch and still had the issue. I'll look at it more closely tonight.

[erilem]

Replying to erilem:

Replying to ahocevar:

Replying to erilem:

So we have three different things to deal with: * the documentDrag regression (I currently have no idea how to fix that one)

This is fixed by my patch.

I thought I applied your patch and still had the issue. I'll look at it more closely tonight.

You're right, your patch fixes it. I wanted to understand what the actual issue was, so I've spent to time on a patch that fixes the regression without changing the code the way your patch does. See openlayers-2941.2.patch . What do you think? Do you think a patch like mine would make sense even if we apply yours?

[erilem]

Replying to erilem:

You're right, your patch fixes it. I wanted to understand what the actual issue was, so I've spent to time on a patch that fixes the regression without changing the code the way your patch does. See openlayers-2941.2.patch . What do you think? Do you think a patch like mine would make sense even if we apply yours?

Forget it. My patch doesn't work. Sorry for the noise.

[erilem]

The issue is that Events:getMousePosition no longer works (since r10871) when the element is document - pixels with NaN x and y are returned. One solution is to use the viewport element instead of the document in the drag handler for documentDrag. See openlayers-2941.3.patch , which works for me in FF3, Chrome7, IE6 and IE7.

Andreas, feel free to commit either my patch or yours. The one that you think is best.

Also, we should open a new ticket for 2941.patch .

[ahocevar]

@erilem: not an easy decision. Your patch adds less complexity to Handler.Drag, which I like a lot. But playing with document-drag.html and dragging very fast, my patch has the advantage that it produces less offset between the drag starting point on the map and the mouse cursor after dragging. So I decided to commit my patch. Thanks a lot for your hard work on this!

[ahocevar]

r10954

[erilem]

Andreas, my patch changed

if (this.started && OpenLayers.Util.mouseLeft(evt, this.map.div)) {

to

if (this.started && OpenLayers.Util.mouseLeft(evt, this.map.viewPortDiv)) {

in the mouseout function.

You know this part better than I do, what do you think?

[ahocevar]

Good catch erilem. You are absolutely right. Can you please commit this change? Thanks.

[erilem]

closed again with [10956].