This is the place holder for the patch I will attach hopefully within 24hr.

The patch includes: - spring loaded folders (500 ms) - DND allowed with in an arbitrary number of levels. - hovering outside an open menu invalidates its menupopup only after 500ms. - when the mouse stays outside the menus and personal toolbar during more than 500ms, all the menu close. - no more high CPU usage during DND over menus. - folder in personal toolbar is surrounded by a border only if the mouse pointer is over it. - feedback line is always removed after cancel or drop. - inserts the bookmark where the feedback line is. (this is a hack, the method determineDropPosition always returns DROP_AFTER wherever the mouse pointer is due to computational errors in the vertical case) - flavour "text/unicode" and "moz/rdfitem" was missing for in the menuDNDobserver and folderObserver. - drag is allowed from (sub)folders in the PT. - no more global variables, functions specific to the observer are encapsulated. This patch works well with linux after having applied the patch in bug 96504 (http://bugzilla.mozilla.org/attachment.cgi?id=77435&action=view) Unfortunately, under at least Windows ME and Mac 9.x, OSX timeout are not fired during DND. I have tested that the patch does not work in Windows ME but is harmless, and is equivalent to disable DND inside folders and BM button. Claudius, could you apply this patch and let me know if it works under Windows2000, NT and XP, please? I really need to know that in order to evaluate if I need to emulate timeout during dragover so that the feature would be implemented for all platforms. In other words, I also need to know from drivers if DND inside folders are critical for Mozilla 1.0 (or Netscape next release) for Win98 and Mac (assuming that the patch works for Windows2000, NT and XP) As soon as drivers tells me it is a MUST HAVE, I will dedicate time to fix this issue. In the next comment, I will explain how the current patch works.

I have merged the 3 DND observers in navigatorDD.js inside a single one. - no more duplicated code - huge code simplification.(also in navigator.xul) - the logic in the personal toolbar is a tree and there is no structural difference between a leave in the personal toolbar and a folder, nor between folders. Event handling: I have define an dragEnter method in nsDragAndDrop similar to dragExit. - ondragenter: fire a timeout that will load the (sub)menus if necessary. - ondragclose: fire a timeout that will close only necessary (sub)menus according to the element currently selected by the mouse. If the mouse is outside an PT element, all the tree will be closed. - we take advantage of the event bubbling: the event handlers are only defined in the root of the personal toolbar in navigator.xul Wrong drop position: - in nsBookmarkService: createBookmarkWithDetails insert at an dropIndex+1. It is wrong since the first child element has an index of 1, not 0. - when moving bookmarks in the PT, determineDropPosition was calculated wrongly after the removal of the dragged element (after the removal, elements are shifted so that the coordinates of the mouse pointer are no more in sync with the element in which the drop has occurred)

Ere has kindly tested my patch on W2000. and patch v1.0 does not work neither. I have therefore emulated timeout during dragover. Code is not XP: - the Windows/Mac part of the code can not work for Linux, since menu close relies on a the fact that timeouts are fired after that the drop is released. - bonus for the linux user: menus close if the mouse pointer stays outside for more than 500ms. I have tested this patch on linux and WinME, I think it's time for review (btw, I would like to thank also timeless, peterv, rossi, bz, grayrest, Kryptolus, pink and shaver for their help on IRC. my mom, too)

Just removing few left ^M.

For the linux users: 1) apply the patch in bug 96504 (http://bugzilla.mozilla.org/attachment.cgi?id=77435&action=view) 2) open by lclicking the (sub)folders you want to drag links into. (this is due to bug 141203) 3) go

Per discussion with blizzard, this patch disables DND into PT folders and the bookmark button for Mozilla 1.0 on linux. I also removed the debug dumps. This patch is ready to be reviewed. It has been tested on linux and WinME but not on Mac. (If I were told that one day, I would have written a window/mac only patch, I would have not believed it)

Marking nsbeta1+/adt2

Can you attach a -wbu diff?

Ben and Blake, can you two give this the love it so desperately needs. This is a Mozilla stop-ship and needs to land today.

Created isPlatformNotSupported property per jag request and systematic use of if (this.isPlatformNotSupported) return true;

Comment on attachment 82144 [details] [diff] [review] Patch v3.1 (diff -wbu) sr=ben@netscape.com

My bad, I forgot to consider the case where the drop occurs in an empty area of the personal toolbar. I added feedback in this case, also. Finally, I prevent the drop in the few pixels surrounding the hbox and menu/menuitems where the event target is the 'toolbar' or 'menupopup'. Modifications only affect the onDrop, onDragSetFeedBack and onDragRemoveFeedBack methods.

Pierre - Can you take a look at, and chime in on bug 96504. It looks like it is related to this one.

looking for the simpler fix at 96504. removing from 138000 list.

Following Asa's last comment: This bug is still marked as a dependency of bug 138000 even if 138000 has been removed as blocked by it on this page. Is this a bug in Bugzilla or is it normal behavior? It should perhaps also be removed from the bug 138000 page.

Being annoyed by Bookmarks always appearing where I didn't expect them after drag-and-drop (in Personal toolbar and in "Manage Bookmarks" - mostly they appear one line above from where I placed them - this is one of my most hated bugs nowadays) I looked around in bugzilla - and found a real jungle (including this bug here) of open bugs describing the same: bug 114256 (bookmark gets into wrong folder after dnd) although related, this particular aspect is fixed now, but at least one of the dups is more general and not adressed yet (bug 114878). bug 114606 (bookmark goes to position above) describes exactly what I see and hate, but in the meanwhile this bug has been somewhat changed into a tracking bug and now also adresses other bugs related to bookmark d'n'd. (however, component is not changed to "Tracking") Many bugs that also describe the exact behaviour are duped to this one (I'll list them later) because of its desription, but now nobody cares anymore about the described problem because this one is now considered a tracking bug by some people... bug 119879 (bookmark appears one line above after dnd) is 100% what I see and hate. bug 127437 (wrong position after dnd in personal toolbar) is about the same problem just at another place (still mostly off-by-one after dragging bookmark). Has *lots* of duplicates. bug 137119 (bookmark positioned one line too high after dragging) is about bookmark manager again, still the same problem. Was duped against bug 114606 already (same summary) but later reopened (bug 114606 is supposed to be tracking). bug 139471 is finally about general dnd cleanup and is also supposed to fix this particular issue I'm talking about. Very recently worked on, but now unfortunately removed from RC2 list. So it looks like (almost!) all of these six open bugs are duplicates of one another, but correct duping and resolving was prevented by halfheartedly changing bug 114606 into a tracking bug. In addition the underlying bug I speak about (bookmark d'n'd results in wrong position) was reported in many other bugs which are now duped to one of the six bugs above: bug 115420, bug 116076, bug 117134, bug 120245, bug 122599, bug 130702, bug 137119, bug 130453, bug 130836, bug 132390, bug 134656, bug 137117, bug 140071, bug 140342 and bug 140971 are the ones I found. As you can see from the dup count or simply from imagining a browser where bookmarks do not materialize where they are dropped, this is a quite serious problem (and in my humble opinion should not go into 1.0). I think it would be a rewarding task for someone with the appropriate permissions to clean up this jungle a bit. Maybe by clearly (and correctly) labeling a general tracking bug and a separate one for the one particular d'n'd problem I'm talking about (and duping the others against it). This cleanup would get us rid of several ASSIGNED and NEW bugs and prepare the way for fixing this one annoying d'n'd issue. Bug 139471 unfortunately was taken off the RC2 list (bug 138000) by Asa because of its complexity. A simple "fix this one bug" wouldn't have been, I think. BTW: yes, this problem also occurs with 2002050706 (branch) and 2002050708 (trunk).

