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

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Ria Klaassen (not reading all bugmail)]

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.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-05-10+09%3A00&maxdate=2006-05-10+22%3A00

So maybe bug 337305 has done this.

Comment 1

[Peter Lairo]

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?

Comment 2

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #1)
> 
Indeed. Main folders don't expand so you can't open sub-folders as well.

Comment 3

[Ger Teunis]

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.

Comment 4

[Ger Teunis]

Could this be a result of bug #203573 ?
Which addresses the UI freeze of firefox during dragging.

Comment 5

[Serge Gautherie (:sgautherie)]

This can not be bug 203573, as there was no patch submitted there (yet) !

Comment 6

[Ger Teunis]

(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.

Comment 7

[Ria Klaassen (not reading all bugmail)]

Bug 203573 was filed years before places was born and this function only regressed after 10 May 2005, like described in comment 0.

Comment 8

[Ria Klaassen (not reading all bugmail)]

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?

Comment 9

[TimoT]

if bug 326273 fits into regression range it seems like a likely candidate, it caused other d&d related regressions too 

Comment 10

[Ger Teunis]

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.

Comment 11

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #10)
> 
What patch exactly in the range of comment 0 do you suspect Ger?

Comment 12

[Asaf Romano (gone)]

Looks like a thread manager regression. This WFM if I drag the link out of another Fx instance.

Comment 13

[Benjamin Smedberg]

Neil, can you take a look at this?

Comment 14

[Neil Deakin]

My guess is that the timer isn't being fired during a drag.

Comment 15

[Ger Teunis]

Seeing this in Thunderbird as well.
Dragging an email over an collapsed folder will not open the folder.

Comment 16

[Neil Deakin]

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.
 
 

Comment 20

[Weston Carloss]

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]

Comment 21

[Weston Carloss]

Sorry for the bug spam, but I think the error message in <A HREF="https://bugzilla.mozilla.org/show_bug.cgi?id=337761#c20">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.

Comment 22

[Benjamin Smedberg]

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 ;-)

Comment 23

[Neil Deakin]

I've already investigated this comment 16. I really won't be able to understand the thread code in any short timeframe.
 

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

(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. 

Comment 26

[yagood]

Strange, WFM now.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072504 Minefield/3.0a7pre

Comment 27

[mmortal03]

I think this is a duplicate of Bug 299185.

Comment 30

[mmortal03]

(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).

Comment 32

[Serge Gautherie (:sgautherie)]

See also bug 338401 comment 17, about "Bookmark Manager".

Comment 33

[Serge Gautherie (:sgautherie)]

Cancelling "suryakrao: blocking1.8.0.14 = ?" as this bug is 1.9 trunk only.
suryakrao, explain if you disagree.

Comment 36

[lazierthanthou]

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:
event.target.boxObject.y = 209 (= a)
event.target.boxObject.height = 20 (= b)
event.clientY = 250 (= c)
I was expecting that a <= c <= a + b, which is not the case.
also, i found that event.target refers to the correct item. I did so by looking at event.target.label. The dropped item is getting added a couple of items below the mouse pointer because event.clientY is not on event.target at all. Thus comparing event.clientY with nodeY in the function is incorrect. event.target 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.

Comment 37

[Ria Klaassen (not reading all bugmail)]

Mrinal, this issue seems to be bug 405087.

Comment 39

[Harry]

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).

Comment 40

[Douglas Calvert]

Does not work on:

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

Comment 42

[Michael Adams]

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.

Comment 43

[Martijn Wargers (dead)]

Attachment: 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.

Comment 44

[Martijn Wargers (dead)]

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. 

Comment 46

[Jim Mathies [:jimm]]

Attachment: drag and drop event patch v.1

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.

Comment 47

[Jim Mathies [:jimm]]

Just noticed, was this a problem on all platforms or just windows?

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 49

[Jim Mathies [:jimm]]

Sorry not the native event handler, the event processing that takes place in nsBaseAppShell::Run, which calls NS_ProcessNextEvent(thread). 

Comment 50

[Serge Gautherie (:sgautherie)]

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> ?

Comment 51

[Jim Mathies [:jimm]]

>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.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I wonder if Windows delivers window messages anywhere where we can grab them and run our event loop normally.

Comment 53

[Serge Gautherie (:sgautherie)]

(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...

Comment 54

[Jim Mathies [:jimm]]

>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.

Comment 55

[Jim Mathies [:jimm]]

Attachment: test settimeout html

A useful testcase with a little bit of animation on a div.

Comment 56

[Ere Maijala (slow)]

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.

--Ere

Comment 57

[Jim Mathies [:jimm]]

> 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. 

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 60

[Jim Mathies [:jimm]]

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.

http://lxr.mozilla.org/mozilla/source/widget/src/xpwidgets/nsBaseAppShell.cpp#89

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 66

[Neil Deakin]

This actual folder opening is fixed by the patch in bug 389931, which fixes the underlying issue on all platforms. 

Comment 69

[Ria Klaassen (not reading all bugmail)]

I don't see the problem anymore. This bug is fixed, by bug 389931.

Comment 70

[Ria Klaassen (not reading all bugmail)]

And thanks, of course, to everyone who contributed. 

Comment 71

[mmortal03]

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

Comment 72

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 73

[mmortal03]

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!

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 75

[Andy Boze]

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.

Comment 76

[Martijn Wargers (dead)]

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.

Comment 77

[Ria Klaassen (not reading all bugmail)]

Everything I tested works normally. I made some screenshots:
http://img166.imageshack.us/img166/6108/dragdrop1ue2.jpg
http://img249.imageshack.us/img249/2189/dragdrop2md8.jpg
When I release the mouse button it is inserted on the right spot.

Comment 78

[Ale Mand]

WFM as well. Thanks everybody. 

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre

Comment 79

[Andy Boze]

(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 80

[Serge Gautherie (:sgautherie)]

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

Marking obsolete, as it was never checked in.

Comment 81

[Serge Gautherie (:sgautherie)]

(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.

Comment 82

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5275 added to Litmus to cover this scenario.