If you hover the mouse over a bookmark on the Bookmarks Toolbar, you get a tooltip showing the full title and its URI. However, if you hover the mouse over a bookmark inside a folder, or from the Bookmarks menu, you don't see the tooltip. It should always be possible to see the tooltip, as in IE. It's faster than right-clicking and selecting Properties just to verify the URI.

Should this be all the time, or only if you don't have the status bar visible? Currently, the status bar will also show you the URL (although this is sometimes blocked if your bookmarks menu is tall enough)

The problem with the status bar is it's not obvious if you don't already know about it. Not to mention that bouncing your eyes from the bookmark menu at the top of the screen to the status bar at the bottom as you mouse over each entry is not very user friendly. The tooltip should also contain the bookmark's description (if any), requested in related bug 135962.

I second the request. The tooltips should especially be shown in the Bookmarks-Sidebar. There, you can't foresee the URL in the statusbar since when you click on the bookmark the URL already starts loading.

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

tooltip URL doesn't show. Only shows name of bookmark name. Tooltip is slow or doesn't show up at all.

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

I agree, it should definitely show the url, as it would improve usability, the same applies to the history sidebar as well I think..

This can be done with a very minor change to browser.xul. Change: <menuitem class="menuitem-iconic bookmark-item" uri="rdf:*" label="rdf:http://home.netscape.com/NC-rdf#Name" image="rdf:http://home.netscape.com/NC-rdf#Icon" status="rdf:http://home.netscape.com/WEB-rdf#status" statustext="rdf:http://home.netscape.com/NC-rdf#URL" /> To <menuitem class="menuitem-iconic bookmark-item" uri="rdf:*" label="rdf:http://home.netscape.com/NC-rdf#Name" image="rdf:http://home.netscape.com/NC-rdf#Icon" status="rdf:http://home.netscape.com/WEB-rdf#status" statustext="rdf:http://home.netscape.com/NC-rdf#URL" tooltip="btTooltip" /> On a side note, I didn't see this bug till after I implemented this for myself so I didn't realize the status bar was supposed to show the URL (making this unnecessary to me), because the status bar has never shown the URL for me. <shrug />

This is just the kind of consistency cleanup & polish that would be nice to get on the 1.0 train. Vlad, any chance you can look into this for us?

FYI: Bug 256065, which I raised, is similar.

Patch for this should be easy, if someone wants to tackle it; it can probably go in 1.0 if there's a patch.

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

Patch per comment #8. Note that bug 244385 is making the tooltip disappear almost immediately.

