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

RESOLVED FIXED in Firefox1.5

Status

()

defect
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: syskin2, Assigned: martijn.martijn)

Tracking

({fixed1.8})

1.5.0.x Branch
Firefox1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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.

Updated

15 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

15 years ago
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();
vlad, this one is fun, its one of those "going down the rabbit hole" bugs from
my two-three minutes of poking :)
Assignee: p_ch → vladimir
(Reporter)

Comment 3

15 years ago
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();
(Reporter)

Comment 4

15 years ago
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.
(Reporter)

Comment 5

15 years ago
Posted patch Simple patch, works for me. (obsolete) — Splinter Review
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.
(Reporter)

Updated

15 years ago
Attachment #154077 - Flags: review?(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.
Attachment #154077 - Flags: review?(mconnor) → review-
(Reporter)

Comment 7

15 years ago
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
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?
Assignee: vladimir → vladimir+bm

Comment 9

14 years ago
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 11

14 years ago
(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

14 years ago
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

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

Comment 14

14 years ago
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

14 years ago
Note that this bug also prevents you from middle-clicking items in the Bookmarks
menu (which would otherwise open them in new tabs).
*** Bug 285511 has been marked as a duplicate of this bug. ***
*** Bug 253753 has been marked as a duplicate of this bug. ***

Comment 18

14 years ago
Same problem with version 1.0.3 of Firefox.

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

Comment 20

14 years ago
Problem still present with version 1.0.4.
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

14 years ago
(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.
(Assignee)

Comment 23

14 years ago
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.
(Assignee)

Comment 24

14 years ago
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.
(Assignee)

Comment 25

14 years ago
See also bug 291781.
Depends on: 291781
(Assignee)

Comment 26

14 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #187229 - Flags: review?(mconnor)
(Assignee)

Updated

14 years ago
Assignee: vladimir+bm → martijn.martijn
Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.8b4) Gecko/20050908 Firefox/1.4

Same here

Comment 28

14 years ago
*** Bug 265380 has been marked as a duplicate of this bug. ***
Severity: minor → normal
Status: NEW → ASSIGNED
Flags: blocking1.8b5?
OS: Windows 2000 → All
Hardware: PC → All
Version: unspecified → 1.5 Branch
Flags: blocking1.8b5? → blocking1.8b5+
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.
Attachment #187229 - Flags: review?(mconnor) → review-
Posted patch focus the menu instead (obsolete) — Splinter Review
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).
Assignee: martijn.martijn → mconnor
Attachment #154077 - Attachment is obsolete: true
Attachment #187229 - Attachment is obsolete: true
Attachment #195883 - Flags: review?(vladimir)
Comment on attachment 195883 [details] [diff] [review]
focus the menu instead

r=vladimir
Attachment #195883 - Flags: review?(vladimir) → review+
Attachment #195883 - Flags: approval1.8b5?

Updated

14 years ago
Attachment #195883 - Flags: approval1.8b5? → approval1.8b5+
fixed branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
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.
Attachment #187229 - Flags: review- → review?(mconnor)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 34

14 years ago
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
Attachment #187229 - Flags: review?(mconnor) → review?(bugs.mano)
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.
Attachment #187229 - Flags: review?(bugs.mano) → review+
Assignee: mconnor → martijn.martijn
Status: REOPENED → NEW
Target Milestone: --- → Firefox1.5
mozilla/browser/components/bookmarks/content/bookmarksMenu 1.52
mozilla/browser/base/content/browser.js 1.505
Status: NEW → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 38

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

Comment 39

14 years ago
Comment on attachment 196469 [details] [diff] [review]
patch

Approved for 1.8b5 per bug meeting
Attachment #196469 - Flags: approval1.8b5? → approval1.8b5+
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
Keywords: fixed1.8
(Assignee)

Comment 41

14 years ago
Thanks for adjusting the patch, Asaf.
*** Bug 309940 has been marked as a duplicate of this bug. ***
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.