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

RESOLVED FIXED

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: Mark Mentovai)

Tracking

({fixed1.8.1})

Trunk
PowerPC
Mac OS X
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.72 KB, patch
Josh Aas
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
beltzner
: approval1.8.1+
Details | Diff | Splinter Review
[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.
No longer depends on: 333791
(Assignee)

Comment 1

12 years ago
Created attachment 230151 [details] [diff] [review]
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?
Attachment #230151 - Flags: review?(joshmoz)
> 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.
> 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.
Summary: [mac] onDragExit in tabbrowser.xml gets called aDragSession.sourceNode == null (on windows, it is non-null) → [mac] onDragExit in tabbrowser.xml gets called aDragSession.sourceNode == null (on windows, it is non-null) when I drop a tab on itself
(Assignee)

Comment 4

12 years ago
OK, I see that.  The patch on this bug won't fix this particular problem, but it will fix other problems.
(Assignee)

Comment 5

12 years ago
Created attachment 230160 [details] [diff] [review]
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.
Attachment #230151 - Attachment is obsolete: true
Attachment #230160 - Flags: review?(joshmoz)
Attachment #230151 - Flags: review?(joshmoz)

Updated

12 years ago
Attachment #230160 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

12 years ago
Attachment #230160 - Flags: superreview?(sspitzer)
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?)
Attachment #230160 - Flags: superreview?(sspitzer) → superreview?(pavlov)
Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

better idea:  pink!
Attachment #230160 - Flags: superreview?(pavlov) → superreview?(mikepinkerton)
Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

er, ok, if you say so.
Attachment #230160 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 9

12 years ago
Checked in on the trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

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

Needed for drag and drop parity, supports bug 333791.
Attachment #230160 - Flags: approval1.8.1?

Comment 11

12 years ago
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
(Assignee)

Comment 12

12 years ago
This should go on to the 1.8 branch if bug 333791 does.
Blocks: 333791
Comment on attachment 230160 [details] [diff] [review]
Fix (for this AND something else)

a=drivers. Please land on MOZILLA_1_8_BRANCH.
Attachment #230160 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 14

12 years ago
Checked in on MOZILLA_1_8_BRANCH before 1.8.1b2.
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.