201854 - Can't drag/move bookmark from/to/within overflow/dropdown/chevron list of personal toolbar

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Hideo Oshima]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030412
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030412

A '>>' appears when meny bookmarks are on the personal toolbar.
You can't drag any bookmark in the dropdown list that display 
when you click '>>'.

Reproducible: Always

Steps to Reproduce:
1.Regist many bookmarks onto the personal toolbar.
2.Click '>>' on the right side of the personal toolbar.
3.Dropdown list will be appear.
4.Drag a bookmark of the dropdown list.

Actual Results:  
Can't drag any bookmark from dropdown list of the personal toolbar.

Expected Results:  
Can drag any bookmark.

Comment 1

[Andreas Kunz]

This might be bug 151336 - if the dropdown is implemented just like a folder.

cc'ing Jan Varga to decide. Thanks.

Comment 2

[baffclan]

Reproduced with 2003041709-trunk/Win98.

A bookmark in ">>" folder can drag and drop tabbar.
But, cannot drop folder on personal toolbar.

Windows Build have not Bug 151336.

Comment 3

[Hideo Oshima]

Changing OS to All...

Comment 4

[Steffen Clüver]

same trouble with v1.5 - 20031007 - Win2000
sc

Comment 5

[Alex Hirzel]

Seing this in Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6) Gecko/20040113

Comment 6

[Andreas Kunz]

Confirming with current build on Win2k.

No JS error when moving inside the overflow menu, following error when dragging
to the personal toolbar:
Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIRDFContainer.Init]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://communicator/content/bookmarks/bookmarks.js :: anonymous :: line 1316"
 data: no]
Source File: chrome://communicator/content/bookmarks/bookmarks.js
Line: 1316

Comment 7

[Andreas Kunz]

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

Comment 8

[Andreas Kunz]

Attachment: (Av1) work in progress

This allows drag and drop from, to and inside the chevron/overflow menu.

Problem left: when dragging to or from that list its display is not updated
completely: the dragged item is removed/added properly, but the topmost item
that e.g. has space on the toolbar now is still _also_ visible in the overflow
menu - until it's closed and reopened.

And this seems to break dragging items from outside the Personal Toolbar to the
PT, so it is really work in progress only.

Comment 9

[Andreas Kunz]

Attachment: (Av1b) better patch, almost fully working

This actually works and breaks nothing (cause was a bad and ugly if clause
which is changed to something much better in this patch). 
The overflow menu is now drag 'n drop enabled and works well with all kinds of
drag sources and targets supported by bookmarks.

Comment 10

[Andreas Kunz]

Comment on attachment 140381 [details] [diff] [review]
(Av1b) better patch, almost fully working

Requesting pch's opinion on this.

Comment 11

[Andreas Kunz]

One problem I noticed: when Mozilla is freshly loaded and the chevron has never
been opened, it seems to be uninitialized in a sense that it does not have
children yet.

This will cause an error in updateOverflowMenu when doing 
if (menu.hidden == button.collapsed)
because menu is one of the children...
This is triggered by any drop on the personal toolbar.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 140381 [details] [diff] [review]
(Av1b) better patch, almost fully working


'r=?': (see comment 11)

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> One problem I noticed: when Mozilla is freshly loaded and the chevron has never

Should this be fixed in this bug, or is it a different/related issue for another
bug report ?

*****

I wonder if you would have a similar patch for dragging a URL from the URL bar
to a PT folder that has up/down arrow due to too many entries to display:
currently, the mouse change to "forbidden" instead of scrolling the folder.
Workaround is to first scroll the folder, then drag the URL.

(that could be(come) another bug)

Comment 14

[Andreas Kunz]

Comment on attachment 140381 [details] [diff] [review]
(Av1b) better patch, almost fully working

I'm currently unlikely to have enough time to solve the problem in comment #11.
Anybody is invited to do so; I'm sceptical about requesting review for this
patch because I think this issue should be fixed first. But after all *I* did
not request it and maybe jag will have an idea how to solve this best...

Comment 15

[jag (Peter Annema)]

I think there's a way to detect that the overflow popupmenu is currently open,
it would suffice to only call updateOverflowMenu in that case instead of always.
When the menu is closed, it'll get updated next time someone opens it. When it's
open, you won't have the problem of its content not existing.

Except I can't seem to find a way to do it. You could of course use
onpopupshowing and onpopuphiding and keep track of it that way, but that seems
rather ugly.

To work around those warnings, there is a menugenerated attribute on the
popupmenu, so you could call updateOverflowMenu only when that attribute is
true. It's not quite as clean as only calling it when the menu is open, but it's
better than nothing I guess...

