345425 - [mac] onDragExit in tabbrowser.xml gets called aDragSession.sourceNode == null (on windows, it is non-null) when I drop a tab on itself

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[(not reading, please use [e-mail] instead)]

[mac] onDragExit in tabbrowser.xml gets called aDragSession.sourceNode == null (on windows, it is non-null)

to reproduce, drag and drop a tab onto itsself, and add a check to onDragExit to see what aDragSession.sourceNode is.

this comes from bug #333791.

I'm working around this (by checking aDragSession.sourceNode, but it appears to be a dnd platform parity bug.

Comment 1

[Mark Mentovai]

Attachment: Fix (for something else?)

> onDragExit in tabbrowser.xml gets called aDragSession.sourceNode == null

Hi, Seth, how're the kids?  I don't see this happening.  I placed a couple of dumps:

if (aDragSession.sourceNode) dump("i HAVE a sourceNode\n");
                        else dump("you HAVE no sourceNode!\n");

in tabbrowser.xml here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.103.2.54&mark=1803#1798

and always HAD a sourceNode.

I did find another bug in the Mac drag'n'drop code that caused NS_DRAGDROP_EXIT to be sent instead of NS_DRAGDROP_ENTER, due to a stupid mechanical error I made in bug 342361.  Could the extraneous ondragexit events for widgets that we're actually entering be causing your problems?

Comment 2

[(not reading, please use [e-mail] instead)]

> Hi, Seth, how're the kids?  

don't make me come over there.

> I don't see this happening.  I placed a couple of dumps:
> if (aDragSession.sourceNode) dump("i HAVE a sourceNode\n");
>                        else dump("you HAVE no sourceNode!\n");
>
> and always HAD a sourceNode.

Yesterday, on the trunk, on mac os x, debug, I definitely did not have a aDragSession.sourceNode when I attempted to drag-n-drop a tag onto itself.

I'll try your patch again and double check.

> I did find another bug in the Mac drag'n'drop code that caused 
> NS_DRAGDROP_EXIT to be sent instead of NS_DRAGDROP_ENTER, due to a stupid
> mechanical error I made in bug 342361.  Could the extraneous ondragexit events > for widgets that we're actually entering be causing your problems?

I don't think that would cause this, unless in that scenario, aDragSession.sourceNode was null.

Comment 3

[(not reading, please use [e-mail] instead)]

> when I attempted to drag-n-drop a tag onto itself.

oops, sorry mark.  see what happens when you actually drop the tab on itself.

that's when I get the onDragExit with aDragSession.sourceNode == null.

Comment 4

[Mark Mentovai]

OK, I see that.  The patch on this bug won't fix this particular problem, but it will fix other problems.

Comment 5

[Mark Mentovai]

Attachment: Fix (for this AND something else)

We're calling EndDragSession too soon, and that clears out sourceNode.  We don't need to call EndDragSession in the Drop proc because we already call it from InvokeDragSession immediately after calling TrackDrop, and the Drop proc can only be called from inside of TrackDrop.

I've also included the enter/exit confusion patch and removed a couple of overridden methods that did nothing but call their superclasses.  I love it when you can fix bugs purely by removing code.

Comment 6

[(not reading, please use [e-mail] instead)]

Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

I'm not comfortable sr'ing anything in mozilla/widget.

who else could do this?  (stuart?)

Comment 7

[(not reading, please use [e-mail] instead)]

Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

better idea:  pink!

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

er, ok, if you say so.

Comment 9

[Mark Mentovai]

Checked in on the trunk.

Comment 10

[Mark Mentovai]

Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

Needed for drag and drop parity, supports bug 333791.

Comment 11

[Wayne Woods]

Is this going on the 1.8 branch as well? If so, we can remove the extra check from the patch on bug 333791 before it's applied to Fx 2.0

Comment 12

[Mark Mentovai]

This should go on to the 1.8 branch if bug 333791 does.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

a=drivers. Please land on MOZILLA_1_8_BRANCH.

Comment 14

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH before 1.8.1b2.