Closed Bug 234116 Opened 17 years ago Closed 17 years ago

Context menu commands don't work in *subfolders* of bookmarks menu

Categories

(Firefox :: Bookmarks & History, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: p_ch)

References

()

Details

(Keywords: regression)

Reported by "powder" at http://forums.mozillazine.org/viewtopic.php?p=373419#373419.

Steps to reproduce:
1. Go to a subfolder of the bookmarks menu.
2. Right-click an item.
3. Select "Properties" or "Delete".

Result: The bookmarks menu goes away, but the command is not carried out.  This
appears on my JS console:

Error: [Exception... "Node was not found"  code: "8" nsresult: "0x80530008
(NS_ERROR_DOM_NOT_FOUND_ERR)"  location:
"chrome://browser/content/bookmarks/bookmarksMenu.js Line: 32"]
Source File: chrome://browser/content/bookmarks/bookmarksMenu.js
Line: 32

Regression window (testing trunk builds of Firefox on WinXP):
works:  02/03
broken: 02/05 scragz
broken: 02/12
This'll be the day that I die...
Happens on Linux too for me with Mozilla/5.0 (X11; U; Linux i686; en-US;
rv:1.7a) Gecko/20040214 Firebird/0.8.0+
OS: Windows XP → All
Bug 20022's patch was backed out yesterday, but I still see this bug, so I guess
this bug wasn't caused by that patch.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040215
Firebird/0.8.0+
Works in 20040204
broken in 20040205
Also bug 210910 was broken again too at the same date.
also broken in latest:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040216
Firefox/0.8.0+
Bug 197427 is one possible candidate for the cause of this problem since it
altered removeChild() on the day of the regression (as reported by Hussam).
Maybe someone can try backing out the patch and seeing if the problem goes away?
That line number has to be bogus.  Line 32 is in the license for bookmarksMenu.js
If I click the link in the JS console, it highlights the line
"aTarget.removeChild(nodeList[0]);".  Context:

  /////////////////////////////////////////////////////////////////////////////
  // hides the 'Open in Tabs' on popuphidden so that we won't duplicate it -->
  hideOpenInTabsMenuItem: function (aTarget)
  {
    var nodeList;
    nodeList = aTarget.getElementsByAttribute("class", "openintabs-menuitem");
    if (nodeList.length > 0)
>     aTarget.removeChild(nodeList[0]);
    nodeList = aTarget.getElementsByAttribute("class", "openintabs-menuseparator");
    if (nodeList.length > 0)
      aTarget.removeChild(nodeList[0]);
  },
Whose bright idea was it to use # characters to comment out the license?  In JS
files, that's usually /*...*/ commenting.

Thanks, Jesse, for the context.
> That line number has to be bogus.  Line 32 is in the license for
> bookmarksMenu.js

All hail the mighty preprocessor, for it makes line numbers in chrome useless. 
Of course it could save the line number info and that could be passed to the JS
engine to provide useful line numbers, but....

As for that exception, here is what's going on:

1) GetElementsByAttribute will return not only children but also other
   descendants
2) Clearly the thing it's returning in this case is NOT a child
3) The old XUL removeChild used to just silently ignore you when you asked it to
   remove a random node that was not a child.  After the changes in bug 197427,
   it properly throws an exception.
4) I have no idea whether you want to just catch the exception or whether this
   code wants to be doing something a little smarter in cases when the first
   thing it gets is NOT a child (like actually looking for a child in that list
   or something.... since the search is depth-first, the first node in the list
   could be not a child even if there are children in the list).

Chances are, this code was buggy in some subtle way anyway due to items 1 and 4
above, so it's a good thing we fixed bug 197427.  ;)
Bug 234717, "Right-clicking a File Within a Bookmarks Folder in the Bookmarks
Menu Makes That Folder Inaccessible if Nothing is Selected on the Context Menu",
has the same error message and is probably the same bug.
Blocks: 234717
Flags: blocking0.9?
Strange, works for me for a bookmarks being shown in the bookmark toolbar, and
in folders (and in those folders, ad infinitum) being shown in the bookmark
toolbar.  HOWEVEVER, when I start from the Bookmarks menu, then the context
commands fail (even for bookmarks in the personal toolbar folder.)

Note Using: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040219
Firefox/0.8.0+

Can anyone else confirm my observations?
Yes, same for me - personal toolbar bookmarks context menu works, but not on
main menu bookmarks.
(In reply to comment #10)
> Chances are, this code was buggy in some subtle way anyway due to items 1 and 4
> above, so it's a good thing we fixed bug 197427.  ;)

I would say the code isn't buggy. It's just that for some reason the code isn't
being called for each sub menu (folder), see bug 234717. There's a patch to work
around this attached to that bug.
(In reply to comment #13)
> Yes, same for me - personal toolbar bookmarks context menu works, but not on
> main menu bookmarks.

Correct. Context menus work on the Bookmarks Toolbar and in the Bookmarks
Manager but not the Bookmarks Menu. Interestingly enough, no JavaScript errors
are thrown when the dialog fails to appear from a right-click on the Bookmarks Menu.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040229 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)
I believe this is the same as "my" bug 23380
*** Bug 233380 has been marked as a duplicate of this bug. ***
*** Bug 176880 has been marked as a duplicate of this bug. ***
The proposed fix for Bug 234116 will also fix this bug too.
(In reply to comment #19)
> The proposed fix for Bug 234116 will also fix this bug too.
Sorry for the bug spam. I meant "The proposed fix for Bug 234717 will also fix
this bug too"
Again, sorry for the bug spam

fixed.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: blocking0.9?
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.