Closed
Bug 342361
Opened 19 years ago
Closed 19 years ago
Drop indicator remains visible after a tab is dragged into the content area
Categories
(Core Graveyard :: Widget: Mac, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8.1beta1
People
(Reporter: moco, Assigned: mark)
References
Details
(Keywords: fixed1.8.1)
Attachments
(2 files)
(deleted),
patch
|
jaas
:
review+
bryner
:
superreview+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•19 years ago
|
||
s/I don't we're not getting/I don't think we're getting
Reporter | ||
Comment 2•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 4•19 years ago
|
||
This is because our drag and drop code in nsMacEventHandler is horribly (but not hopelessly) broken.
Assignee: sspitzer → mark
Assignee | ||
Updated•19 years ago
|
Component: Tabbed Browser → Widget: Mac
Product: Firefox → Core
Target Milestone: --- → mozilla1.8.1beta1
Version: 2.0 Branch → 1.8 Branch
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8.1?
Hardware: PC → Macintosh
Assignee | ||
Comment 5•19 years ago
|
||
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.
Attachment #226703 -
Flags: review?(joshmoz)
Attachment #226703 -
Flags: review?(joshmoz) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #226703 -
Flags: superreview?(bryner)
Comment 6•19 years ago
|
||
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•19 years ago
|
||
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.
Updated•19 years ago
|
QA Contact: tabbed.browser → mac
Assignee | ||
Comment 8•19 years ago
|
||
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•19 years ago
|
||
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.
Flags: blocking1.8.1? → blocking1.8.1-
Assignee | ||
Comment 10•19 years ago
|
||
Comment 11•19 years ago
|
||
(In reply to comment #8)
> Wayne, this patch was prepared against the branch.
Now I see. Sorry, bad assumption.
Comment 12•19 years ago
|
||
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?
Reporter | ||
Comment 13•19 years ago
|
||
wayne, do you have mark's other fix (for bug #342229) in your tree as well?
Comment 14•19 years ago
|
||
(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?
Assignee | ||
Comment 15•19 years ago
|
||
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.
Assignee | ||
Comment 16•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #226703 -
Flags: superreview?(bryner) → superreview+
Assignee | ||
Comment 17•19 years ago
|
||
Checked in on trunk.
Assignee | ||
Comment 18•19 years ago
|
||
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.
Attachment #226703 -
Flags: approval1.8.1?
Updated•19 years ago
|
Attachment #226703 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 19•19 years ago
|
||
Checked in on MOZILLA_1_8_BRANCH for 1.8.1b1.
Keywords: fixed1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•