Andreas, you have apammed this (long) comment in at least four bug reports. Please, don't do that again. The issues covered by the six bugs you mentionned relate to different parts of the code and should be kept in separate bug entries. Since RC2 is really close, big patches should not be checked in it, but should be included for the next milestone.

In fact I'm very sorry that some people got this comment more that one time, and (although intended) I forgot to include a big "sorry" about that. But the cc lists of the affected bugs look very different, in fact only jnkg and blaker appear more than once (twice). On the other hand I wanted to reach all the people who care about this problem and therefore have included themselves in cc lists. You (and ben) as developers got this comment several times although not on cc lists and I'm again sorry for that. But on to topic: Bug 114606: "When relocating a bookmark in a folder on the end of the folder, it goes not on the end but one position above." [...] - "I can get the same results for all/most bookmarks, not just the bottom." Bug 137119: "Actual Results: Bookmark ends up a line to high, Expected Results: Bookmark should end up at the place it was dragged to." Bug 119879: "dropped bookmarks are inserted in incorrect position" [given from the description: one position too high] At least these three sound exactly identical. Don't they? So I think I *did* some work, not just spam... And I do not know the code like you do, but isn't the personal toolbar just another bookmarks folder? If yes, bug 127437 is quite the same: "dragging and dropping bookmarks in the personal toolbar puts them one place to the right of where they should be." Again sorry if I'm totally wrong and these are different bugs in the source, but at least one can't tell it from the description. Whatever, I'll keep quiet now. I just hoped to point you to three or four duplicates and one bug (bug 114606) that some people see as a tracking bug although its component is *not* properly set to "Tracking" and its description only describes one special aspect (so it's quite confusing)...