CC'ing neil, in case he has an idea of how to improve this.

Comment 16

[Andreas Kunz]

Attachment: fully working patch

Ok, this patch is now 100% working and avoids that JS error mentioned above by
checking if the overflow menu has already children.

I did this by checking:
   if (chevronPopup.firstChild) {
while other possible solutions include:
   if (bb.getAttribute("menugenerated") == "true") {
(as suggested by jag) and:
   if (bb.childNodes.length > 0) {

I think my check is at least as logical and simpler, but if another one is
considered better, I'll happily exchange that one line.

Comment 17

[Andreas Kunz]

Comment on attachment 147673 [details] [diff] [review]
fully working patch

Requesting r= from jag.

I think this would be really nice to have in 1.7. I know it's already a bit
late, but maybe...

Comment 18

[jag (Peter Annema)]

Comment on attachment 147673 [details] [diff] [review]
fully working patch

Your check seems fine, though I'm not sure the resizeFunc() should be inside
it. Though I guess if there's no firstChild you can only possibly be dragging
from one place on the toolbar to another place, which shouldn't cause a need
for us to call resizeFunc().

I wonder though, do you need to call resizeFunc() at all? It looks to me as if
that already gets called whenever I move a bookmark around (through the
onAssert and onUnassert handlers).

Requesting r= from Neil

Comment 19

[Andreas Kunz]

(In reply to comment #18)
> I wonder though, do you need to call resizeFunc() at all? It looks to me as if
> that already gets called whenever I move a bookmark around

Commenting out that line causes bad things (visually): dragging a bookmark
within the overflow menu makes it vanish (in most cases), dragging a bookmark
into it will have no effect at all and dragging items out of it to the PT will
make another bookmark disappear (the one pushed out of the PT). It's all only a
visual problem, upon reopening of the overflow menu everything is there again.

Checking directly in Venkman is a bit confusing: setting a breakpoint in
resizeFunc shows it is run at some point of time, but setting a second
breakpoint where I used to call resizeFunc causes the first breakpoint not being
hit anymore.
Replacing the first breakpoint with one at the very end of the file at:
   setTimeout(BookmarksToolbar.resizeFunc, 0, null);
(called by on(Un)Assert) shows it is called within 
   BookmarksUtils.moveSelection(...)
or, more exactly, within 
   doTransaction: function ()
of BookmarkMoveTransaction (there are several!) in bookmarks.js.
So it's probably just too early to be useful in this case...

Adding "alert" statements as a means of debugging indicates that resizeFunc is
even called twice during the various stages of moving a bookmark, with different
data (even differing number of nodes in the PT), only the third call, introduced
with my patch, gets it right.

So in short: Yes, I need to.

Comment 20

[[e-mail]]

Comment on attachment 147673 [details] [diff] [review]
fully working patch

>-          <menupopup onpopupshowing="if (event.target == this) BookmarksToolbar.updateOverflowMenu(this);"/>
>+          <menupopup id="bookmarks-chevron-popup"
>+                     onpopupshowing="if (event.target == this) BookmarksToolbar.updateOverflowMenu(this);"/>
I don't think you'll need this, see below.

>     var parent           = this.getBTContainer(aNode);
>+    if (parent == "bookmarks-chevron")
>+      parent = "NC:PersonalToolbarFolder";

>         parent = this.getBTContainer(aNode);
>+        if (parent == "bookmarks-chevron")
>+          parent = "NC:PersonalToolbarFolder";
Why not fix getBTContainer?

>+    var chevronPopup = document.getElementById("bookmarks-chevron-popup");
>+    if (chevronPopup.firstChild) {
>+      BookmarksToolbar.resizeFunc(null);
>+      BookmarksToolbar.updateOverflowMenu(chevronPopup);
>+    }
Strangely enough I believe that the when the popup is open the chevron should
have an open="true" attribute :-) What's easier to test is that setting the
open="true" attribute should open the popup ;-)

Comment 21

[Andreas Kunz]

Attachment: patch

Review comments addressed; id is still needed for updateOverflowMenu.

Comment 22

[Andreas Kunz]

Attachment: patch

oops

Comment 23

[Andreas Kunz]

Comment on attachment 147837 [details] [diff] [review]
patch

Getting it on Neil's list.

Due to bug 151336 this will not work on Linux and it is disabled on Mac by
intention:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/boo
kmarksMenu.js#334

Comment 24

[jag (Peter Annema)]

Hmm, so I suspect resizeFunc gets called (or should get called) from onAssert,
onUnassert... Though I wonder how onMove plays into this... I'm surprised that
the onAssert call doesn't give you the right information. Could you try putting
the resizeFunc() call (or rather the timeout helper function call) in onMove
(see onAssert and onUnassert) and see if you can then remove the resizeFunc()
call? Though I guess at that point we'll also have to update the popup menu from
that timeout iff it's open.

Comment 25

[[e-mail]]

Comment on attachment 147837 [details] [diff] [review]
patch

This patch does give the chevron the same behaviour as pt bookmark folders; I
discovered that my theme causes folder dnd to fail and/or crash :-(

Comment 26

[Andreas Kunz]

Comment on attachment 147837 [details] [diff] [review]
patch

> Could you try putting the resizeFunc() call (or rather the timeout
> helper function call) in onMove and see if you can then remove the 
> resizeFunc() call?

I tested, but I can't remove. (onMove isn't even called, if we can trust
venkman)

Requesting sr= from Jag.

Comment 27

[jag (Peter Annema)]

Comment on attachment 147837 [details] [diff] [review]
patch

sr=jag

Comment 28

[Andreas Kunz]

Comment on attachment 147837 [details] [diff] [review]
patch

Requesting approval for 1.7.
This is pretty low risk and looks like really nice to have there.

Comment 29

[Andreas Kunz]

Fix checked into trunk by timeless on 2004-05-12 13:23.
Leaving open for branch...

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 147837 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.7

Comment 31

[Andreas Kunz]

Also checked into 1.7 branch by timeless on 2004-05-12 15:01.
-> FIXED
Thanks to all! 
Don't think I made this patch for any other reason than to make organizing my
own bookmarks easier! :-)

Comment 32

[baffclan]

I cannot reproduce 2004051319-trunk/WinXP-JA.

thanks for fixing this.
however, a dropdown list does not close.

Comment 33

[Andreas Kunz]

> however, a dropdown list does not close.

baffclan, could you please describe this problem more detailed?

Comment 34

[baffclan]

Andreas Kunz :

overflow list in personal toolbar
1. click a '>>', dropdown list is open.
2. drag/drop a bookmark from doropdown list to personal toolbar.
3. a dropdown list is open.

folder in personal toolbar
1. click a folder in personal toolbar, dropdown list is open.
2. drag/drop a bookmark from this folder to personal toolbar.
3. a dropdown list is close.

I think, should close a dropdown list.

Comment 35

[Andreas Kunz]

baffclan:
Ok, I see. Bug 243692 filed for it, patch is coming.
Thanks for spotting this inconsistency.

Verifying per comment #32 and own testing.

Comment 36

[Andreas Kunz]

If you try to drag to the overflow menu _before_ you did any other PT drag
action, it will not open. See bug 243961.

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 147837 [details] [diff] [review]
patch

>Index: xpfe/components/bookmarks/resources/bookmarksMenu.js
>===================================================================
>@@ -112,6 +112,8 @@
>       break;
>     case "bookmarks-chevron":
>       parent = "NC:PersonalToolbarFolder";
>+      item = document.getElementById("bookmarks-ptf").lastChild;
>+      aOrientation == BookmarksUtils.DROP_AFTER;

The '==' sign seems odd:
Is the line useless, or should it read '=' instead ?

>       break;
>     default:
>       if (aOrientation == BookmarksUtils.DROP_ON)

Comment 38

[[e-mail]]

(In reply to comment #37)
>(From update of attachment 147837 [details] [diff] [review])
>>+      item = document.getElementById("bookmarks-ptf").lastChild;
>>+      aOrientation == BookmarksUtils.DROP_AFTER;
>The '==' sign seems odd:
>Is the line useless, or should it read '=' instead ?
Well, it looks as if neither line is necessary, as the default is that the bookmark drops on to the personal toolbar folder as if it was empty space.

Comment 39

[Andreas Kunz]

Attachment: (Bv1) remove two unnecessary lines

You are both so right. Sometimes even six or more eyes do not see the obvious...

I removed the lines and tested that everything (in this case: dropping a bookmark on the chevron itself) works as before.

Requesting r= from

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) <bookmarksMenu.js>

Looking at this file a little more, I think that we could merge a few more lines;
and fix some space nits.

Comment 41

[Serge Gautherie (:sgautherie)]

Neil, you may do the review too...

Comment 42

[[e-mail]]

Comment on attachment 215814 [details] [diff] [review]
(Bv2) <bookmarksMenu.js>

>-      item = document.getElementById("bookmarks-ptf").lastChild;
>-      aOrientation == BookmarksUtils.DROP_AFTER;
How is this supposed to work? I don't see these getting set anywhere else.

Comment 43

[Serge Gautherie (:sgautherie)]

I don't know anything more than what was written in comments 38, 39 and 40, and that andreas's previous patch (= theses 2 very lines) got db48x's r+...

Comment 44

[[e-mail]]

Trying with and without Serge's patch, I've discovered that dragging onto the empty space itself doesn't actually work despite the original code to find the last visible item because the orientation is still DROP_ON and not DROP_AFTER.

Comment 45

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2a-XPFE) <bookmarksMenu.js>

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060514 SeaMonkey/1.1a] (nightly) (W98SE)

