237592 - Bookmarks/RSS items should always show tooltip when hovering with mouse

Status: VERIFIED FIXED

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

Description

[David Tenser [:djst]]

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.

Comment 1

[Warner Young]

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)

Comment 2

[Ron Baldwin]

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.

Comment 3

[Roasted]

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.

Comment 4

[Richard Corbin]

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

Comment 5

[eric martin]

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

Comment 6

[Bill Mason]

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

Comment 7

[Dan]

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

Comment 8

[Jon Hall]

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 />

Comment 9

[Asa Dotzler [:asa]]

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?

Comment 10

[Richard Corbin]

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

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 12

[Kevin Brosnan]

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

Comment 13

[Jason Barnabe (np)]

Attachment: (Av1-FF) patch

Patch per comment #8.

Note that bug 244385 is making the tooltip disappear almost immediately.

Comment 14

[Serge Gautherie (:sgautherie)]

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

Comment 15

[Jason Barnabe (np)]

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

Comment 16

[Serge Gautherie (:sgautherie)]

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

[Serge Gautherie (:sgautherie)]

Comment on attachment 173038 [details] [diff] [review]
(Av1-FF) patch

(damn it, bugzilla bug...)

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-MAS) <navigatorOverlay.xul>

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 ?

Comment 19

[[e-mail]]

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

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a-MAS) <navigatorOverlay.xul>

Bv1, with comment 19 suggestion(s).

{{ (quoting comment 13)
Note that bug 244385 is making the tooltip disappear almost immediately.
}}

Comment 21

[[e-mail]]

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.

Comment 22

[Serge Gautherie (:sgautherie)]

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

Comment 23

[Andreas Wuest]

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

Comment 24

[Andreas Wuest]

Attachment: (Av2-FF) Variant 1: more memory hungry

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.

Comment 25

[Andreas Wuest]

Attachment: (Av3-FF) Variant 2: less memory hungry, but tooltips also on folders and function menu entries

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!

Comment 26

[[e-mail]]

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

Comment 27

[Andreas Wuest]

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

Comment 28

[[e-mail]]

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

Comment 29

[Andreas Wuest]

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

Comment 30

[Andreas Wuest]

Attachment: (Av3a-FF) Makes tooltips appear also on bookmarks which are nested inside a folder in the bookmarks toolbar

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 31

[Mike Connor [:mconnor]]

Comment on attachment 173038 [details] [diff] [review]
(Av1-FF) patch

this is bad, for the reasons Neil outlined in his comment.

Comment 32

[Mike Connor [:mconnor]]

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;

Comment 33

[Mike Connor [:mconnor]]

would be nice to have, but not a blocker.

Comment 34

[Adam Guthrie]

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

Comment 35

[Serge Gautherie (:sgautherie)]

Attachment: (Av3b-FF)

Av3a, with comment 32 suggestion(s),
and code simplification.

I don't use FF: Could you test/review/check in this patch ? Thanks.

Comment 36

[Serge Gautherie (:sgautherie)]

Attachment: (Av3c-FF)

Av3b, with a few (unrelated) FF<-SM synchronization.

I don't use FF: Could you test/review/check in this patch ? Thanks.

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-SM)

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 38

[[e-mail]]

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;

Comment 39

[[e-mail]]

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

Comment 40

[[e-mail]]

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

Comment 41

[Serge Gautherie (:sgautherie)]

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

Comment 42

[[e-mail]]

