Closed Bug 337761 Opened 18 years ago Closed 16 years ago

Bookmarks / personal toolbar folder doesn't open for dragged link


(Core :: XPCOM, defect, P3)






(Reporter: ria.klaassen, Assigned: jimm)



(Keywords: regression)


(2 files, 1 obsolete file)

Steps to reproduce:
- Drag link or favicon onto folder
- Result: folder does not open, although after you drop it on the icon, you'll find the link inside the folder
- You can also see other weird things, for instance a drag indicator on the toolbar, and when you drop the link in the page, one of the folders opens and closes quickly without any use

Regression between 1.9a1_2006051010 and 1.9a1_2006051022.

So maybe bug 337305 has done this.
Is this bug about not being able to D&D a URL into a sub-folder (on the bookmarks toolbar) (folder doesn't "open" on hover")?

Does "Places" not have an owner?
(In reply to comment #1)
Indeed. Main folders don't expand so you can't open sub-folders as well.
This affects dragging a link to the "Bookmarks" menu as well here.
Tried with a clean profile and clean firefox folder.

When I release the button (mouse up) on the "Bookmarks" menu, it will be added to the bottom of the list and the menu will open for a very brief period and close.
Flags: blocking1.9a2?
Could this be a result of bug #203573 ?
Which addresses the UI freeze of firefox during dragging.
This can not be bug 203573, as there was no patch submitted there (yet) !
(In reply to comment #5)
> This can not be bug 203573, as there was no patch submitted there (yet) !

Because there isn't a fix for that bug this bug can't be a result of that bug? That makes no sense.
From that bug I quote: "the workaround fires events after the end of the drag
session only.". That is exactly what I am seeing when dragging links to the bookmarks menu.
Bug 203573 was filed years before places was born and this function only regressed after 10 May 2005, like described in comment 0.
Re-checked, and the regression range is correct. Has someone an idea what bug else could have caused this except for bug 337305 - which still seems the most likely culprit to me?
if bug 326273 fits into regression range it seems like a likely candidate, it caused other d&d related regressions too 
What I see is that the pre-Threadmanager changes drags didn't block the updates of the content of the firefox window (during page-load etc). After the patch was checked in: the content and all other UI elements get blocked until the drag is ended.
(In reply to comment #10)
What patch exactly in the range of comment 0 do you suspect Ger?
Looks like a thread manager regression. This WFM if I drag the link out of another Fx instance.
Blocks: nsIThreadManager
No longer blocks: 337305
Component: Places → XPCOM
OS: Windows XP → All
Product: Firefox → Core
QA Contact: places → xpcom
Hardware: PC → All
Flags: blocking1.9?
Neil, can you take a look at this?
Assignee: nobody → enndeakin
Flags: blocking1.9a2?
Flags: blocking1.9?
Flags: blocking1.9+
My guess is that the timer isn't being fired during a drag.
Seeing this in Thunderbird as well.
Dragging an email over an collapsed folder will not open the folder.
The timer callbacks only get called if the disabled code in nsBaseAppShell::NativeEventCallback is reenabled, but that causes a significant hit to performance. I really can't say what code should be instead though.

Assignee: enndeakin → nobody
Blocks: 381255
I'm seeing this behavior since Places has been enabled.  The following message appears in the Error Console (Console2):

Error: Cannot modify properties of a WrappedNative = NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN
Source file: chrome://downbar/content/downbaroverlay.js
Line: 192

Here's my current build ID, although I've been seeing it since place was enabled a few days ago on the trunk.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070528 Minefield/3.0a5pre ID:2007052818 [cairo]
Sorry for the bug spam, but I think the error message in <A HREF="">my previous post</A> is caused by the download statusbar extension, and not the bookmarks problem.  However, I'm still having a problem with folders not expanding on the bookmarks toolbar or the bookmarks menu itself.
Neil, reassining to you per the granparadiso meeting. If you can't get to this let me/Damon know and we'll play developer-roulette again ;-)
Assignee: nobody → enndeakin
I've already investigated this comment 16. I really won't be able to understand the thread code in any short timeframe.
(In reply to comment #15)
> Seeing this in Thunderbird as well.
> Dragging an email over an collapsed folder will not open the folder.

That's been filed as bug 338401; I'm assuming it'll get fixed when this does. 

Strange, WFM now.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072504 Minefield/3.0a7pre
Assignee: enndeakin → nobody
I think this is a duplicate of Bug 299185.
Summary: Folder does not open anymore for dragged link → Bookmarks / personal toolbar folder doesn't open for dragged link
(In reply to comment #14)
> My guess is that the timer isn't being fired during a drag.

Yeah, it looks like it isn't being fired until the click is released, instead; at least, that is how the UI is acting in practice. (and sorry if I am stating the obvious, just thought I'd add this for the sake of completion).
Flags: in-litmus?
Flags: blocking1.8.0.14?
Priority: -- → P3
See also bug 338401 comment 17, about "Bookmark Manager".
Blocks: 338401, 381699
Cancelling "suryakrao: blocking1.8.0.14 = ?" as this bug is 1.9 trunk only.
suryakrao, explain if you disagree.
Flags: blocking1.8.0.14?
Blocks: 395176
Bug 407893 is almost a duplicate. I had made an additional observation under that bug which is as follows:
"Similarly, if I expand a folder in the bookmark toolbar and drag an item (from
within that folder) to a different position in the same folder, the item is
dropped 4 items below where the mouse pointer is."

I investigated this a little bit. In Mozilla Firefox 3 Beta 1\chrome\browser.jar\content\browser\places\controller.js in the function _getDropPoint: function TBV_DO_getDropPoint(event):
I find that on dropping a dragged item at a particular entry, I get the following values: = 209 (= a) = 20 (= b)
event.clientY = 250 (= c)
I was expecting that a <= c <= a + b, which is not the case.
also, i found that refers to the correct item. I did so by looking at The dropped item is getting added a couple of items below the mouse pointer because event.clientY is not on at all. Thus comparing event.clientY with nodeY in the function is incorrect. and its properties should be used to identify the target.
But, if we want to find whether mouse is in upper half or lower half, event.clientY is needed which, IMHO, appears to return incorrect value.
Mrinal, this issue seems to be bug 405087.
Someone marked my bug as a duplicate of this one, so I thought I would add that it is impossible for a bookmark to be dragged into a sub-subdirectory, when this was possible in 2.0.11 (I'm running 3b1).
Does not work on:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2007122815 Minefield/3.0b3pre

I can say this problem still exists.
Depends on: 389931
Attached file testcase
The problem is that a popup doesn't open on a dragenter (or other drag event) event.

Now, the places code also uses timers in combination with drag events, so it also suffers from bug 203573, atm. But because of the first mentioned issue, you can't work around this, by not using a timer.

Related bugs for the first mentioned issue: bug 389615, bug 343729.

It seems to me something like in bug 395397 should be done for windows.
Jim, maybe you know how to fix this?
It seems to me that (at least) windows widget code needs to be fixed to allow things like timers and layout still to keep working while a drag is performed inside Mozilla. 
Assignee: nobody → jmathies
Attached patch drag and drop event patch v.1 (obsolete) — Splinter Review
Ok, here’s a patch that addresses this. The main thread is caught up in nsDragService’s DoDragDrop, and doesn’t return until the drop operation finishes. During that call thread events don’t get processed up in nsAppShell. Windows calls back to Fx3 into nsNativeDragTarget, where we process drag events and dispatch them as UI events into the current window as mouse messages. These are then processed by the view, which ultimately calls down into the places code observer where the timer is set. That timer event though is based on an nsRunnable event, so it doesn’t get processed until after DoDragDrop returns. Hence the Bookmark stuff failing but things like tab arrows are successfully drawn within the layout code.

What I did was simply add a call to nsThreadUtil’s NS_ProcessPendingEvents in nsNativeDragTarget so when the timer event fires, it gets processed during the drag operation. That seems to have fixed things and doesn’t appear to be hurting drag operation performance.
Attachment #298333 - Flags: superreview?(roc)
Attachment #298333 - Flags: review?(emaijala)
Just noticed, was this a problem on all platforms or just windows?
If the user stops moving the mouse, events won't be processed, right?

I'm amazed that Windows doesn't run our window event loops during drag. Does it not process messages in *any* window?
Sorry not the native event handler, the event processing that takes place in nsBaseAppShell::Run, which calls NS_ProcessNextEvent(thread). 
Comment on attachment 298333 [details] [diff] [review]
drag and drop event patch v.1

What is the point of adding a space in <nsDragService.cpp> ?
>If the user stops moving the mouse, events won't be processed, right?

Yes, actually it does. Javascript timers in general freeze during drag and drop operations currently. For example you can stop any setTimeout from firing by dragging something out of the browser window and holding it on the dekstop.
I wonder if Windows delivers window messages anywhere where we can grab them and run our event loop normally.
(In reply to comment #27)
> I think this is a duplicate of Bug 299185.

While the symptom seems to be similar/same,
that bug predates the regression(s) caused by bug 326273 (on 20060510),
hence it's not a duplicate of this bug.

(In reply to comment #47)
> Just noticed, was this a problem on all platforms or just windows?

On comment 12, Asaf changed this bug from Windows to All;
yet, in my memory, this regression was Windows specific...
>I wonder if Windows delivers window messages anywhere where we can grab them
>and run our event loop normally.

Ok, I'll do some more poking around and see what I can come up with.
A useful testcase with a little bit of animation on a div.
Comment on attachment 298333 [details] [diff] [review]
drag and drop event patch v.1

Excellent! I believe this will fix quite a few issues. r=me, just don't add that extraneous space into nsDragService.cpp.

Attachment #298333 - Flags: review?(emaijala) → review+
> I wonder if Windows delivers window messages anywhere where we can grab them
> and run our event loop normally.

It does, we have for example a hidden event window that can receive events during the operation.  All other win32 window event procedures also continue to receive events. One experiment I tried was to set a timer in the event window during drag operations to keep event processing occurring which worked. Setting and clearing this from nsDragService was a pain and js timers were still  inaccurate though due to the resolution of the timer. 

Overall this patch still seems like the simplest solution. The only drawback is that event processing will stop if the mouse leaves all moz created windows and remains stationary. Events will get processed if the mouse sits over one of our windows and is stationary as windows continues to send event callbacks in this case. 

Any other suggestions on alternative approaches are welcome. :) I'd be happy to test them out. My experience with our event system / windowing is pretty new so I'd welcome any feedback / ideas.   

> just don't add that extraneous space into nsDragService.cpp.

Sorry, should have caught that. Will clean it up and repost. 

Hang on. If the hidden event window ("nsAppShell:EventWindow") is still able to receive events, why don't Gecko events work? When we post a Gecko event we should be calling nsAppShell::ScheduleNativeEventCallback, which post a message to that window which will give us a chance to process the Gecko events.
There was some code to make sure this was happening, but it created a big perf regression. In my testing I was able to see this, this call was made repeatedly during startup. After startup things seemed to calm down. Without this or some other solution SetNativeCallback is never called during the drag operation.

I think we need to try to make that work without a performance regression instead of hacking in a call during drags. I.e., the real fix to this bug is to fix 338225. If that is fixed, we don't need to do anything here and any fix specific to the drag code will become cruft.
Depends on: 338225
This actual folder opening is fixed by the patch in bug 389931, which fixes the underlying issue on all platforms. 
Blocks: 418088
Attachment #298333 - Flags: superreview?(roc)
I don't see the problem anymore. This bug is fixed, by bug 389931.
Closed: 16 years ago
Resolution: --- → FIXED
And thanks, of course, to everyone who contributed. 
Which build is it working for you?  I still see this in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022604 Minefield/3.0b4pre ID:2008022604
The patch that fixed this bug went in after the 2-26 nightly was built. Either download an hourly tinderbox build or wait until tomorrow's nightly.

For future record, note that comment #69 came at nearly 11AM PST and that your buildid says that it was pulled sometime during the 4AM hour PST (that's what the last 04 in the buildid is), so it was pulled 7 hours before the relevant patch went in.
Thanks for the clarification.  It will be fixed in tomorrow's nightly, then.  I didn't realize that bug resolutions were allowed to be made based on results from hourly builds.  I had always assumed that hourly builds could be too chaotic to definitively report a resolution on a bug.  I apologize for making that assumption, and I am glad this is finally fixed.  Thanks everyone!
I'm still a bit confused. Does this bug only cover the Bookmarks Toolbar? Doing the same for the Bookmarks Menu shows that it doesn't work. The menu doesn't get opened when dragging a bookmark over it. This is reproducable at least under OS X and Windows. Ria, please help. ;) If it only applies to the toolbar folder I can file a new bug if there isn't already one.
Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b4pre) Gecko/2008022702 SeaMonkey/2.0a1pre

A URL dragged from the location bar to "Bookmarks" on the SeaMonkey menu will open bookmarks, but as soon as I try to drag the URL to the location I want to drop it within the list of bookmarks, bookmarks closes. When I drag a URL from the location bar to any folder on the PTF, nothing happens: the folder doesn't open to allow me to drop the URL.
Please file new bugs for drag and drop issues you're still seeing in the latest build.
This bug was a regression from bug 326273 and was fixed by bug 389931.
The issues mentioned in comment 74 and comment 75 are different issues.
Everything I tested works normally. I made some screenshots:
When I release the mouse button it is inserted on the right spot.
WFM as well. Thanks everybody. 

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre
(In reply to comment #76)
> Please file new bugs for drag and drop issues you're still seeing in the latest
> build.

Filed bug 419953.
Comment on attachment 298333 [details] [diff] [review]
drag and drop event patch v.1

Marking obsolete, as it was never checked in.
Attachment #298333 - Attachment is obsolete: true
No longer blocks: 338401
(In reply to comment #73)
> I didn't realize that bug resolutions were allowed to be made based on
> results from hourly builds.

Actually, a bug is usually resolved fixed as soon as the patch is (cvs) checked in.
(It may be reopened later if something goes wrong.)


Comment 74 and comment 75 (and comment 79):
*FF regression: bug 419740.
*SM regression: bug 420341.


V.Fixed, per bug 420341 comment 1.
Depends on: 419740, 420341
Target Milestone: --- → mozilla1.9beta4
No longer blocks: 380301
No longer depends on: 338225
No longer blocks: 418088
No longer blocks: 395176
No longer blocks: 381255 added to Litmus to cover this scenario.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.