246158 - bookmarks right-click menu no longer works after removing "bookmarks" from all toolbars

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Radek 'sysKin' Czyz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040608 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040608 Firefox/0.8.0+

If you use "customize" and remove "bookmarks" list from all toolbars, you will
not be able to right-click on a bookmark (in "bookmarks" menu).

Reproducible: Always
Steps to Reproduce:
1. menu -> view -> toolbars -> customize. Find "Bookmarks Toolbar Item" on your
toolbar and drag it from your toolbar to the customize window (ie remove it from
your toolbar)
2. restart firefox (important)
3. menu -> bookmarks, right click on any bookmark

Actual Results:  
The menu that pops up is empty, it's just a tiny gray square with no elements

Expected Results:  
Normal menu should appear

Seems to be related with bug 221482 and bug 232707 except that this time
restarting doesn't fix anything.
A workaround: keep the "Bookmarks Toolbar Item" on some toolbar but keep that
toolbar hidden. Seems easy but took me a loooong time to find.

Comment 1

[Nickolay_Ponomarev]

Same for me with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7)
Gecko/20040626 Firefox/0.9.1

There's an error in JavaScript console:

Error: bt has no properties
Source File: chrome://browser/content/bookmarks/bookmarksMenu.js
Line: 68

>var bt = document.getElementById("bookmarks-ptf");
>bt.focus();

Comment 2

[Mike Connor [:mconnor]]

vlad, this one is fun, its one of those "going down the rabbit hole" bugs from
my two-three minutes of poking :)

Comment 3

[Radek 'sysKin' Czyz]

A very similar bug 251258 was fixed recently.
The patch is very simple and - guess what - a very similar patch fixes this bug
for me.

I don't have sourcefiles so I can't make a proper cvs diff - but I'll attach it
as soon as I can. The fix is as follows: (compare with bug 251258's fix):

var bt = document.getElementById("bookmarks-ptf");
- bt.focus();
+ if (bt)
+   bt.focus();

Comment 4

[Radek 'sysKin' Czyz]

OK forget the above - the context menu does appear after that patch (and in fact
any other code that gets "bookmarks-ptf" checks if it succeeded) but this
context menu doesn't work.

No JS console error this time.

Comment 5

[Radek 'sysKin' Czyz]

Attachment: Simple patch, works for me.

As explained above - the bug is fixed with this patch (the menu appears) but
the whole thing is NOT fixed - this menu doesn't work.

However, it does seem like a correct first step.

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 154077 [details] [diff] [review]
Simple patch, works for me.

it doesn't make sense to check this type of thing in.  I'd rather have the
context menu completely inaccessible than to be exposed and broken.

Comment 7

[Radek 'sysKin' Czyz]

OK, new revelations from me.
It would be very good if someone smart could tell me what's going one here, but
the following thing happens:
 - if we don't focus Bookmarks Toolbar, bookmark's menu doesn't work
 - alternatively, we can focus Bookmarks Menu instead.

Now, based on this, I have a patch that works - but the real queston is, why
this happens? If there is a good explaination, then this patch is OK. If not,
then something else is broken.

Again, I can't do a proper diff - but this time I'll wait for someone's good
comment before actually making one.

Diff goes like this - in bookmarksMenu.js around line 115

    var bt = document.getElementById("bookmarks-ptf");
+   if (!bt) // bookmark toolbar is hidden
+     bt = document.getElementById("bookmarks-menu");
    bt.focus();

Please note that if BOTH toolbar and menu are not available, bookmarks' menu
won't work AGAIN. I'm not sure how to remove the menu though (and then I'd test
on sidebar).

Perhaps just focusing menu is enough, since it can't be hidden easly?

Waiting for some comments,
Regards,
Radek

Comment 8

[fabio andree scheurel]

I have this bug, too, and i have it since I use Firefox (ie since it was renamed
to firefox). At first, I was just disappointed, that there is no possibility to
work with a context menu in the bookmarks menu, but some days ago I found out
that this feature was just not working on my installed browser.

So, right now I am using the latest 1.0 Release, where I could not find the file
bookmarksMenu.js. All other tips with the Item etc. didn't work either. So
hopefully there is any other solution?

Comment 9

[Takeshi Nishimura]

I can still reproduce this problem on trunk-20050316.
(Correct spelling for the item is "Bookmarks Toolbar Items".)
Confirmed: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv1.8b2)
Gecko/20050316 Firefox/1.0+

Yes, though workaround is of course to put "Bookmaks Toolbar Items" on any
toolbar, it is nearly impossible to find out the cause if you accidentally
removed that item and later see the problem ("small square box").

BTW, adding keywords "small square box" and "tiny rectangle" in this comment. 
These words would have helped me much.

Comment 10

[Takeshi Nishimura]

Attachment: Screen shot (of recent trunk)

Comment 11

[Takeshi Nishimura]

(In reply to comment #7)
The following comments will help you.
http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/content/bookmarksMenu.js#480
#   we needed to focus the element that has the bm command controller
#   to get here. Now, let''s focus the content before performing the command:

It seems that doCommand() in the focused node is called.

Comment 12

[Marco Iannaccone]

Its eems this bug <a
href="https://bugzilla.mozilla.org/show_bug.cgi?id=267732">267732</a> is a clone
of this one.

Anyway, the problem is still present with version 1.0.2.

Comment 13

[Wolf [:wolf]]

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

Comment 14

[tjones]

I have also seen this bug: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7.6) Gecko/20050317 Firefox/1.0.2

