125301 - Canceling drag with Esc does not properly clean things up when dragging in a tree (e.g. bookmarks, mail) => infinite scrolling

Status: RESOLVED WORKSFORME

(Core :: XUL, defect)

Description

[:deanis74]

1. Manage Bookmarks
2. Drag a bookmark over a folder
3. Press Esc immediately
4. Keep the pointer over the folder with the mouse down

Expected Results: You can wait there all day.

Actual Results: Folder springs open as if still in the drag-and-drop.

Jan: Where would I find the code that handles the Esc keypress and cancels the drag?

Comment 1

[Jan Varga [:janv]]

onDragExit() is not called ?

Comment 2

[:deanis74]

Are you saying that pressing Esc might not be calling OnDragExit()?

Comment 3

[Jan Varga [:janv]]

btw, WFM on Linux

Comment 4

[:deanis74]

Lovely, so this may be Windows-specific?  Adding a couple Mac people to give it
a try on that platform.

Jan or Blake: Where would I find where Outliner handles the pressing of Esc when
in a drag-and-drop operation?

Comment 5

[Jan Varga [:janv]]

Dean, sorry, I have no idea

Comment 6

[:deanis74]

Hyatt, do you know where it is?

Comment 7

[:deanis74]

Pressing Esc also leaves the drop line indicator hanging around.

Comment 8

[:deanis74]

I think I've tracked Esc down to being handled here:

http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/nsDragAndD
rop.js#156

Is there any way to determine what type of exception was raised?  Or maybe we 
should always call OnDragExit in there?

Comment 9

[:deanis74]

Turning on Debug > Stop for Exceptions in the JSD stops there when I press 
Esc.  Entering "evald $[0].stringValue" in the console displays details about 
the exception:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) 
[nsIDragService.invokeDragSession]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
location: "JS frame :: chrome://global/content/nsDragAndDrop.js :: anonymous :: 
line 154" data: no]


NS_ERROR_FAILURE isn't really helpful there.  One of two things could be done 
to fix this:
1. always trigger onDragExit() if hit this exception.
2. return a better error code if invokeDragSession() fails because Esc was 
pressed.

I'll look into both of these.

Comment 10

[:deanis74]

http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsDragService.cpp#158

DoDragDrop will return DRAGDROP_S_CANCEL if the drag is canceled.  So it is 
possible to return a more specific error code than NS_ERROR_FAILED.  Any ideas 
what code?

If I pursue this, I'll have to check if any code is looking specifically for 
NS_ERROR_FAILED from nsDragService::InvokeDragSession() or 
StartInvokingDragSession().

Changing OS to Windows as this is in Win-specific code, and works on Linux as 
per comment 3.

Comment 11

[Brendan Eich [:brendan]]

I doubt anyone checks for NS_ERROR_FAILURE in particular.  Should Esc really
cause a *failure*?  If so, perhaps NS_ERROR_ABORT is appropriate (its comment in
xpcom/base/nsError.h is uselessly vague).  If not -- if this case should return
NS_OK -- then how to convey the fact that the drag-drop was canceled?

Why exactly is Linux different?  Is it not failing in the NS_FAILED/JS-exception
sense for this case?

/be

Comment 12

[:deanis74]

nsDragSession::InvokeDragSession on Linux either returns NS_ERROR_INVALID_ARG if
a supplied parameter is null, or NS_OK.  It never returns an error if it is
called with all parameters.

Actually, looking through the implementation of InvokeDragSession on all
platforms, every platform except Windows will return NS_OK if all parameters
were passed in, memory could be allocated, etc.  Basically, on all all other
platforms if the native drag function is called, InvokeDragSession will return
NS_OK.  So the only platform the exception gets raised on is Windows.

Comment 13

[Brendan Eich [:brendan]]

So isn't the simplest fix to make the Windows version return NS_OK always, not
return that ?: expression that selects NS_OK or NS_ERROR_FAILURE?

/be

Comment 14

[:deanis74]

Well, sure, if you want to take the easy and obvious route!  I don't think there
would be any problems with this change, since expecting different return values
from InvokeDragSession() definitely isn't xp.  Pink, can you think of anything
obvious we might be missing?

Comment 15

[:deanis74]

If I change that to NS_OK, the exception doesn't get thrown anymore, which I 
think is good.  But now OnDragExit() never gets called.  I put a breakpoint at:

http://lxr.mozilla.org/mozilla/source/content/events/src/nsEventListenerManager.
cpp#1988

And I never reach that if I cancel a drag using Esc.  I can't see what's 
different in the DND code since StartInvokingDragSession(), and thus 
InvokeDragSession(), returns NS_OK whether Esc is pressed or the drag is 
completed as expected.

Anyone have an idea about what's going on here?  I take it that OnDragExit() is 
called on other platforms, even if the drag is canceled with Esc?

Comment 16

[:deanis74]

nsNativeDragTarget::DragLeave() gets called when Esc is pressed.  This is 
because ::DoDragDrop() returns DRAGDROP_S_CANCEL.  Something in the chain after 
this is not sending the right message to the right listener.

Ignore my previous question about other platforms, this is definitely windows-
only.

Comment 17

[:deanis74]

Pierre, do you have any ideas on this?  This is easy to duplicate in Manage > 
Bookmarks.  Just drag a bookmark so you see the placement line show up between 
two other bookmarks and press Esc.  The line doesn't disappear.

Here's a quick summary of what I've found so far:

- Pressing Esc while in a drag on Windows doesn't clean things up properly, at 
least while dragging rows in a tree.  Basically, OnDragExit() for the tree 
doesn't get called.

- Pressing Esc raises the exception mentioned in comment 8.

- If I change the return value of nsDragService::StartInvokingDragSession on 
windows to always return NS_OK instead of "return (DRAGDROP_S_DROP == res?
NS_OK:NS_ERROR_FAILURE)", I avoid that assertion.  But that doesn't help at 
all, because OnDragExit() still doesn't get called.

A thought I just had.  When DoDragDrop returns DRAGDROP_S_CANCEL, it calls 
IDropTarget::DragLeave.  Can we do something in nsNativeDragTarget::DragLeave() 
to call OnDragExit for the tree?

Comment 18

[:deanis74]

Changing summary, this is about more than springloaded folders.

Comment 19

[:deanis74]

Sending back to component owner.  I've done all the investigation necessary, I
just couldn't fix it.

Comment 20

[Andreas Kunz]

This causes crash bug 207343.

Comment 21

[Wayne Mery (:wsmwk)]

fixed by bug 210719 ?

Comment 22

[Wayne Mery (:wsmwk)]

WFM Build identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007091102 SeaMonkey/2.0a1pre

Comment 23

[Wayne Mery (:wsmwk)]

WFM also 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070802 SeaMonkey/1.1.4