139471 - navigatorDD.js cleanup

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Pierre Chanial]

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

Comment 1

[Pierre Chanial]

Attachment: patch v1.0 (with debug dumps)

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.

Comment 2

[Pierre Chanial]

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)

Comment 3

[Pierre Chanial]

Attachment: patch v2.0 (with debug dumps)

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)

Comment 4

[Pierre Chanial]

Attachment: Patch v2.01 (with debug dumps)

Just removing few left ^M.

Comment 5

[Pierre Chanial]

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

Comment 6

[Pierre Chanial]

Attachment: Patch v3.0

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)

Comment 7

[Peter Trudelle]

Marking nsbeta1+/adt2

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Can you attach a -wbu diff?

Comment 9

[Pierre Chanial]

Attachment: Patch v3.0 (diff -wbu)

Comment 10

[Asa Dotzler [:asa]]

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.

Comment 11

[Pierre Chanial]

Attachment: Patch v3.1

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

Comment 12

[Pierre Chanial]

Attachment: Patch v3.1 (diff -wbu)

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 82144 [details] [diff] [review]
Patch v3.1 (diff -wbu)

sr=[e-mail]

Comment 14

[Pierre Chanial]

Attachment: Patch v3.2

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.

Comment 15

[Jaime Rodriguez, Jr.]

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

Comment 16

[Asa Dotzler [:asa]]

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

Comment 17

[Jerome Lacoste]

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.

Comment 18

[Andreas Kunz]

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

Comment 19

[Pierre Chanial]

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.

Comment 20

[Andreas Kunz]

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

Comment 21

[timeless]

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

Comment 22

[Pierre Chanial]

Attachment: Patch v3.3

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.

Comment 23

[Pierre Chanial]

Attachment: Patch v3.4

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.

Comment 24

[Pierre Chanial]

Attachment: Patch v3.41

some comment modifications + 
optimization concerning getTime after timeless review.

Comment 25

[Pierre Chanial]

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

Comment 26

[Pierre Chanial]

Attachment: Patch v3.5

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.

Comment 27

[rossi]

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 28

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 83806 [details] [diff] [review]
Patch v3.5

r=[e-mail]

Comment 29

[Pierre Chanial]

Attachment: last one

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.

Comment 30

[Pierre Chanial]

this patch should not be checked in before bug 145350.

Comment 31

[Joe Hewitt (gone)]

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.

Comment 32

[Peter Trudelle]

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

Comment 33

[Brendan Eich [:brendan]]

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

/be

Comment 34

[Pierre Chanial]

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.

Comment 35

[Pierre Chanial]

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.

Comment 36

[Pierre Chanial]

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.

Comment 37

[Pierre Chanial]

Attachment: patch for the trunk

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

Comment 38

[Boris 'pi' Piwinger]

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

pi

Comment 39

[Boris 'pi' Piwinger]

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

Comment 40

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 85073 [details] [diff] [review]
patch for the trunk

sr=[e-mail]

Comment 41

[Pierre Chanial]

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.

Comment 42

[Peter Trudelle]

Nominating for branch & removing vestigial ETA.

Comment 43

[:deanis74]

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?

Comment 44

[Pierre Chanial]

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

Comment 45

[scottputterman]

Claudius, can you verify this on the trunk?

Comment 46

[scottputterman]

adding adt1.0.1- per ADT.

Comment 47

[HJ]

http://bugzilla.mozilla.org/show_bug.cgi?id=139471

Comment 48

[HJ]

Damn, wrong tab, sorry for the SPAM.

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 50

[Pierre Chanial]

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!

Comment 51

[Sander]

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

Comment 52

[sairuh (rarely reading bugmail)]

rs vrfy.