200745 - Dragging uses the wrong event position, making it hard to drag small images

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Simon Fraser [no longer active]]

I've noticed that in debug builds (and probably on slower machines), dragging
images doesn't always work correctly. You have to click and drag on the image
slowly, with great care, for it to work. It's harder if you drag across the
short axis of the image, or try and click-drag quickly.

The problem is that for the drag, we use the coordinates of the mouse move event
after the mouse down, rather than the initial click. This code is in
nsEventStateManager::GenerateDragGesture(). The drag event does use the frame
that the first click hit (mGestureDownFrame), but it uses the widget of the
second event. This needs fixing.

Comment 1

[Simon Fraser [no longer active]]

It also turns out that we need to reset mCurrentTargetContent in the ESM while
dispatching the DOM event for the drag, so that the event target gets set
correctly (if we don't set it in the ESM, a cached value for the 'current
target' in the pres shell is used).

Comment 2

[Simon Fraser [no longer active]]

Attachment: First-cut patch to the ESM

Here's a patch which you ESM folks should clean up

Comment 3

[Simon Fraser [no longer active]]

There's still a bug (with or without this patch), which is that if you click on
an image and drag fast enough outside the window, a context menu shows up where
the drag started. I guess this happens if the second, mouse move event occurs
outside the window.

Comment 4

[Mike Pinkerton (not reading bugmail)]

looks good enough to me, but bryner should probably take a real look at this, or
someone with more ESM experience.

Comment 5

[John Keiser (jkeiser)]

Comment on attachment 119523 [details] [diff] [review]
First-cut patch to the ESM

Looks fine to me, except for two things:

(1) as you noted you had already changed, the second if (mGestureDownFrame)
still needs to be there to catch destroyed frame conditions

(2) mCurrentRelatedContent -- if this needs to be set at all, it should just be
set to null.  As it is, you are setting it to the same content as
mCurrentTargetContent, which doesn't seem useful.

Comment 6

[Steve K]

There is an earlier, but still duplicate bug reporting this behavior in bug
194459 (with votes and discussion).

My guess is that although that bug is earlier, this one seems to be in the
proper component, and also has a patch attached, so that one might be marked as
a (before the fact) duplicate of this.

Comment 7

[Simon Fraser [no longer active]]

*** Bug 194459 has been marked as a duplicate of this bug. ***

Comment 8

[Kathleen :Brade]

-->sfraser since he proposed the patch

Comment 9

[Michael Buckland]

Discussed in edt.  Plussing.

Comment 10

[Simon Fraser [no longer active]]

Attachment: nsEventStateManager patch

This patch improves things, except in the case where you quickly drag out of
the window. To fix that, I'm going to have to capture the mouse on mousedown.

Comment 11

[John Keiser (jkeiser)]

Comment on attachment 120605 [details] [diff] [review]
nsEventStateManager patch

r=me, we may want to consider what we want relatedTarget for in the future but
this bug is more important.  Asking for sr=bryner

Comment 12

[Simon Fraser [no longer active]]

Checked in.

Comment 13

[Greg K.]

My bug 185906 may be a dup of this, as well.

Comment 14

[Alex Fritze]

This patch causes a crash in nsEventStateManager::GenerateDragGesture() when the
content tree is rearranged between mousedown and mousemove listeners, see  testcase.
The problem is that mGestureDownFrame will be null by the time
GenerateDragGesture is called and we're missing a test against that.

Simple fix:
Index: nsEventStateManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v
retrieving revision 1.433
diff -u -r1.433 nsEventStateManager.cpp
--- nsEventStateManager.cpp	19 Apr 2003 00:46:27 -0000	1.433
+++ nsEventStateManager.cpp	24 Apr 2003 12:26:32 -0000
@@ -1416,7 +1416,10 @@
 
       // get the widget from the target frame
       nsCOMPtr<nsIWidget> targetWidget;
-      mGestureDownFrame->GetWindow(aPresContext, getter_AddRefs(targetWidget));
+      if (mGestureDownFrame) 
+        mGestureDownFrame->GetWindow(aPresContext, getter_AddRefs(targetWidget));
+      else
+        targetWidget = aEvent->widget;
       
       nsEventStatus status = nsEventStatus_eIgnore;
       nsMouseEvent event;

Comment 15

[Alex Fritze]

Attachment: testcase for GenerateDragGesture crasher

Comment 16

[Simon Fraser [no longer active]]

Bug 202843 covers that regression. I added a link to the testcase there.