Closed Bug 246158 Opened 20 years ago Closed 19 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

1.5.0.x Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox1.5

People

(Reporter: syskin2, Assigned: martijn.martijn)

References

Details

(Keywords: fixed1.8)

Attachments

(2 files, 3 obsolete files)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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();
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.
Attached 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.
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-
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
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.
(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.
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.
*** Bug 267732 has been marked as a duplicate of this bug. ***
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. 
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. ***
Same problem with version 1.0.3 of Firefox.

*** Bug 293806 has been marked as a duplicate of this bug. ***
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.
(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.
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.
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.
See also bug 291781.
Depends on: 291781
Attached 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: 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
*** 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-
Attached 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?
Attachment #195883 - Flags: approval1.8b5? → approval1.8b5+
fixed branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Depends on: 308743
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)
Attachment #187229 - Attachment is obsolete: false
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: fixed1.8
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+
Attached patch patchSplinter Review
Martijn's patch fixed + backout of attachment 195883 [details] [diff] [review].
Attachment #196469 - Flags: 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
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Attachment #195883 - Attachment is obsolete: true
Attachment #195883 - Flags: approval1.8b5+
Attachment #187229 - Attachment is obsolete: true
Attachment #196469 - Flags: approval1.8b5?
Bug 308743 is indeed fixed in today's trunk build :)
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
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.

Attachment

General

Created:
Updated:
Size: