138523 - Personal Toolbar context menu for the empty area does not work

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Leo H]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    2002041711

When right-clicking on the Personal Toolbar (not on any of the links on it, but
just on empty space on the Personal Toolbar) you get context menu that has many
invalid/non-functional items.



Reproducible: Always
Steps to Reproduce:
Right-click on the Personal Toolbar (not on any of the links on it, but just on
empty space on the Personal Toolbar)

Actual Results:  You get the following menu:

Open in New Window
-------------
New Folder...
-------------
Cut
Copy
Paste
File Bookmarks(s)...
-------------
Delete
Rename...
-------------
Properties


Of these, only File Bookmarks, Rename, and Properties do anything.


Expected Results:  The rest of the options should either do something or be removed.
I expect that "Open in New Window" should open the Person Toolbar folder as it
does with the folders placed on the toolbar.
I expect that "New Folder..." should create a new folder on the toolbar.

Comment 1

[Michael Hendy (Hendikins)]

Confirming since I can't find an obvious dupe... 

Comment 2

[Leo H]

I just learned how to modify the Mozilla UI and created a complete fix for this
bug.  I only had to slightly modify two .js files in the bookmarks module. 
However, I'm not yet able to create a patch diff file... My Win2000 environment
isn't cooperating... Do I need to create that diff to be able to submit the patch?

Comment 3

[Koike Kazuhiko]

