The add bookmarks dialog opens in the left corner of the browser window when using right-click-> bookmark this page. I found this a few days before the dialog started opening here no matter how you invoked it (last patch landing in 385266 caused that) but I thought this would have been fixed during that fix for this issue so I didn't file a bug for this..sorry. Error messages in console: Error: [Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemInserted]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: file:///C:/Program%20Files/firefox/components/nsPlacesTransactionsService.js :: PCIT_doTransaction :: line 297" data: no] Source File: file:///C:/Program%20Files/firefox/components/nsPlacesTransactionsService.js Line: 297 Error: [Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemChanged]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: chrome://browser/content/places/utils.js :: anonymous :: line 1398" data: no] Source File: chrome://browser/content/places/utils.js Line: 1398 Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getItemTitle]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://browser/content/places/editBookmarkOverlay.js :: EIO__getItemStaticTitle :: line 297" data: no]

Should this be fixed for beta 1?

Not blocking beta, but definitely blocking final.

Comment on attachment 286864 [details] [diff] [review] patch v1 This is kinda wrong for the sidebar case.

Comment on attachment 287042 [details] [diff] [review] patch v2 What's the point of window.top?

(In reply to comment #6) > (From update of attachment 287042 [details] [diff] [review]) > What's the point of window.top? > Not failing in the sidebar case. In the sidebar we are in web-panels.xul instead of browser.xul

Thanks, I see ... should have read Mano's comment.

The changes of bug 400924 were not yet in my tree when I did the previous diff.

Comment on attachment 287635 [details] [diff] [review] patch v2 (unbitrotted) [02:08] <Mano> i don't think you should change PlacesCommandHook itself to use window.top.PlacesCommandHook rather than |this| [02:08] <Mano> I would rather fix callers [02:08] <Mano> as in, callers should do window.top.PlacesCommandHook.showEditBookmarkPopup [02:09] <Mano> rather than just PlacesCommandHook.showEditBookmarkPopup

This last chunk in the previous diff was not actually wanted.

Comment on attachment 287796 [details] [diff] [review] patch v4 bis Er, so this docks the panel to the star icon even if panel is opened for the sidebar's browser? Really, the only case in which the panel should be opened under the star icon is in bookmarkCurrentPage.

What's the use case for bookmarking in the sidebar anyway? Isn't a page loaded there bookmarked in the first place? Can we just remove or disable that command?

target="sidebar"

(In reply to comment #14) > (From update of attachment 287796 [details] [diff] [review]) > Er, so this docks the panel to the star icon even if panel is opened for the > sidebar's browser? No. var starIcon = aBrowser.ownerDocument.getElementById("star-button"); aBrowser.ownerDocument ensures we can't find the star icon when we are actually in web-panels.xul

OK, what about right-click on a background tab->bookmark this tab? For safety, you could add a window.content == aBrowser.contentWindow check in bookmarkPage.

(In reply to comment #18) > OK, what about right-click on a background tab->bookmark this tab? > When right-clicking on a tab->bookmark this tab the popup is never docked. I should take care of the "current tab" case. > For safety, you could add a window.content == aBrowser.contentWindow check in > bookmarkPage. > Not sure I understand what you want here. Do you mean the aBrowser.ownerDocument thing is a hack that is not visible enough and should be replaced?

Where do you/we check aBrowser.ownerDocument?

(In reply to comment #20) > Where do you/we check aBrowser.ownerDocument? > >+ // dock the panel to the star icon if it is visible, otherwise dock >+ // it to the content area >+ var starIcon = aBrowser.ownerDocument.getElementById("star-button"); >+ if (starIcon && isElementVisible(starIcon)) { If we don't get the star-button element, the popup is not docked. And we don't get the star-button when aBrowser.ownerDocument is not browser.xul

Yes, but bookmarkPage shouldn't dock the panel to the star-button if it's called for a background-tab-browser (which happen to live in browser.xul...).

Comment on attachment 288424 [details] [diff] [review] patch v5 r=mano

Checking in browser/base/content/browser-places.js; /cvsroot/mozilla/browser/base/content/browser-places.js,v <-- browser-places.js new revision: 1.63; previous revision: 1.62 done Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.888; previous revision: 1.887 done Checking in browser/base/content/nsContextMenu.js; /cvsroot/mozilla/browser/base/content/nsContextMenu.js,v <-- nsContextMenu.js new revision: 1.28; previous revision: 1.27 done

This may have caused: https://bugzilla.mozilla.org/show_bug.cgi?id=403641

So this fixed just the Context click content "Book This Page..." Attempt to context click any link, background tab or sidebar link will open the add dialog at the upper left corner of the content pane. verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre 2 cents: most folks won't understand why the dialog opens in two different places. Most probably won't care. Enough will file bugs.

(In reply to comment #27) > So this fixed just the Context click content "Book This Page..." Attempt to > context click any link, background tab or sidebar link will open the add dialog > at the upper left corner of the content pane. > > verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; > rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre > > 2 cents: most folks won't understand why the dialog opens in two different > places. Most probably won't care. Enough will file bugs. Thanks for mentioning that this also affects right-clicking on a background tab -> Bookmark This Tab, I have added that to bug 405339. In terms of sidebar links (I guess you mean the History sidebar), when I right-click -> Bookmark This Link, I get a different dialog (with the title 'Add Bookmark') that opens in the center of the content pane. Maybe the behavior you saw only occurs on Mac? (I'm on Vista.)

(In reply to comment #28) > In terms of sidebar links (I guess you mean the History sidebar) I think the sidebar case here was when you bookmark an url, edit the bookmark to check "Load this bookmark in sidebar" and then load it.

(In reply to comment #29) > (In reply to comment #28) > > > In terms of sidebar links (I guess you mean the History sidebar) > > I think the sidebar case here was when you bookmark an url, edit the bookmark > to check "Load this bookmark in sidebar" and then load it. Ah right, thanks for the explanation, I didn't know you could do that :)

Yep, that's it, links loaded in the sidebar from an Open in the sidebar bookmark. Actually, in that case the add dialog opens at the upper left of the sidebar as opposed to the upper left of the content area. I'll add this comment to 405339 as well.

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv