398409 - right-click -> Bookmark This Page, dialog opens in left corner of window (not under star button)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[u88484]

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]

Comment 1

[AndrewM]

Should this be fixed for beta 1?

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking beta, but definitely blocking final.

Comment 3

[Florian Quèze [:florian]]

Attachment: patch v1

Comment 4

[Asaf Romano (gone)]

Comment on attachment 286864 [details] [diff] [review]
patch v1

This is kinda wrong for the sidebar case.

Comment 5

[Florian Quèze [:florian]]

Attachment: patch v2

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 287042 [details] [diff] [review]
patch v2

What's the point of window.top?

Comment 7

[Florian Quèze [:florian]]

(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

Comment 8

[Dão Gottwald [:dao]]

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

Comment 9

[Florian Quèze [:florian]]

Attachment: patch v2 (unbitrotted)

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

Comment 10

[Asaf Romano (gone)]

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

Comment 11

[Florian Quèze [:florian]]

Attachment: patch v3

Comment 12

[Florian Quèze [:florian]]

Attachment: patch v4

Comment 13

[Florian Quèze [:florian]]

Attachment: patch v4 bis

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

Comment 14

[Asaf Romano (gone)]

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.

Comment 15

[Dão Gottwald [:dao]]

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?

Comment 16

[Asaf Romano (gone)]

target="sidebar"

Comment 17

[Florian Quèze [:florian]]

(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

Comment 18

[Asaf Romano (gone)]

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.

Comment 19

[Florian Quèze [:florian]]

(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?

Comment 20

[Asaf Romano (gone)]

Where do you/we check aBrowser.ownerDocument?

Comment 21

[Florian Quèze [:florian]]

(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

Comment 22

[Asaf Romano (gone)]

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 23

[Florian Quèze [:florian]]

Attachment: patch v5

Comment 24

[Asaf Romano (gone)]

Comment on attachment 288424 [details] [diff] [review]
patch v5

r=mano

Comment 25

[Reed Loden [:reed]]

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

Comment 26

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 27

[Tracy Walker [:tracy]]

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.

Comment 28

[AndrewM]

(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.)

Comment 29

[Florian Quèze [:florian]]

(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.

Comment 30

[AndrewM]

(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 :)

Comment 31

[Tracy Walker [:tracy]]

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.

Comment 32

[Gervase Markham [:gerv]]

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