(In reply to comment #13) > Note that bug 244385 is making the tooltip disappear almost immediately. True, they are related from a user point of view; yet there is no bugzilla/code "dependency" between them: issues are separate, fix are separate. (Jason: I not sure you need to add your name in the file for a one-liner patch, but that's up to you.) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041217] While this bug is targeted to FF, MAS has the same issue ! Would you mind if I use it to post the MAS counterpart patch ? (Or would you post it yourself ? Or do you want me to file a separate bug ?)

(In reply to comment #14) > (Jason: I not sure you need to add your name in the file for a one-liner patch, > but that's up to you.) Sorry if I shouldn't have included it. This is one of my first patches, and the documentation doesn't say when it's appropriate to add it. I wouldn't be insulted if it went it without my name. > Would you mind if I use it to post the MAS counterpart patch ? > (Or would you post it yourself ? Or do you want me to file a separate bug ?) You can do it.

(In reply to comment #15) > (In reply to comment #14) > > (Jason: I not sure you need to add your name in the file for a one-liner patch, > > but that's up to you.) > Sorry if I shouldn't have included it. This is one of my first patches, and the > documentation doesn't say when it's appropriate to add it. I wouldn't be > insulted if it went it without my name. First patch: new code contributors are always welcomed :-) Contributor line: I agree with you ... I feel like only code owners, or significant feature addition contributors, should (= is worth) add their names, but that's only my guess. > > Would you mind if I use it to post the MAS counterpart patch ? > You can do it. Thanks, I'll do (later).

Comment on attachment 173038 [details] [diff] [review] (Av1-FF) patch (damn it, bugzilla bug...)

MAS counterpart. [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041122] (release) (W98SE) Neil: The toolbar (root) items show Title + Url as expected; With this patch, I get the Title from the menu items too, but the Url line is missing, which makes this patch useless. (how) Can I get the Url to show up too ?

(In reply to comment #18) >(how) Can I get the Url to show up too ? That works using the statustext attribute.

Bv1, with comment 19 suggestion(s). {{ (quoting comment 13) Note that bug 244385 is making the tooltip disappear almost immediately. }}

Comment on attachment 173499 [details] [diff] [review] (Bv1a-MAS) <navigatorOverlay.xul> Unfortunately each tooltip= attribute creates a separate listener which could add up to a lot of wasted memory. Fortunately you can put the tooltip on an ancestor and it will apply to all of its descendants.

(In reply to comment #21) > (From update of attachment 173499 [details] [diff] [review] [edit]) > Fortunately you can put the tooltip on an > ancestor and it will apply to all of its descendants. I'm guessing here :-/ I tried to move |tooltip="btTooltip"| from |<rule> / class="menuitem-iconic bookmark-item"| to |<rule iscontainer="true"> / class="menu-iconic bookmark-item"| which +/- works, but not at all for the first "root popups" from the toolbar... I need a more detailed/code help on this: helpwanted...

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

This is variant 1 of how to resolve this bug.It simply adds 'tooltip="btTooltip"' to the bookmark-items inside a bookmark-toolbar folder. Advantages: tooltips are shown exclusively on bookmarks. Disadvantages: memory-consuming as was pointed out in comment #c21.

This is variant 2 of how to resolve this bug. It simply adds 'tooltip="btTooltip"' to the hbox which contains the bookmark-items and folders and to the hbox which contains the overflowing items inside the bookmark-toolbar. Instead, it could also been simply added to the stack, but I was concerned about the toolbarbutton with class="bookmark-item bookmarks-toolbar-customize". In my solution, the tooltip attribute does not apply to this certain button. Advantages: low memory consumption. Disadvantages: tooltips are also shwon for folders (repeats the name of the folder), and for the 'Open in Tabs' entry inside a folder. Remarks: this all aplies only to the bookmark-toolbar, but not to items in the bookmark menu!

(In reply to comment #25) >Disadvantages: tooltips are also shwon for folders (repeats the name of the >folder), and for the 'Open in Tabs' entry inside a folder. You could always tweak the code to ignore elements without URLs.

(In reply to comment #26) > You could always tweak the code to ignore elements without URLs. You mean by tweaking the fillInBTTooltip() (bookmarksMenu.js, line 1019) function? Something along the lines of a "if (!tipElement.hasAttribute('statustext')) return;"?

There already is a test there, which is what makes the labels appear...

Ok, I should have a patch ready by the end of tomorrow.

Here is the new version of the tooltip fix for bookmark items inside bookmark folders in the bookmark toolbar. I chose the less memory hungry variant, and modified the fillInBTTooltip (bookmarksMenu.js) such that no tooltips are shown when hovering over some other non-bookmark item in the toolbar (e.g. a folder itself, or the 'Open in Tabs' item inside a folder). Although there are less listeners created, it does not harm performance, since we don't enter the fillInBTTooltip function every time we hover over an item, and the tooltip event is only triggered after the specified tooltip timeout. Please note that this patch does not cover the tooltip issues with the bookmarks sidebar!

Comment on attachment 173038 [details] [diff] [review] (Av1-FF) patch this is bad, for the reasons Neil outlined in his comment.

Comment on attachment 184751 [details] [diff] [review] (Av3a-FF) Makes tooltips appear also on bookmarks which are nested inside a folder in the bookmarks toolbar >- if (!title && !url) { >- // bail out early if there is nothing to show >+ if (!url || (!title && !url)) { >+ // bail out early if there is either no url (i.e. if tipElement is not a bookmark, e.g. >+ // a bookmark folder or the 'Open in Tabs' item) or if neither title nor url are >+ // present (i.e. tipElement is e.g. an empty menu entry or some other strange thing); >+ // if only a url but no title or both is present, we want to show the tooltip > return false; > } when would we ever hit the second case? The following is sufficient I think. // if there isn't a url, don't show a tooltip if (!url) return false;

would be nice to have, but not a blocker.

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

Av3a, with comment 32 suggestion(s), and code simplification. I don't use FF: Could you test/review/check in this patch ? Thanks.

Av3b, with a few (unrelated) FF<-SM synchronization. I don't use FF: Could you test/review/check in this patch ? Thanks.

Bv1a, with comment 21 suggestion(s), and with a few (unrelated) FF->SM synchronizations; in other words, I ported Av3c-FF patch (mostly based on AndreasW's Av3a-FF !). [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051110 SeaMonkey/1.5a] (nightly) (W98SE) I tested it successfully: Tooltips show up for "URI items" on Personal Toolbar and inside its folders; and not for folders/groups/separators. As a sidenote, *Bookmarks _menu_ still does not show any tooltip. *SideBar show tooltips for "too long" titles only, and does not show the URI. Could/Should/How theses two be sync'ed with what P.T. will now do !?

Comment on attachment 202690 [details] [diff] [review] (Bv2-SM) I've just realized that the old fillInBTTooltip was written that way to work with xpfe bookmark groups, which have never had tooltips before... you'll need to undo those changes, sorry. >+ // loads a bookmark with the mouse middle button Should be ... middle mouse button >+ tooltipTitle.removeAttribute("hidden"); >+ tooltipTitle.setAttribute("hidden", "true"); Nit: should be tooltipTitle.hidden = false / true;

(In reply to comment #38) >>+ tooltipTitle.removeAttribute("hidden"); >>+ tooltipTitle.setAttribute("hidden", "true"); >Nit: should be tooltipTitle.hidden = false / true; Well, considering the first comment, that hardly matters any more!

(In reply to comment #37) >*Bookmarks _menu_ still does not show any tooltip. Right, could you add them to both main menu and personal toolbar menubutton?

(In reply to comment #38) > (From update of attachment 202690 [details] [diff] [review] [edit]) > I've just realized that the old fillInBTTooltip was written that way to work > with xpfe bookmark groups, which have never had tooltips before... you'll need > to undo those changes, sorry. You mean that SM needs to keep |if (!title && !url) {| !? (while |!url| is enough for FF !?) What is the SM expected behaviour ? (to get a tooltip on items/folders/groups, == all but separators !?)

(In reply to comment #41) >You mean that SM needs to keep |if (!title && !url) {| !? (while |!url| is >enough for FF !?) I guess so. >What is the SM expected behaviour ? (to get a tooltip on items/folders/groups, >== all but separators !?) groups: tooltip shows the label items: tooltip shows the url, and the label if it is different but not blank. folders: no tooltip please

(In reply to comment #40) > (In reply to comment #37) > >*Bookmarks _menu_ still does not show any tooltip. > Right, could you add them to both main menu and personal toolbar menubutton? P.T. menubutton got the tooltip after restoring fillInBTTooltip() per comment 42. Main menu: I don't know where (and maybe what) a change should be applied :-/ (helpwanted)

(In reply to comment #43) >P.T. menubutton got the tooltip after restoring fillInBTTooltip() per comment 42. I don't see how that happens, there's no change to bookmarks-button that I see. >Main menu: I don't know where (and maybe what) a change should be applied :-/ <menu id="BookmarksMenu"> needs the tooltip too.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051114 SeaMonkey/1.5a] (nightly) (W98SE) (In reply to comment #44) > >P.T. menubutton got the tooltip after restoring fillInBTTooltip() per comment 42. > I don't see how that happens, there's no change to bookmarks-button that I see. (I wouldn't know, yet they do.) > >Main menu: I don't know where (and maybe what) a change should be applied :-/ > <menu id="BookmarksMenu"> needs the tooltip too. Well, that what I was relunctant to guess. It works, even a little too well for my taste: The 4 items in its |menupopup| also display a tooltip :-/ Even for the 3rd/disabled item :-// I let you decide if something can be done about it, or if that's acceptable...

Comment on attachment 203442 [details] [diff] [review] (Bv3-SM) OK, so this patch is no good.

Comment on attachment 202690 [details] [diff] [review] (Bv2-SM) This patch is fine, but I'd like the bookmarksMenu change (as in attachment 203442 [details] [diff] [review]) plus the same for the bookmarks-button in navigator.xul, thanks.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051203 SeaMonkey/1.5a] (nightly) (W98SE) Bv3-SM, with comment 47 suggestion(s), and after some emailing between Neil and me. All seems to work as expected, except folders, for which I guess the auto-opening feature "stops" the tooltip one. Unless there is a way around that, I feel like this patch is satisfying.

Comment on attachment 205441 [details] [diff] [review] (Bv4-SM) Almost there, sort of... I don't see a tooltip on the bookmarks button's bookmarks though. <toolbarbutton type="menu" id="bookmarks-button" class="bookmark-item"> >+ if (title && (title != url)) { > tooltipTitle.setAttribute("value", title); >+ tooltipTitle.hidden = false; >+ } else >+ tooltipTitle.hidden = true; I don't like the "} else" style (both times). I can think of three options: 1. Just put the {}s back (i.e. "} else {") 2. Reverse the if so you can swap the then and the else (i.e. "else {") 3. Always set the attribute, and then use a boolean expression to set the hidden property.

(In reply to comment #45) > (In reply to comment #44) > > >P.T. menubutton got the tooltip after restoring fillInBTTooltip() per comment 42. > > I don't see how that happens, there's no change to bookmarks-button that I see. > > (I wouldn't know, yet they do.) and (In reply to comment #49) > (From update of attachment 205441 [details] [diff] [review] [edit]) > Almost there, sort of... I don't see a tooltip on the bookmarks button's > bookmarks though. > <toolbarbutton type="menu" id="bookmarks-button" class="bookmark-item"> Neil is right (;-)), I was misunderstanding what he meant, because I had turn off the Bookmarks button on the P.T..

Bv4-SM, with comment 49 suggestion(s), and after some emailing between Neil and me. This patch now has 3 parts: *Adds tooltips for bookmarks (== this bug). *Removes some unwanted "visual features" of the P.T. Bookmarks button popup. *Does some additionnal code+space cleanup.

Comment on attachment 207285 [details] [diff] [review] (Bv5-SM) Sorry, still not quite right... >+ oncommand="BookmarksMenu.loadBookmark(event, this.database)" Might as well drop in a ; at the end of the JS statement ;-) >- <menuitem accesskey="&addCurPageCmd.accesskey;" key="addBookmarkKb" observes="Browser:AddBookmark"/> >- <menuitem accesskey="&addCurPageAsCmd.accesskey;" key="addBookmarkAsKb" observes="Browser:AddBookmarkAs"/> >+ <menuitem accesskey="&addCurPageCmd.accesskey;" observes="Browser:AddBookmark"/> >+ <menuitem accesskey="&addCurPageAsCmd.accesskey;" observes="Browser:AddBookmarkAs"/> > <menuitem id="PT_bookmarks_groupmark" observes="Browser:AddGroupmarkAs"/> >- <menuseparator/> >- <menuitem accesskey="&manBookmarksCmd.accesskey;" key="manBookmarkKb" observes="Browser:ManageBookmark"/> >+ <menuitem accesskey="&manBookmarksCmd.accesskey;" observes="Browser:ManageBookmark"/> See later for possibility of observes/command conversion. Either way the accesskeys don't need to be duplicated here, because the <command>s already have them, and you'll notice that the other copy of the menuitems don't have access keys. >- <command id="cmd_handleBackspace" oncommand="BrowserHandleBackspace();" /> >- <command id="cmd_handleShiftBackspace" oncommand="BrowserHandleShiftBackspace();" /> >+ <command id="cmd_handleBackspace" oncommand="BrowserHandleBackspace();"/> >+ <command id="cmd_handleShiftBackspace" oncommand="BrowserHandleShiftBackspace();"/> Don't bother changing non-bookmark-related items. > <menupopup id="menu_BookmarksPopup" > onpopupshowing="updateGroupmarkMenuitem('bookmarks_groupmark');"> > <!-- for some reason these don't work as command="" --> Aha, well the reason for that is that updateGroupmarkMenuitem needs to update Browser:AddGroupmarkAs and not the individual menuitems directly (which will then not need ids). >- <menuitem key="addBookmarkKb" observes="Browser:AddBookmark"/> >+ <menuitem key="addBookmarkKb" observes="Browser:AddBookmark"/> > <menuitem key="addBookmarkAsKb" observes="Browser:AddBookmarkAs"/> > <menuitem id="bookmarks_groupmark" observes="Browser:AddGroupmarkAs"/> >- <menuitem key="manBookmarkKb" observes="Browser:ManageBookmark"/> >+ <menuitem key="manBookmarkKb" observes="Browser:ManageBookmark"/> The observes= originally lined up vertically with each other, but then the groupmark menuitem spoiled it... >+ if (!(tooltipElement.hidden = (!title || (title === url)))) The old code only used != , do you have a reason for the === ? Also see below. >+ if (!(tooltipElement.hidden = !url)) >+ tooltipElement.setAttribute("value", url); Unfortunately this is too compact... I think two separate statements for hidden and value works best.

Bv5-SM, with comment 52 suggestion(s). (In reply to comment #52) > >+ if (!(tooltipElement.hidden = (!title || (title === url)))) > The old code only used != , do you have a reason for the === ? I reversed the test(s), and '3=' is supposed to be faster than '2='. Tested on [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051230 SeaMonkey/1.5a] (nightly) (W98SE)

(See comment 53.)

Comment on attachment 207387 [details] [diff] [review] (Bv6-SM) >- const disabled = gBrowser.browsers.length == 1; >+ const disabled = (gBrowser.browsers.length === 1); Sorry, we're not changing style here just because of a minuscule perf win. r+sr=me with this put back (and the other === changed too). > var BookmarksMenu = { > _selection:null, > _target:null, > _orientation:null, >- >+ > Might as well just have one blank line here.

Bv6-SM, with comment 55 suggestion(s). Keeping {{ (Bv6-SM) patch 2006-01-02 16:50 PST 16.46 KB neil.parkwaycc.co.uk: review+ neil.parkwaycc.co.uk: superreview+ }}

Comment on attachment 207816 [details] [diff] [review] (Bv6a-SM) [Checked in: Comment 57] Check in: { 2006-01-08 11:45 bugzilla%standard8.demon.co.uk }

Comment on attachment 207816 [details] [diff] [review] (Bv6a-SM) [Checked in: Comment 57] Neil, I'd like to submit this patch for (Gv1.8 =) SMv1.1 branch, but I'm not too sure on how to proceed...

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

Will there also be a pref to switch bookmarks tooltips off? I mean without turning off all tooltips.

Bv6a-SM-181, unbitrotted for checkin to current 1.8.1 branch.

Comment on attachment 220752 [details] [diff] [review] (Bv6a-SM-181) [Checkin: Comment 62] Checkin: { 2006-05-05 06:53 bugzilla%standard8.demon.co.uk }

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060505 SeaMonkey/1.1a] (tinderbox-builds, 2006050514) (W98SE) V.Fixed for SeaMonkey on 1.8.1 branch.

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

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

Hey this Bug seems to have been forgotten about, but I'd suggest it be nominated as a priority ("blocking" on the trunk) due to the introduction of Places in the upcoming Firefox 3.x Firefox 3.x introduces a 'Places' toolbar button (it's actually a bookmarks folder), and through the use of sub-folders within it, allows users to quickly see which sites they visited recently, which ones they've been to most, which ones they've tagged recently, etc. And while users who have always made use of their bookmarks toolbar are used to seeing a tooltip with Title & URL when an item is hovered over, they'll be wondering why the heck their Places items don't show that information too. Therefore as long as this Bug remains unresolved, the current situation is one of denied potential and great inconsistency. Consider the (I suspect) fact that the majority of Firefox users likely haven't ever created & used a bookmarks toolbar folder before, but are being given one now, and it means ALL Firefox users who make use of 3's highly-touted and long-awaited innovation will be exposed to this Bug. Therefore I believe it's very important this be fixed ASAP. By the way, there is an extension called "Boox" https://addons.mozilla.org/en-US/firefox/addon/2615 that solved this problem in Firefox-2 beautifully (among other included handy improvements), and even allowed users to select which info to show in the tooltip. I was going to suggest modelling a fix based on that(with permission/cooperation, check licence), but after bumping the extension maxversion to 3.0.*, see that it unfortunately is not working in Minefield. I should of course mention that tooltips still don't appear at all within the Bookmarks Menu. A tooltip with the page title is not needed here for obvious reasons, but the URL should be being shown. Yes, the pointed-to URL in the Status area of the statusbar is shown, however with enough bookmarks placed in the root bookmarks folder, the rolled-out bookmarks menu COVERS the default 'Status' statusbar area, which is no good - and/or a user may have so many extensions installed which have placed icons in the statusbar, their Status area has been reduced to barely more than a few fingerwidths, in any case not enough for many full-length URLs.

Comment on attachment 202679 [details] [diff] [review] (Av3c-FF) This patch has been waiting review (and bitrotting) for 2 years ! Sad :-(

I can't help anymore: this will need someone who knows (what) Places (wants)...

FWIW, it seems I had prepared an Av3c-FF sync'ed to Bv6a-SM, at the time.

Adapted for Firefox which now uses the Places toolbar.

Comment on attachment 308255 [details] [diff] [review] ff-places-v1 Sorry, I'm not a Places peer.

What's the binding change for?

(In reply to comment #72) > What's the binding change for? For some reason, the chevron's menupopup does not show the tooltip implicitely, so I had to make it explicitely inherit the tooltip attribute from the binding parent.

Any news about this bug? It's still present on Firefox 3 pre

Last posted patch (ff-places-v1) works perfectly.

Fixed a cosmetic problem with the last patch (ff-places-v1). It didn't show tooltips longer than 70 chars.

(In reply to comment #77) > Fixed a cosmetic problem with the last patch (ff-places-v1). > It didn't show tooltips longer than 70 chars. > Huh? Long tooltips show up just fine. One of the reasons for doing tooltips is that the title often gets cut off in the menu, and the tooltip is a nice way for people to get the title. 65 characters is barely any longer than the current title cutoff. ---- Also, bug 419313 is very similar (almost a dupe), and it was granted wanted+.

This is Andreas Wuest's patch, with one minor change to extend the behavior to the bookmarks menu, like what SeaMonkey currently does (and not limit this to just the bookmark menus found on the personal toolbar). Mrtb: Tooltips should automatically resize and truncate to what the available space is, so there is no need to truncate them manually. Given that the motivation is to allow users to view the full title, I think it's best to avoid manual truncation. Reviewers and drivers: This should be a fairly low-risk patch, because all the functionality is already present in current the code. All that this patch is doing is "flipping the switch", so to speak, and turning it on for the bookmark menus. Allowing users to see the full title through a tooltip will be good for usability.

I see it with the last nightly and with a clean new profile. Firefox 3 pre cut off tooltip's title when it is longer than 70 chars.

(In reply to comment #80) > I see it with the last nightly and with a clean new profile. > Firefox 3 pre cut off tooltip's title when it is longer than 70 chars. > That's an old bug with tooltips in themed Windows. It doesn't affect Classic (and probably doesn't affect Lin/Mac, though I'm not 100% sure about that). Note that tooltips already show up for bookmarks directly on the personal toolbar, and if you add that bookmark to Firefox 2's personal toolbar, you'll get the same cutoff effect. The cutoff isn't character-based, but width-based, so in this particular case, it's 90 characters.

This bug is about using the bookmark tooltip, which has always existed for bookmarks sitting on the bookmark toolbar, in bookmark menus. The problem of a long tooltip being cut off in themed Windows (but not in Classic, which explains why I didn't see the problem at first) has been around for a very long time. The screenshot that I just attached is from Firefox 1.0. This is a separate unrelated issue, the fix for which is probably somewhere in widgets/src/windows/ and not here in toolkit/. I'm also not fond of manual cutoffs, because that doesn't solve the problem of the tooltip being cut off--all that it does is give us more control over where it's cut off, and it'll just end up cutting off the tooltip earlier than it would've otherwise been cut off, which would not be desirable. So let's file this tooltip cutoff thing as a separate Win32 Widget bug (I suspect that there probably already is a bug for it) and focus on this bug here, which is flipping the switch to turn on tooltips for bookmarks in menus.

Filed bug 431325 for that issue

However the last patch of this bug (ff-places-v1.2) works perfectly.

Requesting review. LOW-Risk patch. Summary of patch: -- Fixes all the problems still present on firefox... * It has the same behavior of SM2 (see attachment 220752 [details] [diff] [review]) * It display tooltips on all the links, folders and subfolders of bookmarks toolbar * It display tooltips also on bookmarks menu (links, folders, subfolders) * It doesn't display a tooltip for an items without title and url

does this fix bug 244371?

No, it doesn't

I created this addon https://addons.mozilla.org/it/firefox/addon/7314 that adds this useful feature to Firefox 3. It fixes also bug 431325.

It includes last patch (ff-places-v2.0)

IMHO this feature should be included on Firefox 3.1

Nice to have, certainly.

Comment on attachment 308255 [details] [diff] [review] ff-places-v1 I guess this patch is obsolete.

Requesting review. LOW-Risk patch. Summary of patch: -- Fixes all the problems still present on firefox... * It has the same behavior of SM2 (attachment 220752 [details] [diff] [review]) * It display tooltips on all the links, folders and subfolders of bookmarks toolbar * It display tooltips also on bookmarks menu (links, folders, subfolders) * It doesn't display a tooltip for an items without title and url Now tooltips are shown also on history menù

Comment on attachment 341312 [details] [diff] [review] ff-places-2.1 (rev 0) refresh because of bug 431325 landing >Index: browser/components/places/content/toolbar.xml >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v >retrieving revision 1.157 >diff -u -r1.157 toolbar.xml >--- browser/components/places/content/toolbar.xml 28 May 2008 19:04:30 -0000 1.157 >+++ browser/components/places/content/toolbar.xml 1 Oct 2008 17:02:48 -0000 >@@ -78,6 +78,7 @@ > collapsed="true" > onpopupshowing="chevronPopupShowing(event);"> > <xul:menupopup anonid="chevronPopup" >+ xbl:inherits="tooltip" > #ifndef XP_MACOSX > context="placesContext" > #endif >Index: browser/base/content/browser-menubar.inc >=================================================================== >RCS file: /cvsroot/mozilla/browser/base/content/browser-menubar.inc,v >retrieving revision 1.159 >diff -u -r1.159 browser-menubar.inc >--- mozilla/browser/base/content/browser-menubar.inc 19 Apr 2008 07:36:39 -0000 1.159 >+++ mozilla/browser/base/content/browser-menubar.inc 1 Oct 2008 16:56:31 -0000 >@@ -351,7 +351,8 @@ > <menupopup id="goPopup" > type="places" > onpopupshowing="HistoryMenu.onPopupShowing(this);" >- place="place:type=0&amp;sort=4&amp;maxResults=10"> >+ place="place:type=0&amp;sort=4&amp;maxResults=10" >+ tooltip="btTooltip"> > <menuitem id="historyMenuBack" > label="&backCmd.label;" > #ifdef XP_MACOSX >@@ -407,7 +408,8 @@ > openInTabs="children" > oncommand="BookmarksEventHandler.onCommand(event);" > onclick="BookmarksEventHandler.onClick(event);" >- onpopupshowing="BookmarksEventHandler.onPopupShowing(event);"> >+ onpopupshowing="BookmarksEventHandler.onPopupShowing(event);" >+ tooltip="btTooltip"> > <menuitem label="&bookmarkThisPageCmd.label;" > command="Browser:AddBookmarkAs" key="addBookmarkAsKb"/> > <menuitem id="subscribeToPageMenuitem" >Index: browser/base/content/browser-places.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/base/content/browser-places.js,v >retrieving revision 1.127 >diff -u -r1.127 browser-places.js >--- mozilla/browser/base/content/browser-places.js 7 May 2008 10:14:51 -0000 1.127 >+++ mozilla/browser/base/content/browser-places.js 1 Oct 2008 17:15:37 -0000 >@@ -744,31 +744,30 @@ > }, > > fillInBTTooltip: function(aTipElement) { >- // Fx2XP: Don't show tooltips for bookmarks under sub-folders >- if (aTipElement.localName != "toolbarbutton") >- return false; >- >- // Fx2XP: Only show tooltips for URL items >- if (!PlacesUtils.nodeIsURI(aTipElement.node)) >+ if (!aTipElement.node) > return false; >+ >+ var title = aTipElement.label; >+ >+ // Show URL only for links >+ if (PlacesUtils.nodeIsURI(aTipElement.node)) >+ var url = aTipElement.node.uri; > >- var url = aTipElement.node.uri; >- if (!url) >+ // Don't show a tooltip without any data. >+ if (!title && !url) > return false; > >- var tooltipUrl = document.getElementById("btUrlText"); >- tooltipUrl.value = url; >- >- var title = aTipElement.label; > var tooltipTitle = document.getElementById("btTitleText"); >- if (title && title != url) { >- tooltipTitle.hidden = false; >- tooltipTitle.value = title; >- } >- else >- tooltipTitle.hidden = true; >+ tooltipTitle.hidden = !title || (title == url); >+ if(!tooltipTitle.hidden) >+ tooltipTitle.textContent = title; >+ >+ var tooltipUrl = document.getElementById("btUrlText"); >+ tooltipUrl.hidden = !url; >+ if (!tooltipUrl.hidden) { >+ tooltipUrl.value = url; > >- // show tooltip >+ // Show tooltip > return true; > } > }; > Little bug on patch, this is the correct one.

Sorry, the previous patch was incorrect, this is the correct one.

Another little fix added.

a drive-by review - // Fx2XP: Don't show tooltips for bookmarks under sub-folders - if (aTipElement.localName != "toolbarbutton") - return false; - - // Fx2XP: Only show tooltips for URL items - if (!PlacesUtils.nodeIsURI(aTipElement.node)) + if (!aTipElement.node) return false; + nit: trailing spaces + var title = aTipElement.label; why using label instead of aTipElement.node.title? for nodes without a title we generate a special cropped title from the uri. so using the label, when you compare title and uri, it will be false in this case. + var title = aTipElement.label; + nit: trailing spaces, there are others later, you should fix them in all the patch + if (!tooltipUrl.hidden) { + tooltipUrl.value = url; where is the closing brace? probably you should remove this "{"

(In reply to comment #98) > a drive-by review > > - // Fx2XP: Don't show tooltips for bookmarks under sub-folders > - if (aTipElement.localName != "toolbarbutton") > - return false; > - > - // Fx2XP: Only show tooltips for URL items > - if (!PlacesUtils.nodeIsURI(aTipElement.node)) > + if (!aTipElement.node) > return false; > + > > nit: trailing spaces Ok > + var title = aTipElement.label; > > why using label instead of aTipElement.node.title? for nodes without a title we > generate a special cropped title from the uri. so using the label, when you > compare title and uri, it will be false in this case. I have never had this problem with this patch. However, ok. > + var title = aTipElement.label; > + > > nit: trailing spaces, there are others later, you should fix them in all the > patch > > + if (!tooltipUrl.hidden) { > + tooltipUrl.value = url; > > where is the closing brace? probably you should remove this "{" Ok. Thanks for this.

+ // Don't show a tooltip without any data. + if (!title && !url) still a couple trailing spaces on these 2 lines. then you can ask review to a peer (mano, dietrich, gavin or mconnor)

Comment on attachment 341611 [details] [diff] [review] ff-places-2.2, changes based on mak77 review We should better ask Gavin to get a quicker review.

Comment on attachment 341611 [details] [diff] [review] ff-places-2.2, changes based on mak77 review >Index: base/content/browser-places.js >+ var title = aTipElement.node.title; >+ // Show URL only for links >+ if (PlacesUtils.nodeIsURI(aTipElement.node)) >+ var url = aTipElement.node.uri; >+ // Don't show a tooltip without any data. >+ if (!title && !url) This means we'll show tooltips for folders (which have a title), which is undesirable. I don't know why it was decided to start showing tooltips for items without URLs. (There's still a lot of trailing whitespace in this patch, and it didn't apply cleanly.)

(In reply to comment #102) > (From update of attachment 341611 [details] [diff] [review]) > >Index: base/content/browser-places.js > > >+ var title = aTipElement.node.title; > >+ // Show URL only for links > >+ if (PlacesUtils.nodeIsURI(aTipElement.node)) > >+ var url = aTipElement.node.uri; > >+ // Don't show a tooltip without any data. > >+ if (!title && !url) > > This means we'll show tooltips for folders (which have a title), which is > undesirable. I don't know why it was decided to start showing tooltips for > items without URLs. > > (There's still a lot of trailing whitespace in this patch, and it didn't apply > cleanly.) Ok.

Comment on attachment 341830 [details] [diff] [review] ff-places-2.3: no more trailing spaces and tooltips for folders >Index: base/content/browser-places.js >+ var tooltipTitle = document.getElementById("btTitleText"); >+ tooltipTitle.hidden = !title || (title == url); >+ if (!tooltipTitle.hidden) >+ tooltipTitle.textContent = title; Why textContent instead of value?

It's used to wrap the title's label if it's too long to be show. See bug 431325 for more details.

Shouldn't you use it for the tooltip as well, then?

The previous one was based on cvs code (firefox 3.0).

It's the same indentical patch but based on hg code.

(In reply to comment #106) > Shouldn't you use it for the tooltip as well, then? I meant "URL" instead of tooltip, but Gabriele pointed out on IRC that it's cropped so it doesn't matter.

pushed http://hg.mozilla.org/mozilla-central/rev/b2248e8b2a74

As what I can see is that it doesn't work for the bookmarks menu on OS X. Don't we have a chance to show the titles there because it's a native menu?

(In reply to comment #111) > As what I can see is that it doesn't work for the bookmarks menu on OS X. Don't > we have a chance to show the titles there because it's a native menu? Dietrich, can you give me an answer to that question? If not, whom I could ask for?

(In reply to comment #111) > As what I can see is that it doesn't work for the bookmarks menu on OS X. Don't > we have a chance to show the titles there because it's a native menu? After some investigation it doesn't seem to be possible with OS X. At least if there will be a way we should handle this in its own bug. Verfied the patch on trunk with: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081017 Minefield/3.1b2pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081017 Minefield/3.1b2pre ID:20081017020232

actually when draggging these new tooltips are causing a lot of headache since as soon as one start dragging a place node in a popup the tooltip appear and as soon as the mouse touches the tooltip all popups are dismissed. if bug 312852 is not fixed before the release i would ask to backout this

Why this bug hasn't been fixed for 1.9.0 yet? We still have it on 1.8.1.

(In reply to comment #118) > Why this bug hasn't been fixed for 1.9.0 yet? We still have it on 1.8.1. Because it's not a 1) stability fix, 2) security fix, or 3) fix for regression from previous *maintenance* release. And it's never been nominated for anything.

Test case https://litmus.mozilla.org/show_test.cgi?id=6752 was updated for regression testing.

Disregard the previous test case link, this is the test case that was updated for regression testing. https://litmus.mozilla.org/show_test.cgi?id=6376

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