Bv2, with comment 44 suggestion(s).

NB: The solution is taken from the FireFox file.

Comment 46

[[e-mail]]

Comment on attachment 222069 [details] [diff] [review]
(Bv2a-XPFE) <bookmarksMenu.js>

Thanks for looking into the fix for this. A quick visual review:

>     if (target.localName == "menu"                 &&
>-        target.parentNode.localName != "menupopup")
>-      return BookmarksUtils.DROP_ON;
>-    if (target.id == "bookmarks-ptf" || 
>-        target.id == "bookmarks-chevron") {
>+        target.parentNode.localName != "menupopup" ||
>+        target.id == "bookmarks-chevron")
>       return BookmarksUtils.DROP_ON;
Rather than trying to merge these I think you should leave them as (three including the next) separate if statements.

>+    if (target.id == "bookmarks-ptf") {
>+      return target.hasChildNodes()
>+             ? BookmarksUtils.DROP_AFTER : BookmarksUtils.DROP_ON;
>     }
The ? belongs at the end of the line, and the next line should have 4 spaces of indent, not 7.

Comment 47

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2b-XPFE) <bookmarksMenu.js> [Checkin: Comment 50]

Bv2a-XPFE, with comment 46 suggestion(s).

Comment 48

[[e-mail]]

Comment on attachment 222114 [details] [diff] [review]
(Bv2b-XPFE) <bookmarksMenu.js>
[Checkin: Comment 50]