Andreas: what you did was wrong, what's more, you should not have defended yourself in the bug, your defense poured salt on the wound by adding yet another unnecessary comment to a bug where a developer was actively developing. according to mozilla this bug is now 6 pages long, of which i'd estimate you are responsible for 1 1/2 pages (and 1 of the others is blank) -- that's a big portion of a serious bug report. In the future if you feel the need to point out duplicates in long form, please contact the bug qa contacts directly. Just because the cc list doesn't overlap doesn't give you carte blank to spam lots of people. Bugzilla supports comment references, so if you absolutely had to (and your actions here *do not* qualify) you could have used a bug reference such as me saying that I agree with Bug 139471 Comment 19. I am pretty sure I got a bugmail for each comment you made, and believe me I was no happier than Pierre. But the bigger problem is that any developer, qa contact, or helper interested in helping fix, review or appraise this bug will have to read your comment and any complaints about your comment. Ok, that said and done, i'm going to try to review the patch. -- this is my third attempt at a comment, the last two attempts resulted in me file>closing instead of file>print previewing. -- ok i've started to review. I think pch will attach a new patch based on my irc reviews and then we'll go from there. -- commenting now to avoid conflicts :-).

New patch accounting for the preliminary remarks from timeless. - adding isContainer() and isInDragRegion() helper functions - one indentation - use of DROP_ON instead of 0 in determineDropPosition (I haven't touched this function) - fixed some comments In addition: - fixed a strict javascript error - set springLoadedMenDelay to 250ms instead of 500ms, but it should be tuned. - remove the return true|false where it was not necessary. Reviewer: this is what this fix does not do: - insertion before and after containers (bug 101059) - choice between dragging before and after in menu (determineDropPosition always returns DROP_BEFORE) - opening menus during DND on the linux platform (see bug 96504). - correct handling of the drop on the bookmark toolbar button (bug 135983). Remaining issues that imho should deserve other bugs: - when the mouse leaves quickly menu||menuitem, dragExit events are not fired. This is an event handling problem and I can not do much to fix it. This leads the feedback line not to be removed when dragging outside the menupopups. Anyway, this feedback line won't appear anymore after the drop. - once they are opened, menus will close only after the drop unless of course the user has dragged over the PT in a way that closes it. I think that given the fact that dragExit is not always fired, the only solution would to to bloat the outsider DND observers (layout, ...) I would prefer a fix for the event handler. - I have left the following feature: when a menu is already open, the user can drag one of its items. I have not modified the previous behaviour: the menu will not close. However, this feature combined with the dragExit handler bug, may lead to feedback line not removed. This feature is ok when the DND occurs in the same container, but is surprizing when the drop occurs in another container (source menu closes and target menu still open). I need UE feedback.

More cleanup concerning how the events bubble. I included Jag's comment: start drag for bookmark-button is now prevented on all platform at the navigator.xul level. However, for the other containers, under linux, bug 143031 will reappear for the containers in the personal toolbar as soon as we decide to allow dragging them. I added more comments too. Patch is now really ready for review. GOOD NEWS!!! DND inside menupopups does not fail on linux! with today's trunk, XUL fast load enabled, the assertion described in bug 141203 is no more triggered. Since I need to modify this patch, I will enable this feature on linux in another bug.

some comment modifications + optimization concerning getTime after timeless review.

I will attach a new patch, since the patch in bug 142113 enables timers during drag and drop for the windows platform.

The fix in bug 142113 did not enable timers during DND as I expected. In this patch, there is nothing new for the Windows platform except a silly thingie I left for the Home Observer (the latter is no more modified). For the platforms that support timer during DND, I have copied the part from the initital patch. There are now two distinct implementations: one uses timers and the other emulates them. The reason is that - we have a cleaner implementation when using timers directly. - I still debug in Linux (it supports timers during DND), It's a pain for me to debug in linux then revert the change for a Windows only patch. After a short inspection of nsWindow.cpp, I have hope to enable the whole feature for linux. At this moment, it is working 80%, but the current patch disables it completely. - I am sure that platforms other than linux support timers during DND. BeOS? QNX? OS/2? It will be simple to modify this code accordingly to each platform. The only difference seen by the user between the two implementations is that for Mac/Windows, if the mouse is hovering during a DND outside an spring loaded menu, the latter won't close automatically. But this implementation CAN NOT work for platform that support timers during DND. In the latter, the menu will close after a short delay cleanly and automatically.

i tested v3.5 on winXP, without the cpp part. everything works fine. i only get one javascript strict warning at startup which pch will fix.

Comment on attachment 83806 [details] [diff] [review] Patch v3.5 r=ben@netscape.com

Ben, I am sorry for the confusion. In my last mail, I just asked you review bug 145350) I fixed the warning errors dicovered by Rossi. I also introduced a canDrop method (old isInDragRegion method) that is handled by nsDragAndDrop.js (bug145350) It means that mouse pointer feedback is handled for free. Also, a drop on the bookmark button append a item at the bookmark list and it is no more possible to drop in IE favorite folder and find: bookmarks.

