342361 - Drop indicator remains visible after a tab is dragged into the content area

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

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

Drop indicator remains visible after a tab is dragged into the content area

this is mac only.

I don't we're not getting the valid ondrag* event when we leave the tab bar that we do on windows.

see also bug #342229 and bug #341100

Comment 1

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

s/I don't we're not getting/I don't think we're getting

Comment 2

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

I noticed that we get the right event when I drag up, to the url bar or bookmarks personal toolbar (away from the tab bar) but into the content area, I don't.

Comment 3

[Wayne Woods]

Yeah, I find that dragExit (in nsDragAndDrop) seems to be called appropriately except when dragging from the tab strip to the content area. That includes dragging from the tab strip to outside the browser window entirely. If some of the tab strip background is showing (say, if you only have a couple of tabs open), then dragging from that background area to the narrow strip at the bottom of the tab strip doesn't seem to do anything either, but maybe that's intentional and they're really the same element?

If the tab strip is absent, and I try to drag a bookmark button from the toolbar to the content area, then dragExit is called *just* before I enter the content area. However, that may be triggered by some narrow element at the bottom of the bookmarks toolbar for all I know, so I can't say for certain that it's working in that case.

dragOver is also called appropriately (and continuously as I move the cursor) for dragging both tabs and bookmarks buttons, until I reach the content area, at which time it stops until I enter the chrome again.

Interestingly, ondragenter events don't seem to be used for the toolbars, though I thought they could be useful if they were.

Comment 4

[Mark Mentovai]

This is because our drag and drop code in nsMacEventHandler is horribly (but not hopelessly) broken.

Comment 5

[Mark Mentovai]

Attachment: Fix

We were doing absolutely the wrong thing with NS_MOUSE_ENTER/EXIT.  We were taking the events from the OS which tell us whether the mouse has entered or exited a window, and sending those directly into the widgets, but windows aren't widgets.  The revised implementation keeps track of the last widget that a drag was over and uses that to send the proper NS_MOUSE_ENTER/EXIT events into the widgets.

Comment 6

[Wayne Woods]

Hi Mark, I tried to apply that patch to test it out. However I think there are some minor differences between the "original" copy of nsMacEventHandler.cpp that you diffed, and the trunk copy I just got from the repository. As such, the patch wouldn't apply to that file. One difference I noted was that your patch had the line:
-    aMouseEvent.point       = widgetHitPoint;
For that line, the cvs copy of the file had "aMouseEvent.Refpoint" instead of "aMouseEvent.point"
The errors given by patch suggested at least one more difference.

I did, however, manually apply the patch, and it gave an error upon building. The difference above made it pretty clear that the line applied by the patch:
+  aMouseEvent.point      = aPoint;
the ".point" had to be changed to ".refPoint"

After changing it, the build worked and I'm currently trying it out. I'm not sure if what other differences there are between the current trunk copy and the copy your patch is based on, however. Cheers.

Comment 7

[Wayne Woods]

I can confirm that ondragexit is working correctly (once the patch is modified to work as described in comment 6). The tab drag indicator arrow is then hidden when you drag over the content area (with the patch from bug 333791 also applied... otherwise a different bug hides it anyway). The same cannot be said for the bookmarks toolbar drag indicator, which still stays where you left it, but that bug may not be related to the drag and drop code. I'm happy to have a closer look once this is done.

I presume ondragover isn't supposed to be called when dragging over the content area? It's not happening, anyway.

Comment 8

[Mark Mentovai]

Wayne, this patch was prepared against the branch.  The trunk patch is slightly different in context and in point/refPoint, as you point out.

Comment 9

[Mike Schroepfer]

We'd happily take a low-risk baked patch for the 1.8 branch - but I'm clearing the  blocking flag as we wouldn't hold the release for this.

Comment 10

[Mark Mentovai]

Attachment: Trunk version (same as above)

Comment 11

[Wayne Woods]

(In reply to comment #8)
> Wayne, this patch was prepared against the branch.

Now I see. Sorry, bad assumption.

Comment 12

[Wayne Woods]

Sorry to be a bother, but one phenomenon I'm noticing, even with the new patch, that I just wanted to check...

If I grab the middle of the tab and drag it slowly towards (and then over) the content area, dragExit is called 3 times... once when it leaves the title part of the tab, once when it leaves the tab and enters the narrow tab background strip at the bottom, and a final time when it leaves that and enters the content area. dragOver is called many times, I think depending on how slowly I drag.

However, if I do the same thing, but drag rapidly, then dragExit is called only once. dragOver is also called only once. Is it intended behaviour that all other calls of those methods are either cancelled or never made?

Comment 13

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

wayne, do you have mark's other fix (for bug #342229) in your tree as well?

Comment 14

[Wayne Woods]

(In reply to comment #13)
> wayne, do you have mark's other fix (for bug #342229) in your tree as well?

I do now :) It has no effect on the observations in comment 12, though. If you drag really quickly, only one dragExit is called.

I do, however, see the timed triggers of onDragOver now, despite bug 342229 comment 6 suggesting that it shouldn't work on the trunk. Maybe it's been fixed elsewhere since then?

Comment 15

[Mark Mentovai]

The NS_DRAGDROP_OVER timer will work on the trunk when the patch on bug 342229 is applied, but the tab bar's behavior as a whole probably won't.  I'll take a look at the multiple-exit thing.

Comment 16

[Mark Mentovai]

Wayne, I've looked at the widget code here again and can say that it's doing the right thing.  You'll need to provide a specific testcase (and probably compare platforms) if you think something is wrong.  It is true that when the mouse is moving quickly, behavior will be slightly diferent from when it is moving slowly, because the OS doesn't report every pixel-level event, only new mouse positions.

Comment 17

[Mark Mentovai]

Checked in on trunk.

Comment 18

[Mark Mentovai]

Comment on attachment 226703 [details] [diff] [review]
Fix

Requesting approval1.8.1.  This is a longstanding platform parity issue, and is needed to support bug 318168 properly on the Mac.

Comment 19

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH for 1.8.1b1.