336241 - canDrop isn't called when just the drag and drop action changes

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Mark Banner (:standard8)]

Whilst trying to implement some new Drag & Drop functionality in address book, I found some problems with the drag and drop code in trees.

Currently, when you drag an address book item over an address book, as the mouse reaches the address book then canDrop is queried with the current action state. However if you then keep the mouse still and change the drag action by pressing a modifier key canDrop isn't re-queried.

I need this as in some cases within address book a copy should be allowed, but a move isn't. With the functionality as it is, it will cope, however the graphics aren't updated appropriately to inform the user as to what is actually happening.

The problem is that in nsTreeBodyFrame::HandleEvent (http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2016) it filters out changes where the mouse hasn't moved, but doesn't take into account the drag action.

I have a patch in progress for this and will attach it later.

Comment 1

[Mark Banner (:standard8)]

Attachment: Keep track of the last drag action and add it to the list of checks for changes

This patch adds a value to Slots so that the drag action of the last call to HandleEvent is tracked and then is used in the comparison for changes to see if just the drag action has changed.

If you wish to test it, at the moment I think the best way is find a drag & drop case for a tree where you can insert some text output into canDrop and then start to involke a drag to the point where canDrop is queried. Keep the mouse where it is and use the modifier keys to change the drag action - you should see the text output coming out as well.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not qualified to review this code, at least if you want a review sometime this month; I'd try someone like Neil, perhaps...

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 220525 [details] [diff] [review]
Keep track of the last drag action and add it to the list of checks for changes

(In reply to comment #2)
> I'm not qualified to review this code, at least if you want a review sometime
> this month; I'd try someone like Neil, perhaps...
> 

Thanks for letting me know. Trying Neil...

Comment 4

[[e-mail]]

Comment on attachment 220525 [details] [diff] [review]
Keep track of the last drag action and add it to the list of checks for changes

>+ *   Mark Banner <[e-mail]>
:-P

>     nsCOMPtr<nsIDragSession> dragSession;
Hey, is this line unused? ;-)

>     PRInt16 lastDropOrient = mSlots->mDropOrient;
>     PRInt16 lastScrollLines = mSlots->mScrollLines;
> 
>+    // Find out the current drag action
>+    PRUint32 currentDragAction = mSlots->mLastDragAction;
>+    if (mSlots->mDragSession)
>+      mSlots->mDragSession->GetDragAction(&currentDragAction);
I think you're going about this the wrong way around. As the two lines of context demonstrate, your temporary variable should be the last value, and the Slots member should be named mDragAction.

>       PRInt16                  mDropOrient;
> 
>+      // The last drag action that was received for this slot
>+      PRUint32                 mLastDragAction;
>+
>       // Number of lines to be scrolled.
>       PRInt16                  mScrollLines;
Putting a 32 between two 16s is not good. Here's the layout on a PPC:
NN NN NN NN mDropRow
NN NN XX XX mDropOrient
NN NN NN NN mDragAction
NN NN XX XX mScrollLines
NN NN NN NN mDragSession
As you can see, you just required 8 extra bytes instead of 4. Try this:
NN NN NN NN mDropRow
NN NN       mDropOrient
      NN NN mScrollLines
NN NN NN NN mDragAction
NN NN NN NN mDragSession

Comment 5

[Mark Banner (:standard8)]

Attachment: Keep track of the last drag action and add it to the list of checks for changes v2

Updated patch to address Neil's comments.

Comment 6

[[e-mail]]

Comment on attachment 220685 [details] [diff] [review]
Keep track of the last drag action and add it to the list of checks for changes v2

Looks good now.

Comment 7

[Mark Banner (:standard8)]

Patch checked into trunk -> fixed.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 220685 [details] [diff] [review]
Keep track of the last drag action and add it to the list of checks for changes v2

Requesting branch approval for this patch. I'd like to get it in as it provides the required functionality for the first address book drag and drop patch on bug 35837.

Its been on trunk for about a week now and no one's raised any regressions as far as I can see.

I'm hoping to get both patches on that bug in for Thunderbird 2.0/SeaMonkey 1.1 but this bug will block either of them going in.

Comment 9

[Mark Banner (:standard8)]

Fix checked into branch.