244371 - Show a tooltip in Bookmarks and History sidebars.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Stephen Smith]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

I don't think anyone has filed this enhancement before (and I'm not sure if
Seamonkey lacks similar support), but I searched through the existing entries
for the History sidebar and I'm surprised that nobody has suggested a similar
enhancement. 

I use the History sidebar in Firefox fairly frequently, and while it shows the
title of web pages in the sidebar I think it would be really useful if the URL
of a particular entry was shown as a tool tip text when the mouse is paused over
the entry. This could easily apply to the Bookmarks sidebar as well.

Reproducible: Always
Steps to Reproduce:
1. Open Bookmarks or History sidebar.
2. Pause mouse over entry in sidebar.
3. Notice that there's no URL displayed as a tool tip text.

Actual Results:  
No URL is shown.

Expected Results:  
The URL entry is shown in the sidebar.

Comment 1

[Mike Connor [:mconnor]]

seems OK, definitely not something we'll look at doing before 1.0 though

Comment 2

[Walter K.]

IMHO, this bug is WONTFIX. The correct behavior is bug 239429 - URL in the
status bar on mouse over. 

Tool tip if for cropped page names. The only case when tool tip with URL should
be shown is when page has no title, and URL is effectively the page name.

Comment 3

[Richard Corbin]

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

Comment 4

[Dan]