this patch should not be checked in before bug 145350.

This patch makes me cry, it is just so good. I wish we could get this on the branch! I have applied the patch in my tree and am testing it and reviewing it now.

I sure hope it is headed for the branch real soon!

Pls. re-record r= and sr= stamps, mail drivers -- this should go in for 1.0.1. /be

Argh... My bad, since I am using a laptop, I've left a serious UI bug for the user using a mouse. I enabled the drag of containers on simple drag gesture. It means that lclick down, hold, then release to select the bookmark will drag the container. I will attach a fix today.

Here is the fix: if (this.isContainer(aEvent.target) && aEvent.target.getAttribute("group") != "true") { if (this.isPlatformNotSupported) return; + if (!aEvent.shiftKey && !aEvent.altKey) + return; // PCH: menus open on mouse down aEvent.target.firstChild.hidePopup(); } I added the shift modifier since on linux, alt is usually used to move the window. I would have preferred to find another way to drag the containers without a key modifier, based on the orientation of the drag gesture (moving the mouse: delta(y) >=0 upwards would trigger the drag) but I fear that mGestureDownPoint in http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1273 is not accessible in js. I will attach a new patch after the reviewer comments.

In the very final version, I will also do: - // PCH: menus open on mouse down + // menus open on mouse down since this is the designed behavior.

patch incorporating modifications mentionned above. r=timeless was pending Boris 'Pi' testing. Thanks a lot Boris!

Patch OK in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0+) Gecko/20020527. pi

*** Bug 148013 has been marked as a duplicate of this bug. ***

Comment on attachment 85073 [details] [diff] [review] patch for the trunk sr=ben@netscape.com

fixed on the trunk. qawanted for all platforms including Sun, Mac, OS/2, QNX etc... This patch has only been tested on linux and windows. We should determine two attributes (in personalToolbarDNDObserver in navigatorDD.js) for each platform: - if the opening of a menu during DND is possible (isPlatformNotSupported) - if timers fire during drag and drop (isTimerSupported) by default, for platforms other than linux, mac and win, isPlatformNotSupported is set to false and isTimerSupported to true. If DND in folders is not working, please set isTimerSupported to false, retest and if it fails, set isPlatformNotSupported to true (as set for linux) And tell me your findings so that we can give the right values for each platform.

Nominating for branch & removing vestigial ETA.

If I'm dragging a bookmark around the Bookmarks menu off the personal toolbar, I can't drop it between two folders. Either one folder or the other springs open. Is this new from this bug or an old problem?

Dean, this is bug 101059. I will work on it soon. I forgot to mention that the list of the remaining issues are in bug 133604

Claudius, can you verify this on the trunk?

adding adt1.0.1- per ADT.

Damn, wrong tab, sorry for the SPAM.

This has a LOT of baking on the trunk. Another bug (depends on this one, but should be a dup) was nominated for 1.0.2. I suggest we take this for 1.0 branch. Will the patch apply cleanly?

I think this patch would apply after backing out three branch-only band-aid patches (bug 143031, bug 96504 and bug 133601) that were fixed on trunk by the current patch. There is also a need to update it because of two small regressions that it caused (bug 150749 and bug 148597) I could update this patch for the m1.0 branch, but unfortunately, I really don't have a lot of time this month. - will the patch from Bolian in bug 148996 be approved? (enabling the feature on the linux platform) - I won't have a branch cvs build but I can hack the chrome - I am ok to be on the hook to fix the eventual regressions, but not to do the QA. - if r/sr is still needed even if the patches have already been r/sr'd, I won't have time to ask r/sr again. I don't know the policy. - it would be good also to take the patch from bug 101059 (enables drop at the bottom of the menupopup) If m1.0.2 is expected in one month, I would prefer to wait when I will have more time. But obviously, s.o. else can do the work and would be glad to help him!

*** Bug 186580 has been marked as a duplicate of this bug. ***

rs vrfy.