I filed a similar problem as bug 140463.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Leo, have you looked into patchmaker (http://www.gerv.net/software/patch-maker/) ?

Comment 5

[Leo H]

Attachment: Context menu fix - PatchMaker diff file

This patch fixes all the personal toolbar context menu irregularities.<br>
Right-clicking on empty space in toolbar now brings up following menu:<br>
<pre>
Open in New Window
------------------
New Folder...
------------------
Paste
</pre>
<br>
Open in New Window and New Folder commands now work.
<br>
Removed the "cut" command from all personal toolbar menus.  The copy function
is not currently reliable (bug 129428 and bug 140463), which causes severe data
loss (whole folders can be accidently deleted) when a user tries to use the
"cut" command.	It is safer to disable the "cut" command entirely right now -
users can still try to use the "copy" and "delete" commands.

Comment 6

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

No. 

While I agree that showing "File Bookmarks" on the Personal Toolbar is silly, I
disagree with some of your other changes. Here's what I think should be in the
PT menu:

Open in New Window...
--
New Folder...
--
Cut (disabled)
Copy (disabled)
Paste (selectively enabled/disabled)
--
Properties

or some variant thereof. 

Comment 7

[Leo H]

No.?  Thats what I get for helping to fix one of the most obvious bugs in the
browser?  Thank you, Ben.
I explained in the patch why I removed the "Cut" and the "Properties" commands -
the first is currently extremely dangerous (I lost a folder full of bookmarks
because of it), and the second is completely pointless (please explain if you
think its not).
Also, I hope you didn't misunderstand - my patch affects just right-clicking on
an empty space on the toolbar, not the links or folders.  Niether the "Copy" or
the "Properties" commands have been removed in the second case.
I just hope you can put in some resonable fix for 1.0.

Comment 8

[Leo H]

Attachment: See comment # 13... Since the copy and cut commands have been fixed, I changed my patch to only disable the cut command on empty area of the toolbar.

Here is a new patch that does exactly what Ben requested.  I hope he will
consent to putting it into 1.0.

Comment 9

[Pierre Chanial]

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

Comment 10

[Pierre Chanial]

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

Comment 11

[Pierre Chanial]

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

Comment 12

[R.K.Aa.]

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

Comment 13

[Leo H]

Comment on attachment 82274 [details] [diff] [review]
See comment # 13...
Since the copy and cut commands have been fixed, I changed my patch to only disable the cut command on empty area of the toolbar.

>--- comm/content/communicator/bookmarks/bookmarksOverlay.js.bak	Mon Apr 29 09:17:54 2002
>+++ comm/content/communicator/bookmarks/bookmarksOverlay.js	Fri May  3 18:29:28 2002
>@@ -241,16 +241,25 @@
>     case "http://home.netscape.com/NC-rdf#Bookmark":
>       commands = ["bm_open", "bm_openinnewwindow", /* "bm_openinnewtab", */ "bm_separator",
>                   "bm_newfolder", "bm_separator",
>-                  "bm_cut", "bm_copy", "bm_paste", "bm_fileBookmark", "bm_separator",
>+                  "bm_cut", "bm_copy", "bm_paste", "bm_separator",
>                   "bm_delete", "bm_rename", "bm_separator",
>                   "bm_properties"];
>       break;
>     case "http://home.netscape.com/NC-rdf#Folder":
>+     if (aNodeID != "NC:PersonalToolbarFolder") {
>       commands = ["bm_openfolder", "bm_openinnewwindow", "bm_separator", 
>                   "bm_newfolder", "bm_separator",
>-                  "bm_cut", "bm_copy", "bm_paste", "bm_fileBookmark", "bm_separator",
>+                  "bm_cut", "bm_copy", "bm_paste", "bm_separator",
>                   "bm_delete", "bm_rename", "bm_separator",
>                   "bm_properties"];
>+     }
>+     else {  // PersonalToolbar
>+        commands = ["bm_openinnewwindow", "bm_separator",
>+                   "bm_newfolder", "bm_separator",
>+                   "bm_cut", "bm_copy", 
>+                   "bm_paste",
>+                   "bm_separator" ,"bm_properties"]; // no reason to modify the personal toolbar properties, except to break something
>+     }
>       break;
>     case "http://home.netscape.com/NC-rdf#IEFavoriteFolder":
>       commands = ["bm_openfolder", "bm_separator",
>@@ -318,7 +327,8 @@
>       this.commands.openFolder(selectedItem);
>       break;
>     case "bm_openinnewwindow":
>-      if (this.resolveType(selectedItem.id) == NC_NS + "Folder")
>+      if (this.resolveType(selectedItem.id) == NC_NS + "Folder" ||
>+          selectedItem.parentNode.id == "PersonalToolbar")
>         this.openFolderInNewWindow(selectedItem);
>       else
>         this.open(null, selectedItem, true);
>--- comm/content/navigator/personalToolbar.js.bak	Mon Apr 29 09:18:04 2002
>+++ comm/content/navigator/personalToolbar.js	Fri May  3 18:32:32 2002
>@@ -77,6 +77,48 @@
>     xulElement.setAttribute("command", cmd);
>     
>     switch (aCommandName) {
>+    // disable cut and copy comands on empty area of personal toolbar
>+    case NC_NS_CMD + "bm_cut":
>+      if (aItemNode.localName == "hbox") {
>+         var commandNode = document.getElementById("cmd_bm_cut");
>+         if (commandNode) { 
>+           commandNode.setAttribute("disabled", "true");
>+         }
>+      }
>+      else {
>+         var commandNode = document.getElementById("cmd_bm_cut");
>+         if (commandNode) { 
>+           commandNode.removeAttribute("disabled");
>+         }
>+      }
>+      xulElement.setAttribute("label", aDisplayName);
>+      break;
>+    case NC_NS_CMD + "bm_copy":
>+     
>+      if (aItemNode.localName == "hbox") {
>+         var commandNode = document.getElementById("cmd_bm_copy");
>+         if (commandNode) { 
>+           commandNode.setAttribute("disabled", "true");
>+         }
>+      }
>+      else {
>+         var commandNode = document.getElementById("cmd_bm_copy");
>+         if (commandNode) { 
>+           commandNode.removeAttribute("disabled");
>+         }
>+      }
>+      xulElement.setAttribute("label", aDisplayName);
>+      break;
>+    case NC_NS_CMD + "bm_paste":
>+      // disable paste if there is nothing to paste
>+      if (!gBookmarksShell.canPaste()) {
>+   	 var commandNode = document.getElementById("cmd_bm_paste");
>+         if (commandNode) { 
>+           commandNode.setAttribute("disabled", "true");
>+         }
>+      }
>+      else {
>+     	 var commandNode = document.getElementById("cmd_bm_paste");
>+         if (commandNode) { 
>+           commandNode.removeAttribute("disabled");
>+         }
>+      }     
>+      xulElement.setAttribute("label", aDisplayName);
>+      break;
>     case NC_NS_CMD + "bm_open":
>       xulElement.setAttribute("label", aDisplayName);
>       xulElement.setAttribute("default", "true");
>@@ -296,6 +338,15 @@
>   getSelection: function ()
>   {
>     return [document.popupNode];
>+  },
>+
>+  getBestItem: function ()
>+  {
>+    var seln = this.getSelection ();
>+    if (seln.length > 0)
>+      return seln[0];
>+    else
>+      return null;
>   },
> 
>   /////////////////////////////////////////////////////////////////////////////

Comment 14

[andrea m]

what is going on with this bug? I'm using 2002072104 and the bug is still there

Comment 15

[Boris Zbarsky [:bzbarsky]]

Leo, is the patch you attached last (attachment 82274 [details] [diff] [review]) still correct with
current trunk?  If so, I'll try to scare up reviews...

Comment 16

[Leo H]

it should be correct, but I no longer really care... the whole bookmarks module
is such a mess, I think it needs a rewrite rather than silly bug fixing.

Comment 17

[Pierre Chanial]

This patch does not apply anymore.
Leo is right about the way we have to fix this kind of bug: I've undertaken a
big clean up of the bookmark code (bug 160019 but much much more to come).

Comment 18

[Leo H]

Thank you, Pierre!

Comment 19

[Pierre Chanial]

taking

Comment 20

[[e-mail]]

IIRC Open in New Window does now work (the code referred to element.id which
didn't work until the PT was fixed to have an id of "NC:PersonalToolbarFolder").

Comment 21

[Eugene Savitsky]

So, will the bookmark code rewritten or what's the state of it?

Tnx

Comment 22

[R.K.Aa.]

"New Folder..." works fine on current trunk CVS, Linux

Comment 23

[Andreas Kunz]

Well, this is fixed, no?
Only Move Bookmarks, Rename and Properties are still activated (of the ones in
doubt). And these refer to the Toolbar folder itself.

Comment 24

[Stefan Borggraefe]

Andreas is right. This was fixed with the landing of the new bookmark code. The
menu looks very different now and all menu items are meaningful and do work.