(In reply to comment #2)
> IMHO, this bug is WONTFIX. The correct behavior is bug 239429 - URL in the
> status bar on mouse over. 
> 
> Tool tip if for cropped page names. The only case when tool tip with URL should
> be shown is when page has no title, and URL is effectively the page name.

Why is that the correct behaviour?.. effectively, if the user has turned off the
status bar they will no longer benefit.. and IMHO it's better in terms of
usability to not have to divert your eyes between the bookmark you have selected
and the status bar every time just to see where you're going to be headed..

Also, IE and Opera users migrating will find this familiar, which can't be bad..
I honestly can't think of a single reason not to do it... in fact, let's add the
created date (as Opera) and bookmark description while we're at it

Comment 5

[Stephen Smith]

Adding further information to the tool tip text is an interesting idea.

If the sidebar item shows the page <title> element, what's the point in the tool
tip text showing exactly the same <title>? I can already see it! 

(In reply to comment #4)
> (In reply to comment #2)
> > IMHO, this bug is WONTFIX. The correct behavior is bug 239429 - URL in the
> > status bar on mouse over. 
> > 
> > Tool tip if for cropped page names. The only case when tool tip with URL should
> > be shown is when page has no title, and URL is effectively the page name.
> 
> Why is that the correct behaviour?.. effectively, if the user has turned off the
> status bar they will no longer benefit.. and IMHO it's better in terms of
> usability to not have to divert your eyes between the bookmark you have selected
> and the status bar every time just to see where you're going to be headed..
> 
> Also, IE and Opera users migrating will find this familiar, which can't be bad..
> I honestly can't think of a single reason not to do it... in fact, let's add the
> created date (as Opera) and bookmark description while we're at it

Comment 6

[Dan]

Definitely.. just to clarify, I agree having the status bar show the URL is
standard behaviour for all links and should also be applied in this case, but I
feel enhancing the tool tip text would be an invaluable improvement in addition

Comment 7

[Ivan Ičin]

(In reply to comment #5)
> Adding further information to the tool tip text is an interesting idea.
> 
> If the sidebar item shows the page <title> element, what's the point in the tool
> tip text showing exactly the same <title>? I can already see it! 

The point is in all those titles that are collapsed and couldn't be read
otherwise. Statusbar is for URL.

Comment 8

[Dan]

(In reply to comment #7)
> The point is in all those titles that are collapsed and couldn't be read
> otherwise. Statusbar is for URL.

I agree, but currently that isn't working either for me (FF1.0 Milestone WinXP)..

My preferred format would be:

<title>
<url>
<description>

As far as I could see this would cover all bases, provide useful information
about the bookmark, consistency with Opera & IE and make user-accessible use of
the description field.. 

Comment 9

[bomfog]

Should this be moved to Places?

Comment 11

[Gabriele Best [:Gabri]]

I'd like to work on it.

Comment 12

[Gabriele Best [:Gabri]]

Nobody can fix this bug without tree tooltip support (bug 421167)

Comment 13

[Shawn Wilsher :sdwilsh]

Note: on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20081210 Shiretoko/3.1b3pre history displays the url, and bookmarks displays the title.

Comment 14

[Gabriele Best [:Gabri]]

Attachment: Patch v1.0

This patch add a tooltip to bookmarks and history sidebar.
The tooltip contains TITLE and URL of the item.

Comment 15

[Gabriele Best [:Gabri]]

That patch is very low impact and a minor fix.
It doesn't displays a tooltip over the folders but only over the links.

Comment 16

[Gabriele Best [:Gabri]]

Attachment: Patch v2.0

That patch is very low impact and a minor fix.

This patch adds a tooltip to bookmarks and history sidebar.
The tooltip contains TITLE and URL of the item.

It also displays a tooltip over the folders with a cropped title.

Comment 18

[Gabriele Best [:Gabri]]

Attachment: Screen after patch

This is a screen after patch 2.0 is applied

Comment 19

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

Comment on attachment 366594 [details]
Screen after patch

uir=beltzner, this is consistent with how we do things on the bookmark toolbar, and one step closer to removing need for the status bar.

Comment 20

[David McRitchie]

Since Bug 259199 was redirected here, I hope that this bug has been expanded in scope  to also include tooltip for the titles in anchor tags of webpages placed in the sidebar and not just bookmarks in the sidebar.

Comment 21

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

Comment on attachment 364693 [details] [diff] [review]
Patch v2.0

>diff -r 69549aec001c browser/components/places/content/sidebarUtils.js

>+  fillInBHSTooltip: function(aEvent)

>+    // Show a tooltip over non-url node if there is a cropped title
>+    if ((!title || !cropped) && !url)
>+      return false;

This comment doesn't seem to match the logic (or I've misunderstood). Either way, I think it should be refactored... seems like what you want is:

let title = node.title;  
let uri;
if (PlacesUtils.nodeIsURI(node))
  uri = node.uri;

if (!uri && !cropped)
  return false;

Am I missing something?

Comment 22

[Gabriele Best [:Gabri]]

Attachment: Patch v2.1

Ok, code changed.

Comment 23

[Gabriele Best [:Gabri]]

Attachment: Patch v2.1.1

Ok, it is the correct one.

Comment 24

[Nochum Sossonko [:Natch]]

Comment on attachment 368780 [details] [diff] [review]
Patch v2.1.1

Gbriele, does the patch still apply? If not, please post an updated patch, Marco can review if he has time.

Comment 25

[Gabriele Best [:Gabri]]

Yes it does.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 368780 [details] [diff] [review]
Patch v2.1.1

the patch is bitrotted here... i commented on the patch, but please before fixing read final comment.

>diff -r 8a3c6f9a258f browser/components/places/content/bookmarksPanel.xul
>--- a/browser/components/places/content/bookmarksPanel.xul	Sat Mar 14 22:26:06 2009 -0700
>+++ b/browser/components/places/content/bookmarksPanel.xul	Sun Mar 22 15:27:17 2009 +0100
>@@ -63,22 +63,31 @@
> 
>   <hbox align="center">
>     <label value="&search.label;" accesskey="&search.accesskey;" control="search-box"/>
>     <textbox id="search-box" flex="1" type="search" cocoa-size="small"
>              aria-controls="bookmarks-view"
>              oncommand="searchBookmarks(this.value);"/>
>   </hbox>
> 
>+  <!-- bookmarks and history sidebar tooltip -->
>+  <tooltip id="bhsTooltip" noautohide="true"
>+           onpopupshowing="return SidebarUtils.fillInBHSTooltip(event)">
>+    <vbox id="bhsTooltipTextBox" flex="1">
>+      <label id="bhsTitleText" class="tooltip-label" />
>+      <label id="bhsUrlText" crop="center" class="tooltip-label" />
>+    </vbox>
>+  </tooltip>
>+
>   <tree id="bookmarks-view" class="sidebar-placesTree" type="places"
>         flex="1"
>         hidecolumnpicker="true"
>         context="placesContext"
>         onkeypress="SidebarUtils.handleTreeKeyPress(event);"
>         onclick="SidebarUtils.handleTreeClick(this, event, true);"
>         onmousemove="SidebarUtils.handleTreeMouseMove(event);"
>         onmouseout="SidebarUtils.clearURLFromStatusBar();">
>     <treecols>
>       <treecol id="title" flex="1" primary="true" hideheader="true"/>
>     </treecols>
>-    <treechildren id="bookmarks-view-children" view="bookmarks-view" class="sidebar-placesTreechildren" flex="1"/>
>+    <treechildren id="bookmarks-view-children" view="bookmarks-view" class="sidebar-placesTreechildren" flex="1" tooltip="bhsTooltip"/>
>   </tree>
> </page>
>diff -r 8a3c6f9a258f browser/components/places/content/history-panel.xul
>--- a/browser/components/places/content/history-panel.xul	Sat Mar 14 22:26:06 2009 -0700
>+++ b/browser/components/places/content/history-panel.xul	Sun Mar 22 15:27:17 2009 +0100
>@@ -110,24 +110,33 @@
>         <menuitem id="bylastvisited" label="&byLastVisited.label;" 
>                   accesskey="&byLastVisited.accesskey;"
>                   type="radio"
>                   oncommand="this.parentNode.parentNode.setAttribute('selectedsort', 'lastvisited'); GroupBy('lastvisited');"/>
>       </menupopup>
>     </button>
>   </hbox>
> 
>+  <!-- bookmarks and history sidebar tooltip -->
>+  <tooltip id="bhsTooltip" noautohide="true"
>+           onpopupshowing="return SidebarUtils.fillInBHSTooltip(event)">
>+    <vbox id="bhsTooltipTextBox" flex="1">
>+      <label id="bhsTitleText" class="tooltip-label" />
>+      <label id="bhsUrlText" crop="center" class="tooltip-label" />
>+    </vbox>
>+  </tooltip>
>+
>   <tree id="historyTree"
>         class="sidebar-placesTree"
>         flex="1"
>         type="places"
>         context="placesContext"
>         hidecolumnpicker="true"
>         onkeypress="SidebarUtils.handleTreeKeyPress(event);"
>         onclick="SidebarUtils.handleTreeClick(this, event, true);"
>         onmousemove="SidebarUtils.handleTreeMouseMove(event);"
>         onmouseout="SidebarUtils.clearURLFromStatusBar();">
>     <treecols>
>       <treecol id="title" flex="1" primary="true" hideheader="true"/>
>     </treecols>
>-    <treechildren class="sidebar-placesTreechildren" flex="1"/>
>+    <treechildren class="sidebar-placesTreechildren" flex="1" tooltip="bhsTooltip"/>
>   </tree>
> </page>
>diff -r 8a3c6f9a258f browser/components/places/content/sidebarUtils.js
>--- a/browser/components/places/content/sidebarUtils.js	Sat Mar 14 22:26:06 2009 -0700
>+++ b/browser/components/places/content/sidebarUtils.js	Sun Mar 22 15:27:17 2009 +0100
>@@ -117,12 +117,45 @@ var SidebarUtils = {
>         window.top.XULBrowserWindow.setOverLink(cell.uri, null);
>       else
>         this.clearURLFromStatusBar();
>     }
>     else
>       this.clearURLFromStatusBar();
>   },
> 
>+  fillInBHSTooltip: function(aEvent)
>+  {
>+    var row = {}, column = {}, part = {};
>+    var tree = document.getElementById("bookmarks-view") || document.getElementById("historyTree");

cut at 80 chars please
var tree = document.getElementById("bookmarks-view") ||
           document.getElementById("historyTree");

>+
>+    var tbo = tree.treeBoxObject;
>+    tbo.getCellAt(aEvent.clientX, aEvent.clientY, row, column, part);

you don't need part, just pass {}

>+
>+    var node = tree.view.nodeForTreeIndex(row.value);
>+    var cropped = tbo.isCellCropped(row.value, column.value);
>+
>+    var title = node.title;
>+    // Show URL only for links
>+    if (PlacesUtils.nodeIsURI(node))
>+      var url = node.uri;

define var url out of the if please
var url;
if()
 url =

>+
>+    if (!cropped && !url)
>+      return false;

add a comment above this please saying "Show tooltip for containers only if their title is cropped." or something like that

>+    var tooltipTitle = document.getElementById("bhsTitleText");
>+    tooltipTitle.hidden = !title || (title == url);
>+    if (!tooltipTitle.hidden)
>+      tooltipTitle.textContent = title;

what about

if (!title || (title == url))
  tooltipTitle.hidden = true;
else
  tooltipTitle.textContent = title;

could be a petasecond faster!

>+
>+    var tooltipUrl = document.getElementById("bhsUrlText");
>+    tooltipUrl.hidden = !url;
>+    if (!tooltipUrl.hidden)
>+      tooltipUrl.value = url;

ditto

>+
>+    // Show tooltip
>+    return true;
>+  },

that said, it's a pity we cannot share code with bookmarks toolbar/menu tooltips to maintain them in sync, as it is any addition/change to the tooltips will have to be done twice.

I think you could reuse btTooltip here, you should replace it with
<tooltip id="btTooltip" noautohide="true"
             onpopupshowing="return window.top.BookmarksEventHandler.fillInBTTooltip(document.tooltipNode)">
      <vbox id="btTooltipTextBox" flex="1">
      </vbox>
    </tooltip>
in both places (in sidebar and in browser)
in fillInBTTooltip you get aTooltipElement, just check if it is "treechildren", "button" or "menuitem", get the node using tree methods and read cropped from the tree, otherwise move on with node = aTipElement.node and set cropped to false (we cannot detect it on buttons, not sure on menuitems, you could check if it's possible).
Then we could create the elements in the tooltip on the fly. This way we would manage all the logic for all bookmarks/history tooltip in a single location, and adding or changing things should be easier.
Being able to share <tooltip> definition (with content already) would be even better and easier, maybe we could put it in placesOverlay.xul, worth a try.

so i'd say, try to reuse btTooltip everywhere, with some magic should be possible, at that point i'd also rename the tooltip to placesTooltip or bhTooltip (bookmarks-history-tooltip), since it's already no more used only on toolbar (menus are using it).

Comment 27

[Gabriele Best [:Gabri]]

(In reply to comment #26)
> that said, it's a pity we cannot share code with bookmarks toolbar/menu
> tooltips to maintain them in sync, as it is any addition/change to the tooltips
> will have to be done twice.
> 
> I think you could reuse btTooltip here, you should replace it with
> <tooltip id="btTooltip" noautohide="true"
>              onpopupshowing="return
> window.top.BookmarksEventHandler.fillInBTTooltip(document.tooltipNode)">
>       <vbox id="btTooltipTextBox" flex="1">
>       </vbox>
>     </tooltip>
> in both places (in sidebar and in browser)
> in fillInBTTooltip you get aTooltipElement, just check if it is "treechildren",
> "button" or "menuitem", get the node using tree methods and read cropped from
> the tree, otherwise move on with node = aTipElement.node and set cropped to
> false (we cannot detect it on buttons, not sure on menuitems, you could check
> if it's possible).
> Then we could create the elements in the tooltip on the fly. This way we would
> manage all the logic for all bookmarks/history tooltip in a single location,
> and adding or changing things should be easier.
> Being able to share <tooltip> definition (with content already) would be even
> better and easier, maybe we could put it in placesOverlay.xul, worth a try.
> 
> so i'd say, try to reuse btTooltip everywhere, with some magic should be
> possible, at that point i'd also rename the tooltip to placesTooltip or
> bhTooltip (bookmarks-history-tooltip), since it's already no more used only on
> toolbar (menus are using it).

I tried some solutions and I think it could be better if we maintain the 2 functions (fillInBTTooltip and fillInBHSTooltip) separated to avoid the incompatibility of some addons, we could try to make an unified function on the next release I think, but I didn't find a common js file between sidebars and toolbars :(

I made 2 patches:
the first has the same structure of patch 2.1.1 and in the second the tooltips' sources are in the same file as you requested (placesOverlay.xul) so we can  maintain them in sync :)

Comment 28

[Gabriele Best [:Gabri]]

Attachment: Patch v2.2

This patch has the same structure of Patch 2.1.1

Comment 29

[Gabriele Best [:Gabri]]

Attachment: Patch v3.0

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this is not a security sensitive or stability fix, not being high priority is fine to take it for the next version, and unifying is better. Extension developers will even be happier to be able to add things to a single place to create better tooltips through all the ui.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #27)
> (In reply to comment #26)
> I tried some solutions and I think it could be better if we maintain the 2
> functions (fillInBTTooltip and fillInBHSTooltip) separated to avoid the
> incompatibility of some addons, we could try to make an unified function on the
> next release I think, but I didn't find a common js file between sidebars and
> toolbars :(

just use window.top.BookmarksEventHandler.fillInBTTooltip(document.tooltipNode)
should work in both places, add the logic into fillInBTTooltip to manage the two cases.
why can't you use the same tooltip id, like bhTooltip or placesTooltip in both places?
I'd really like them to be the same thing and use the same handler, not just near.

Comment 32

[Gabriele Best [:Gabri]]

I've just finished the patch but I need to speak with you via IRC :)

Comment 33

[Gabriele Best [:Gabri]]

Attachment: Patch v4.0 - Unified function for places tooltips

I used the globalOverlay.js file to make visible the fillInPlacesTooltip function to all the sidebars and toolbars then I wrote the new tooltip source into the placesOverlay.xul file. Finally, I deleted the old tooltips' sources.

Comment 34

[Gabriele Best [:Gabri]]

>+    var tooltipTitle = document.getElementById("bhsTitleText");
>+    if (!title || (title == url))
>+      tooltipTitle.hidden = true;
>+    else
>+      tooltipTitle.textContent = title;
>+
>+    var tooltipUrl = document.getElementById("bhsUrlText");
>+    if (!url)
>+      tooltipUrl.hidden = true;
>+    else
>+      tooltipUrl.value = url;

I've written this code instead of what you suggested because I think it's more clear:

>+  var tooltipTitle = document.getElementById("ptTitleText");
>+  if(!title || (title == url)) {
>+    tooltipTitle.hidden = true;
>+  } else {
>+    tooltipTitle.hidden = false;
>+    tooltipTitle.textContent = title; }
>+
>+  var tooltipUrl = document.getElementById("ptUrlText");
>+  if(!url) {
>+    tooltipUrl.hidden = true;
>+  } else {
>+    tooltipUrl.hidden = false;
>+    tooltipUrl.value = url; }

What do you think?

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 391864 [details] [diff] [review]
Patch v4.0 - Unified function for places tooltips

i'm still not sure why you can's use window.top.BookmarksEventHandler.fillInWhateverTooltip everywhere, that should work, browser.js should be included in browser windows. Where do you see it not working?

a change like this in GlobalOverlay won't be accepted by any toolkit peer, that's a global toolkit file, while those are browser tooltips for a specific component. sorry.

actually, i'm thinking bhTooltip would be better, Places is a component codename, better avoiding it for prefixes in browser.


>diff --git a/browser/components/places/content/placesOverlay.xul b/browser/components/places/content/placesOverlay.xul
>--- a/browser/components/places/content/placesOverlay.xul
>+++ b/browser/components/places/content/placesOverlay.xul
>@@ -54,16 +54,25 @@
>   <script type="application/x-javascript" 
>           src="chrome://browser/content/places/utils.js"/>
>   <script type="application/x-javascript"
>           src="chrome://browser/content/places/controller.js"/>
>   <script type="application/x-javascript"
>           src="chrome://browser/content/places/treeView.js"/>
>   <script type="application/x-javascript"
>           src="chrome://global/content/nsDragAndDrop.js"/>
>+
>+  <!-- places tooltip -->
>+  <tooltip id="placesTooltip" noautohide="true"
>+           onpopupshowing="return fillInPlacesTooltip(event)">

you should still be able to use document.tooltipNode, for the tree it will be the treechildren

>+function fillInPlacesTooltip(aEvent) {
>+  var title, url;
>+  var node, cropped;

define near (or on) use please, this is not C :)

>+  var tree = document.getElementById("bookmarks-view") || 
>+               document.getElementById("historyTree");
>+
>+  if (tree) {

if you use document.tooltipNode (that i'd prefer), you can check aNode.localName == "treechildren", and use parentNode to get tree object

>+    var row = {}, column = {};
>+    var tbo = tree.treeBoxObject;
>+    tbo.getCellAt(aEvent.clientX, aEvent.clientY, row, column, {});
>+
>+    node = tree.view.nodeForTreeIndex(row.value);
>+    cropped = tbo.isCellCropped(row.value, column.value);
>+  } else {

is it possible to detect cropped status for menuitems? in such a case this could have 3 conditions: tree, menuitem, button (undetectable atm)


>+    node = document.tooltipNode.node;
>+    cropped = false;

Actually, just do var croppped = false; as default above.

>+  // Show URL only for links

nit: end comments with .
instead of links i'd use "URI-type nodes"

yeah, looking at the code as i suggested i find more readable as it is written now, so just leave it as it is.

Comment 36

[Gabriele Best [:Gabri]]

Attachment: Patch that doesn't work

I've written the patch but there is a very very strange problem with the sidebar: when I hover an element on the treeview, the tooltip appears but it is empty, 

I've made some test and I saw that the tooltip's labels aren't empty or hidden O__O but the tooltip is empty!!!

When I hover an element on the toolbar it works properly.

Comment 37

[Gabriele Best [:Gabri]]

Attachment: Patch that doesn't work (2)

Sorry, the previous one wasn't complete :(

Comment 38

[Gabriele Best [:Gabri]]

I'd like to know if now the structure is correct and if you find any bug.

Comment 39

[Gabriele Best [:Gabri]]

Attachment: Patch v5.0

It includes the last review.

(In reply to comment #35)
> a change like this in GlobalOverlay won't be accepted by any toolkit peer,
> that's a global toolkit file, while those are browser tooltips for a specific
> component. sorry.
> 
> actually, i'm thinking bhTooltip would be better, Places is a component
> codename, better avoiding it for prefixes in browser.

Ok

> is it possible to detect cropped status for menuitems? in such a case this
> could have 3 conditions: tree, menuitem, button (undetectable atm)

It's not possible without using computedStyle function but it makes all the function slower.

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 391913 [details] [diff] [review]
Patch v5.0

i see you're going on vacation, so have a nice stop :)
We're near, thank you for taking care of this.

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -909,37 +909,59 @@ var BookmarksEventHandler = {
>             "PlacesUIUtils.openContainerNodeInTabs(this.parentNode._resultNode, event);");
>         target._endOptOpenAllInTabs.setAttribute("onclick",
>             "checkForMiddleClick(this, event); event.stopPropagation();");
>         target._endOptOpenAllInTabs.setAttribute("label",
>             gNavigatorBundle.getString("menuOpenAllInTabs.label"));
>         target.appendChild(target._endOptOpenAllInTabs);
>     }
>   },
>+  
>+  fillInBHTooltip: function(aDocument, aEvent) {

since you still need to pass the event, if we could get the document from the event target i'd say to go back and just pass the event as you did before, sorry i missed you needed the event coordinates, so you were right. Otherwise, fine as it is.

>+    var node;
>+    var cropped = false;
>+    var tree = aDocument.getElementById("bookmarks-view") || 
>+               aDocument.getElementById("historyTree");
> 
>-  fillInBTTooltip: function(aTipElement) {
>-    if (!aTipElement.node)
>+    if (tree) {

this must be more generic to handle any tree willing to get this kind of tooltip, something like
if (aDocument.tooltipNode.localName == "treechildren") {
  var tree = aDocument.tooltipNode.parentNode;

>+      var row = {}, column = {};
>+      var tbo = tree.treeBoxObject;
>+      tbo.getCellAt(aEvent.clientX, aEvent.clientY, row, column, {});

you have to manually calculate real clientY. tooltip and popupshowing events are referred to the main document, and not to the sidebar document or to the tree.
So for the first row you get row = 6 because getCellAt expects y to be referred to the containing document, you have to subtract the clientY of the the sidebar document.
Try dumping out row.value and you'll see for first row you get row.value = 6 because clientY is about 150.

>+
>+      node = tree.view.nodeForTreeIndex(row.value);
>+      cropped = tbo.isCellCropped(row.value, column.value);
>+    } else {
>+      node = aDocument.tooltipNode.node;
>+    }

wrong code style, else on newline, eventually you can avoid braces since has only one instruction inside.

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>
>+  <!-- places tooltip -->
>+  <tooltip id="bhTooltip"/>

Avoid using places name anywhere please, let's try to be generic for future. I'd use
<!-- Bookmarks and history tooltip -->

Comment 41

[Gabriele Best [:Gabri]]

(In reply to comment #40)
> >-  fillInBTTooltip: function(aTipElement) {
> >-    if (!aTipElement.node)
> >+    if (tree) {
> 
> this must be more generic to handle any tree willing to get this kind of
> tooltip, something like
> if (aDocument.tooltipNode.localName == "treechildren") {
>   var tree = aDocument.tooltipNode.parentNode;

Ok.

> >+      var row = {}, column = {};
> >+      var tbo = tree.treeBoxObject;
> >+      tbo.getCellAt(aEvent.clientX, aEvent.clientY, row, column, {});
> 
> you have to manually calculate real clientY. tooltip and popupshowing events
> are referred to the main document, and not to the sidebar document or to the
> tree.
> So for the first row you get row = 6 because getCellAt expects y to be referred
> to the containing document, you have to subtract the clientY of the the sidebar
> document.
> Try dumping out row.value and you'll see for first row you get row.value = 6
> because clientY is about 150.

I can't reproduce that problem on my PC (windowsXP), the first row returns me 0 and clientY is about 36. What OS do you use?

> >+      node = tree.view.nodeForTreeIndex(row.value);
> >+      cropped = tbo.isCellCropped(row.value, column.value);
> >+    } else {
> >+      node = aDocument.tooltipNode.node;
> >+    }
> 
> wrong code style, else on newline, eventually you can avoid braces since has
> only one instruction inside.

Ok.

> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> >
> >+  <!-- places tooltip -->
> >+  <tooltip id="bhTooltip"/>
> 
> Avoid using places name anywhere please, let's try to be generic for future.
> I'd use
> <!-- Bookmarks and history tooltip -->

Ok. I've included all your suggestions in the patch but I need to know more details about the problems you have with getCellAt.

Comment 42

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #41)
> > >+      var row = {}, column = {};
> > >+      var tbo = tree.treeBoxObject;
> > >+      tbo.getCellAt(aEvent.clientX, aEvent.clientY, row, column, {});
> > 
> > you have to manually calculate real clientY. tooltip and popupshowing events
> > are referred to the main document, and not to the sidebar document or to the
> > tree.
> > So for the first row you get row = 6 because getCellAt expects y to be referred
> > to the containing document, you have to subtract the clientY of the the sidebar
> > document.
> > Try dumping out row.value and you'll see for first row you get row.value = 6
> > because clientY is about 150.
> 
> I can't reproduce that problem on my PC (windowsXP), the first row returns me 0
> and clientY is about 36. What OS do you use?

i've seen the problem on XP, any tooltip in the sidebar was returning the wrong row, and i clobbered just a couple days ago. that's quite strange.

Comment 43

[Gabriele Best [:Gabri]]

Attachment: Patch v5.1

Here the patch, maybe without problems :P

Comment 44

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

nope, i've recompiled the full source and i still see the issue... on m-c

Comment 45

[Gabriele Best [:Gabri]]

m-c?

Comment 46

[Gabriele Best [:Gabri]]

Can you send me your build so I can test it? I wasn't able to reproduce this issue yet :(

Comment 47

[Trejkaz]

Taking myself off the Cc as I was added when Bug 259199 was erroneously marked as a duplicate of this one, and not removed when this was resolved.

Comment 48

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #46)
> Can you send me your build so I can test it? I wasn't able to reproduce this
> issue yet :(

my build is not any different than current mozilla-central with your patch

Comment 49

[Gabriele Best [:Gabri]]

I can see this problem since 090722 build. It's a regression because on 3.5 branch this patch still works.

Comment 50

[Gabriele Best [:Gabri]]

Bug 510008 describes this issue

Comment 51

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 392079 [details] [diff] [review]
Patch v5.1

I should review this again now :)

Comment 52

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 392079 [details] [diff] [review]
Patch v5.1

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -909,37 +909,58 @@ var BookmarksEventHandler = {
>             "PlacesUIUtils.openContainerNodeInTabs(this.parentNode._resultNode, event);");
>         target._endOptOpenAllInTabs.setAttribute("onclick",
>             "checkForMiddleClick(this, event); event.stopPropagation();");
>         target._endOptOpenAllInTabs.setAttribute("label",
>             gNavigatorBundle.getString("menuOpenAllInTabs.label"));
>         target.appendChild(target._endOptOpenAllInTabs);
>     }
>   },
>+  
>+  fillInBHTooltip: function(aDocument, aEvent) {

the above newline has trailing spaces

>-    var tooltipTitle = document.getElementById("btTitleText");
>-    tooltipTitle.hidden = !title || (title == url);
>+    var tooltipTitle = aDocument.getElementById("bhtTitleText");
>+    tooltipTitle.hidden = (!title || (title == url));
>     if (!tooltipTitle.hidden)
>       tooltipTitle.textContent = title;
>+    
>+    var tooltipUrl = aDocument.getElementById("bhtUrlText");

again the above newline has trailing spaces

functionality with the event fix looks good and pretty cool.

Thanks!

Comment 53

[Gabriele Best [:Gabri]]

Attachment: Patch v5.2

Comment 54

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/029104eb567f

Comment 55

[Tyler Downer [He/Him]]

v with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090909 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20090909041701.

Comment 56

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 398657 [details] [diff] [review]
Patch v5.2

asking approval, nice polish and feedback.
extensions can easily hook into tooltips to provider further informations.

Comment 57

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: 1.9.2 roll-up patch

roll-up patch including Bug 516088

Comment 58

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

Comment on attachment 400486 [details] [diff] [review]
1.9.2 roll-up patch

a192=beltzner

Comment 59

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4951da8a2c5a

Comment 60

[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