Closed Bug 345425 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: moco, Assigned: mark)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

[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
Attached patch Fix (for something else?) (obsolete) — Splinter Review
> 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
OK, I see that.  The patch on this bug won't fix this particular problem, but it will fix other problems.
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)
Attachment #230160 - Flags: review?(joshmoz) → review+
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+
Checked in on the trunk.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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?
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
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+
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.