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)
Tracking
()
RESOLVED
FIXED
Firefox1.5
People
(Reporter: syskin2, Assigned: martijn.martijn)
References
Details
(Keywords: fixed1.8)
Attachments
(2 files, 3 obsolete files)
1.42 KB,
image/png
|
Details | |
4.60 KB,
patch
|
asaf
:
review+
mtschrep
:
approval1.8b5+
|
Details | Diff | Splinter Review |
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•20 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•20 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();
Comment 2•20 years ago
|
||
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•20 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•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #154077 -
Flags: review?(mconnor)
Comment 6•20 years ago
|
||
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•20 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
Comment 8•20 years ago
|
||
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•19 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 10•19 years ago
|
||
Comment 11•19 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•19 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•19 years ago
|
||
*** Bug 267732 has been marked as a duplicate of this bug. ***
Comment 14•19 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•19 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).
Comment 16•19 years ago
|
||
*** Bug 285511 has been marked as a duplicate of this bug. ***
Comment 17•19 years ago
|
||
*** Bug 253753 has been marked as a duplicate of this bug. ***
Comment 18•19 years ago
|
||
Same problem with version 1.0.3 of Firefox.
Comment 19•19 years ago
|
||
*** Bug 293806 has been marked as a duplicate of this bug. ***
Comment 20•19 years ago
|
||
Problem still present with version 1.0.4.
Comment 21•19 years ago
|
||
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•19 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•19 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•19 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 26•19 years ago
|
||
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•19 years ago
|
Assignee: vladimir+bm → martijn.martijn
Comment 27•19 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.8b4) Gecko/20050908 Firefox/1.4 Same here
Comment 28•19 years ago
|
||
*** Bug 265380 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Severity: minor → normal
Status: NEW → ASSIGNED
Flags: blocking1.8b5?
OS: Windows 2000 → All
Hardware: PC → All
Version: unspecified → 1.5 Branch
Updated•19 years ago
|
Flags: blocking1.8b5? → blocking1.8b5+
Comment 29•19 years ago
|
||
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-
Comment 30•19 years ago
|
||
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+
Updated•19 years ago
|
Attachment #195883 -
Flags: approval1.8b5?
Updated•19 years ago
|
Attachment #195883 -
Flags: approval1.8b5? → approval1.8b5+
Comment 32•19 years ago
|
||
fixed branch and trunk.
Comment 33•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #187229 -
Attachment is obsolete: false
Updated•19 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 34•19 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
Updated•19 years ago
|
Attachment #187229 -
Flags: review?(mconnor) → review?(bugs.mano)
Comment 35•19 years ago
|
||
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+
Comment 36•19 years ago
|
||
Martijn's patch fixed + backout of attachment 195883 [details] [diff] [review].
Attachment #196469 -
Flags: review+
Updated•19 years ago
|
Assignee: mconnor → martijn.martijn
Status: REOPENED → NEW
Target Milestone: --- → Firefox1.5
Comment 37•19 years ago
|
||
mozilla/browser/components/bookmarks/content/bookmarksMenu 1.52 mozilla/browser/base/content/browser.js 1.505
Status: NEW → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #195883 -
Attachment is obsolete: true
Attachment #195883 -
Flags: approval1.8b5+
Updated•19 years ago
|
Attachment #187229 -
Attachment is obsolete: true
Updated•19 years ago
|
Attachment #196469 -
Flags: approval1.8b5?
Comment 38•19 years ago
|
||
Bug 308743 is indeed fixed in today's trunk build :)
Comment 39•19 years ago
|
||
Comment on attachment 196469 [details] [diff] [review] patch Approved for 1.8b5 per bug meeting
Attachment #196469 -
Flags: approval1.8b5? → approval1.8b5+
Comment 40•19 years ago
|
||
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•19 years ago
|
||
Thanks for adjusting the patch, Asaf.
Comment 42•19 years ago
|
||
*** Bug 309940 has been marked as a duplicate of this bug. ***
Comment 43•18 years ago
|
||
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.
Description
•