The only additional info is that I had moved my bookmarks toolbar items to a
different toolbar (displayed) and hidden my empty bookmarks toolbar. Once I
added something to the bookmarks toolbar, then re-hid it, the context menu was
available again. 
So it appears that:
1. If you remove bookmarks toolbar items to the customize box
OR
2. Hide the *empty* bookmarks toolbar 

you will see this bug. 

Comment 15

[tommyjb]

Note that this bug also prevents you from middle-clicking items in the Bookmarks
menu (which would otherwise open them in new tabs).

Comment 16

[Nickolay_Ponomarev]

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

Comment 17

[Nickolay_Ponomarev]

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

Comment 18

[Marco Iannaccone]

Same problem with version 1.0.3 of Firefox.

Comment 19

[Phil Ringnalda (:philor)]

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

Comment 20

[Marco Iannaccone]

Problem still present with version 1.0.4.

Comment 21

[Nickolay_Ponomarev]

There's no need for confirmations. The bug is in "NEW" state, which means it's
acknowledged but has not yet been fixed. Spamming the bug and all the people in
CC list doesn't help fixing the bug.

Comment 22

[Marco Iannaccone]

(In reply to comment #21)
> There's no need for confirmations. The bug is in "NEW" state, which means it's
> acknowledged but has not yet been fixed. Spamming the bug and all the people in
> CC list doesn't help fixing the bug.

Ops, I'm sorry.

Comment 23

[Martijn Wargers (dead)]

For some reason, controller is null in this place in this situation in the
function goDoCommand:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/globalOverlay.js#89
I have no idea why, but it is only null in this situation.

Comment 24

[Martijn Wargers (dead)]

Forgot to say, this is with the "Simple patch, works for me." patch.
I've checked the 'command' argument in the function, but there is no difference
between the normal situation and the "no bookmarks in toolbar" situation.

Comment 25

[Martijn Wargers (dead)]

See also bug 291781.

Comment 26

[Martijn Wargers (dead)]

Attachment: patch

This fixes the issue for me.
This adds the controller stuff to the personal toolbar (which never gets
removed, afaik), instead of the bookmarks-ptf toolbar-item.

Comment 27

[Rubén Martín [:Nukeador] ❌ [away till Aug 31st]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.8b4) Gecko/20050908 Firefox/1.4

Same here

Comment 28

[Robin Monks]

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

Comment 29

[Mike Connor [:mconnor]]

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

this is too much churn, focusing the bookmark menu works just fine in my
testing.  Patch upcoming.

Comment 30

[Mike Connor [:mconnor]]

Attachment: focus the menu instead

the menu also has a controller, so focusing that works, without any focus
issues popping up (other than the ones we already have, like clicking Expand
etc).

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 195883 [details] [diff] [review]
focus the menu instead

r=vladimir

Comment 32

[Mike Connor [:mconnor]]

fixed branch and trunk.

Comment 33

[Asaf Romano (gone)]

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

Focusing the menu isn't really possible on OS X; to me, this patch looks like
the way to go, renominating.

Comment 34

[Radek 'sysKin' Czyz]

OK now that it's reopened I can say it here: it doesn't work on windows either.
The menu appears fine but doesn't work if bookmarks toolbar item is removed.

That's with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4)
Gecko/20050916 Firefox/1.4

Comment 35

[Asaf Romano (gone)]

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

>Index: browser/components/bookmarks/content/bookmarksMenu.js
>===================================================================

> 
>-    var bt = document.getElementById("bookmarks-ptf");
>-    bt.focus(); // buttons in the bt have -moz-user-focus: ignore
>+    document.getElementById("PersonalToolbar").focus(); 

A (modified) comment about -moz-user-focus: ignore should remain.


>Index: browser/base/content/browser.js
>===================================================================
...
>+  document.getElementById("PersonalToolbar").controllers.appendController(BookmarksMenuController);

this line is a bit too long.


>   try {
>-    var bm = document.getElementById("bookmarks-menu");
>-    bm.controllers.removeController(BookmarksMenuController);
>+    document.getElementById("bookmarks-ptf").database.RemoveObserver(BookmarksToolbarRDFObserver);
>   } catch (ex) {
>   }

This shouldn't fail into the catch block in the case where there's no bookmark
toolbar (The try block should reamin for a failure in RemoveObserver).

Otherwise, r=mano.

Comment 36

[Asaf Romano (gone)]

Attachment: patch

Martijn's patch fixed + backout of attachment 195883 [details] [diff] [review].

Comment 37

[Asaf Romano (gone)]

mozilla/browser/components/bookmarks/content/bookmarksMenu 1.52
mozilla/browser/base/content/browser.js 1.505

Comment 38

[Jesse Ruderman]

Bug 308743 is indeed fixed in today's trunk build :)

Comment 39

[Mike Schroepfer]

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

Approved for 1.8b5 per bug meeting

Comment 40

[Asaf Romano (gone)]

1.8 Branch:
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.28; previous revision: 1.479.2.27
done
Checking in browser/components/bookmarks/content/bookmarksMenu.js;
/cvsroot/mozilla/browser/components/bookmarks/content/bookmarksMenu.js,v  <-- 
bookmarksMenu.js
new revision: 1.48.2.4; previous revision: 1.48.2.3
done

Comment 41

[Martijn Wargers (dead)]

Thanks for adjusting the patch, Asaf.

Comment 42

[Asaf Romano (gone)]

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

Comment 43

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change