>-    if (target.id == "bookmarks-ptf" || 
>-        target.id == "bookmarks-chevron") {
>-      return BookmarksUtils.DROP_ON;
>+    if (target.id == "bookmarks-ptf") {
>+      return target.hasChildNodes() ?
>+          BookmarksUtils.DROP_AFTER : BookmarksUtils.DROP_ON;
>     }
>+    if (target.id == "bookmarks-chevron")
>+      return BookmarksUtils.DROP_ON;
I would have put the chevron test first to make the diff smaller ;-) but I guess they should really be in the order of most use, so it doesn't matter.
>-    if (target.id == "bookmarks-ptf" || 
>-        target.id == "bookmarks-chevron") {
>+    if (target.id == "bookmarks-chevron")
>       return BookmarksUtils.DROP_ON;
>+    if (target.id == "bookmarks-ptf") {
>+      return target.hasChildNodes() ?
>+          BookmarksUtils.DROP_AFTER : BookmarksUtils.DROP_ON;
>     }

Comment 49

[Serge Gautherie (:sgautherie)]

(In reply to comment #48)
> I would have put the chevron test first to make the diff smaller ;-) but I
> guess they should really be in the order of most use, so it doesn't matter.

That's what I thought about then what I choose, even though this otherwise "code merging" patch changes orders at other places :-|

Comment 50

[Serge Gautherie (:sgautherie)]

Comment on attachment 222114 [details] [diff] [review]
(Bv2b-XPFE) <bookmarksMenu.js>
[Checkin: Comment 50]


Checkins: {
2006-05-17 02:16	neil%parkwaycc.co.uk 	mozilla/xpfe/components/bookmarks/resources/bookmarksMenu.js 	1.24

2006-05-17 02:19	neil%parkwaycc.co.uk 	mozilla/xpfe/components/bookmarks/resources/bookmarksMenu.js 	1.21.8.2 	MOZILLA_1_8_BRANCH
}

Comment 51

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-FF) <bookmarksMenu.js>

Bv2b-XPFE port/synchronization to FireFox.

Comment 52

[Serge Gautherie (:sgautherie)]

(In reply to comment #44)
> I've discovered that dragging onto the
> empty space itself doesn't actually work despite the original code to find the
> last visible item because the orientation is still DROP_ON and not DROP_AFTER.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060518 SeaMonkey/1.1a] (nightly) (W98SE)

V.Fixed on MOZILLA_1_8_BRANCH.