(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

Comment 43

[Serge Gautherie (:sgautherie)]

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

Comment 44

[[e-mail]]

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

Comment 45

[Serge Gautherie (:sgautherie)]

Attachment: (Bv3-SM)

[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 46

[[e-mail]]

Comment on attachment 203442 [details] [diff] [review]
(Bv3-SM)

OK, so this patch is no good.

Comment 47

[[e-mail]]

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.

Comment 48

[Serge Gautherie (:sgautherie)]

Attachment: (Bv4-SM)

[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 49

[[e-mail]]

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.

Comment 50

[Serge Gautherie (:sgautherie)]

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

Comment 51

[Serge Gautherie (:sgautherie)]

Attachment: (Bv5-SM)

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 52

[[e-mail]]

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.

Comment 53

[Serge Gautherie (:sgautherie)]

Attachment: (Bv6-SM) [Wrong file]

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)

Comment 54

[Serge Gautherie (:sgautherie)]

Attachment: (Bv6-SM)

(See comment 53.)

Comment 55

[[e-mail]]

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.

Comment 56

[Serge Gautherie (:sgautherie)]

Attachment: (Bv6a-SM) [Checked in: Comment 57]

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 57

[Serge Gautherie (:sgautherie)]

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 58

[Serge Gautherie (:sgautherie)]

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

Comment 59

[Phil Ringnalda (:philor)]

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

Comment 60

[Ria Klaassen (not reading all bugmail)]

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

Comment 61

[Serge Gautherie (:sgautherie)]

Attachment: (Bv6a-SM-181) [Checkin: Comment 62]

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

Comment 62

[Serge Gautherie (:sgautherie)]

Comment on attachment 220752 [details] [diff] [review]
(Bv6a-SM-181)
[Checkin: Comment 62]


Checkin: { 2006-05-05 06:53	bugzilla%standard8.demon.co.uk }

Comment 63

[Serge Gautherie (:sgautherie)]

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

Comment 64

[Mike Connor [:mconnor]]

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

Comment 65

[Phil Ringnalda (:philor)]

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

Comment 66

[RenegadeX]

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 67

[Serge Gautherie (:sgautherie)]

Comment on attachment 202679 [details] [diff] [review]
(Av3c-FF)

This patch has been waiting review (and bitrotting) for 2 years ! Sad :-(

Comment 68

[Serge Gautherie (:sgautherie)]

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

Comment 69

[Serge Gautherie (:sgautherie)]

Attachment: (Av3d-FF)

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

Comment 70

[Andreas Wuest]

Attachment: ff-places-v1

Adapted for Firefox which now uses the Places toolbar.

Comment 71

[[e-mail]]

Comment on attachment 308255 [details] [diff] [review]
ff-places-v1

Sorry, I'm not a Places peer.

Comment 72

[Asaf Romano (gone)]

What's the binding change for?

Comment 73

[Andreas Wuest]

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

Comment 75

[Gabriele Best [:Gabri]]

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

Comment 76

[Gabriele Best [:Gabri]]

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

Comment 77

[Gabriele Best [:Gabri]]

Attachment: ff-places-v1.1: set width's limit to 65 chars

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

Comment 78

[Kai Liu]

Attachment: long tooltips work fine

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

Comment 79

[Kai Liu]

Attachment: ff-places-v1.2: extended to bookmarks menu (behavior now identical to SM2, except folders don't get tooltips)

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.

Comment 80

[Gabriele Best [:Gabri]]

Attachment: Tooltip's title is cut off

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.

Comment 81

[Kai Liu]

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

Comment 82

[Kai Liu]

Attachment: tooltip cutoff in Firefox 1.0

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.

Comment 83

[Gabriele Best [:Gabri]]

Filed bug 431325 for that issue

Comment 84

[Gabriele Best [:Gabri]]

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

Comment 85

[Gabriele Best [:Gabri]]

Attachment: ff-places-v2.0

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

Comment 86

[Dietrich Ayala (:dietrich)]

does this fix bug 244371?

Comment 87

[Gabriele Best [:Gabri]]

No, it doesn't

Comment 88

[Gabriele Best [:Gabri]]

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.

Comment 89

[Gabriele Best [:Gabri]]

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

Comment 90

[Gabriele Best [:Gabri]]

IMHO this feature should be included on Firefox 3.1

Comment 91

[Mike Connor [:mconnor]]

Nice to have, certainly.

Comment 93

[Steffen Wilberg]

Comment on attachment 308255 [details] [diff] [review]
ff-places-v1

I guess this patch is obsolete.

Comment 94

[Gabriele Best [:Gabri]]

Attachment: ff-places-2.1 (rev 0) refresh because of bug 431325 landing

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 95

[Gabriele Best [:Gabri]]

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.

Comment 96

[Gabriele Best [:Gabri]]

Attachment: ff-places-2.1 (rev 1) bugfix

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

Comment 97

[Gabriele Best [:Gabri]]

Attachment: ff-places-2.1 (rev 2) another bugfix

Another little fix added.

Comment 98

[Marco Bonardo [:mak]]

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 "{"

Comment 99

[Gabriele Best [:Gabri]]

Attachment: ff-places-2.2, changes based on mak77 review

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

Comment 100

[Marco Bonardo [:mak]]

+    // 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 101

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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 102

[:Gavin Sharp [email: [e-mail]]]

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

Comment 103

[Gabriele Best [:Gabri]]

Attachment: ff-places-2.3: no more trailing spaces and tooltips for folders

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

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 105

[Gabriele Best [:Gabri]]

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

Comment 106

[:Gavin Sharp [email: [e-mail]]]

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

Comment 107

[Gabriele Best [:Gabri]]

Attachment: [Check-in needed] final patch

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

Comment 108

[Gabriele Best [:Gabri]]

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

Comment 109

[:Gavin Sharp [email: [e-mail]]]

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

Comment 110

[Marco Bonardo [:mak]]

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

Comment 111

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 114

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 115

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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

Comment 116

[Marco Bonardo [:mak]]

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

Comment 118

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 119

[Samuel Sidler (old account; do not CC)]

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

Comment 120

[Aakash Desai [:aakashd]]

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

Comment 121

[Aakash Desai [:aakashd]]

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

Comment 122

[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