Handler.Drag: Does not move until the cursor, and does not finish properly.

[jorix]

Using DragPan control, sometimes when the map is moved does not move to the place expected. This happens when we move fast and stop without releasing the mouse. Is easier to observe with large maps and slow browsers (e.g. IE) . This happens when property "interval" of Drag handler is greater that 0, to increase the performance.

I saw the Handler.Drag ends in four ways (up, out, deactivate & simultaneous right click) but not in all cases does the same. Why? No reason, It is necessary to do the same in all four cases, otherwise unexpected behavior occurs (shows the cursor "move" after finishing with a right click; or can not select text using IE after right click or deactivate() because document.onselectstart = OpenLayers.Function.False)

So the patch of this ticket proposes:

• When the mouse is moved without releasing it produces a "move" deferred.
• Use the same cancellation of "drag" on ending drag (in all four cases).

The patch includes a detailed test.

This also fixes ticket: #1101 and #2734 (patch proposed in #2734 is incomplete, fails the test)

NOTE: With "documentDrag = true" can also occur behavioral problems. But his study is beyond the scope of this ticket.

[jorix]

NOTE: The test "test_Handler_Drag_submethods" has changed a little to check the value of document.onselectstart after finishing the call, because the value after call is that marks the behavior.

The tests work fine in IE8, FF36, Safari5 and Chrome7 (with the current handler five tests fail)

Please review thanks.

[erilem]

I just came across the issue. This is a big issue!

[erilem]

The issue can be easily reproduced on  http://www.openlayers.org/dev/examples/fullScreen.html: move the map w/o immediately releasing the mouse after moving, you'll see "jumps" on mouseup.

And actually the jump somehow makes sense, as it positions the map under the mouse at the same place where the mousedown occurred. We get this jump effect when interval is non-zero, i.e. when we purposefully miss mousemove events.

patch-2936-A0.diff fixes the "jump on mouseup" issue. With the patch the jump occurs earlier, when the "delayed drag" timeout expires. So the patch does improve things in my opinion - better user experience.

Tests continue to pass.

@jorix, my patch is just a subset of yours. Small patches with restricted focuses are much easier to review. Tell me if you think some of your tests could be added to my patch. Thanks for reporting the issue and the patch.

[jorix]

Replying to erilem:

patch-2936-A0.diff fixes the "jump on mouseup" issue. With the patch the jump occurs earlier, when the "delayed drag" timeout expires. So the patch does improve things in my opinion - better user experience.

Perfect! I watched the patch: this.lastMoveEvt = evt; at the beginning of the method is very effective. (I put inside, so my code is more complicated :-()

Question: Can be omitted to assign this.delayedEvt = null; at the end of drag?

Small patches with restricted focuses are much easier to review.

Yes, I know. I did not see the two bugs could be independent. I worried about leaving active setTimeout so I looked at the completions of the drag.

Tell me if you think some of your tests could be added to my patch. Thanks for reporting the issue and the patch.

The tests are designed to testing thoroughly proper completion of drag. In a new ticket?
Only a very small affect the behavior of your patch, but to add them should dismantle the structure of the tests. I prefer not to touch the structure of the tests.

[erilem]

patch-2936-A1.diff comes with tests, and addresses the case where a new mousedown occurs before the timer triggers.

@jorix,

• I don't see any problem with not setting lastMoveEvt to null after drag. Could you please provide a test case if you see one?
• With this patch leaving an active timer shouldn't cause any issue, because of the if(this.dragging) guard. Again, please provide a test case if you see an issue.

[jorix]

Replying to erilem:

patch-2936-A1.diff comes with tests, and addresses the case where a new mousedown occurs before the timer triggers. @jorix ...

I tried the new patch and tests I see everything well (and with if(this.dragging) avoids my only small concern, a possible delayed move after mousedown).

Okay! With this patch, users will feel more comfortable with drag! :-)

[crschmidt]

Although it looks like this issue is less likely to be an issue now, because the default dragTimeout is 0, with a higher value, I was able to see problems in this behavior, and I can confirm that this appears to be the correct way to handle the situation -- fixes bugs, doesn't fail tests for me, etc.

There was one problem with applying the patch against trunk, so I'm uploading a new version; erilem, you can feel free to commit.

[crschmidt]

This patch appears to break panning in the google-v3.html example.

[crschmidt]

Ignore previous comment. Google v3 dragging appears to simply be broken in trunk.

[jorix]

@erilem: What to do with the other bugs in the cancellation of drag?

for example:

• keep left mousedown.
• right mousedown.
• release the left mouse button and then the right.
  • IE, Chrome & Safari: try by selecting a text out of the map, oops! does not work.
  • FF, Chrome & Safari: press any option from the popup menu. Oops! the cursor is still dragging.

[erilem]

Replying to jorix:

@erilem: What to do with the other bugs in the cancellation of drag? for example: * keep left mousedown. * right mousedown. * release the left mouse button and then the right. * IE, Chrome & Safari: try by selecting a text out of the map, oops! does not work. * FF, Chrome & Safari: press any option from the popup menu. Oops! the cursor is still dragging.

I think they deserve separate tickets.

[erilem]

(In [11635]) the map may jump on mouseup after dragging, p=me, r=crschmidt (closes #2936)

[erilem]

Thanks for the updated patch Chris.

[tschaub]

This patch results in unhandled errors while dragging on mobile devices (at least my iOS simulator). Open the mobile example and drag pan to see the errors (enable the debug console in Safari first).

[tschaub]

equal treatment for touch

[tschaub]

r11635 didn't set lastMoveEvt on touch devices.

[erilem]

Thanks for catching this Tim, please commit.

[tschaub]

(In [11671]) Setting lastMoveEvt in dragmove so it is accessible in both touch and mouse environments. r=erilem (closes #2936)