Statusbar URLs of bookmarks-menu missing in Places builds

RESOLVED WORKSFORME

Status

()

Firefox
Bookmarks & History
P3
normal
RESOLVED WORKSFORME
12 years ago
9 years ago

People

(Reporter: Ria Klaassen (not reading all bugmail), Assigned: Annie Sullivan)

Tracking

({fixed1.8.1})

Trunk
Firefox 2 beta1
x86
Windows XP
fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: swag: 1 day)

Attachments

(2 attachments, 1 obsolete attachment)

5.70 KB, patch
Annie Sullivan
: review+
Ben Goodger (use ben at mozilla dot org for email)
: superreview+
Details | Diff | Splinter Review
160.37 KB, image/jpeg
Details
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.
(Reporter)

Updated

12 years ago
Summary: Statusbar URLs of bookmarks-menu missing in Places → Statusbar URLs of bookmarks-menu missing in Places builds
Created attachment 213372 [details] [diff] [review]
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.
Assignee: nobody → mossop
Status: NEW → ASSIGNED
Attachment #213372 - Flags: review?(annie.sullivan)
(Assignee)

Updated

12 years ago
Attachment #213372 - Flags: review?(annie.sullivan) → review+
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.
Attachment #213372 - Flags: superreview-

Updated

12 years ago
Assignee: mossop → nobody
Status: ASSIGNED → NEW
Priority: -- → P3
Target Milestone: --- → Firefox 2 alpha2
Assignee: nobody → annie.sullivan
(Assignee)

Comment 3

12 years ago
> 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") ?
(Assignee)

Comment 4

12 years ago
Created attachment 214327 [details] [diff] [review]
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?
Attachment #214327 - Flags: review?(bugs)
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!
Attachment #214327 - Flags: review?(bugs) → review-
Comment on attachment 213372 [details] [diff] [review]
Various minor fixes

sr=ben@mozilla.org
Attachment #213372 - Flags: superreview- → superreview+
(Assignee)

Updated

12 years ago
Attachment #214327 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.1
(Reporter)

Comment 7

12 years ago
Hm I see RESOLVED FIXED here but the statusbar URLs are still missing.
Now I see Done instead of Undefined.
(Reporter)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

12 years ago
(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.
(Reporter)

Comment 9

12 years ago
Created attachment 214443 [details]
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).
It is not the fault of my bookmarks. Also without imported bookmarks I see this.

Comment 11

12 years ago
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

12 years ago
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)
*** Bug 330807 has been marked as a duplicate of this bug. ***
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
(Assignee)

Updated

12 years ago
Whiteboard: swag: 1 day
This appears to be fixed. Somewhere in the beginning of May.
(Reporter)

Updated

12 years ago
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → WORKSFORME
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.