328779 - Statusbar URLs of bookmarks-menu missing in Places builds

Status: RESOLVED WORKSFORME

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

Description

[Ria Klaassen (not reading all bugmail)]

When I hover over a bookmark I see "undefined" in the statusbar.
I'd also vote for a statusbar URL for every link in the bookmarks toolbar.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Various minor fixes

This patch fixes a number of minor issues that show up when the JavaScript console shows strict messages. One of them is setting the status bar so I guess I may as well post this here.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 213372 [details] [diff] [review]
Various minor fixes

r-

Since the content is identical to the url attribute, why even bother? Also, this pollutes the View (menu binding) with metadata that is specific to one instance of it.

Comment 3

[Annie Sullivan]

> Since the content is identical to the url attribute, why even bother? Also,
> this pollutes the View (menu binding) with metadata that is specific to one
> instance of it. 

Right now the statusbar says "undefined" when you hover over a link right now, so I think we should do something better.

Now that we have a popupshowing handler in the BookmarksEventHandler object, would it work to loop through all the elements in the menu and do something like if (hasAttribute("url")) setAttribute("statustext") ?

Comment 4

[Annie Sullivan]

Attachment: Sets the statusbar text in the popupshowing event. Fixes some JavaScript warnings.

This patch uses the popupshowing event for the bookmarks menu/toolbar to set the statusbar text, so that the binding isn't required to set it.  It gets rid of a JavaScript warning by removing a reference to child.url, and also the trailing comma from Dave's patch.  It also fixes a bug I introduced where menus weren't being closed on middle-click.

Dave, maybe it makes more sense to file a separate bug and fix the rest of the JS errors there?

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214327 [details] [diff] [review]
Sets the statusbar text in the popupshowing event. Fixes some JavaScript warnings.

>Index: browser/components/places/content/menu.xml
>                 element.setAttribute("url", child.uri);
>-                element.setAttribute("statustext", child.url);

You're going to kill me (again)... but after some research it occurs to me that this probably isn't that harmful since the "statustext" attribute appears to be part of the toolkit API (defined in toolbar.xml in toolkit/content/widgets) and not a browser-specific thing. Urg. Walking the list on popup showing means an additional crawl just for this. So leave it be I guess. 

I think this means the original patch is fine. Sorry for being cracktarded!

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 213372 [details] [diff] [review]
Various minor fixes

sr=[e-mail]

Comment 7

[Ria Klaassen (not reading all bugmail)]

Hm I see RESOLVED FIXED here but the statusbar URLs are still missing.
Now I see Done instead of Undefined.

Comment 8

[Annie Sullivan]

(In reply to comment #7)
> Hm I see RESOLVED FIXED here but the statusbar URLs are still missing.
> Now I see Done instead of Undefined.

What platform are you on?  Are you using a trunk build?  Do you have any extensions installed?  I tested this on Windows XP with trunk and 1.8 branch builds, and I'm seeing the urls.

Comment 9

[Ria Klaassen (not reading all bugmail)]

Attachment: Screenshot

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060308 Firefox/1.6a1
It is on a brand new profile without extensions and default settings (first run).

Comment 10

[Ria Klaassen (not reading all bugmail)]

It is not the fault of my bookmarks. Also without imported bookmarks I see this.

Comment 11

[Daniel C]

Not fixed for me either (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060308 Firefox/1.6a1) - I just see 'Done'.

Comment 12

[Ryuichi KUBUKI]

Still not fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060316 Firefox/1.6a1 (non-Cairo trunk, with no extensions)

Comment 13

[Ria Klaassen (not reading all bugmail)]

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

Comment 14

[Ria Klaassen (not reading all bugmail)]

This appears to be fixed. Somewhere in the beginning of May.

Comment 15

[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