Closed Bug 244371 Opened 18 years ago Closed 12 years ago

Show a tooltip in Bookmarks and History sidebars.

Categories

(Firefox :: Bookmarks & History, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.6b1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: steve, Assigned: Gabri)

References

Details

(Keywords: polish)

Attachments

(3 files, 11 obsolete files)

697.78 KB, image/png
beltzner
: ui-review+
Details
11.00 KB, patch
Details | Diff | Splinter Review
6.80 KB, patch
beltzner
: approval1.9.2+
Details | Diff | Splinter Review
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.
seems OK, definitely not something we'll look at doing before 1.0 though
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → After Firefox 1.0
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.
*** Bug 250776 has been marked as a duplicate of this bug. ***
(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
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
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
(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.
(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.. 
Assignee: bross2 → nobody
QA Contact: mozilla → history
Should this be moved to Places?
Component: History → Bookmarks
QA Contact: history → bookmarks
Version: unspecified → Trunk
Component: Bookmarks → Places
QA Contact: bookmarks → places
Duplicate of this bug: 438334
I'd like to work on it.
Nobody can fix this bug without tree tooltip support (bug 421167)
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.
Assignee: nobody → gabri.best
Status: NEW → ASSIGNED
Target Milestone: Future → ---
Attached patch Patch v1.0 (obsolete) — Splinter Review
This patch add a tooltip to bookmarks and history sidebar.
The tooltip contains TITLE and URL of the item.
Attachment #364377 - Flags: ui-review?
Attachment #364377 - Flags: review?(gavin.sharp)
Attachment #364377 - Flags: approval1.9.1?
Attachment #364377 - Flags: ui-review? → ui-review?(faaborg)
Summary: Show URL as tool tip text in Firefox Bookmarks and History sidebars. → Show a tooltip in Bookmarks and History sidebars.
That patch is very low impact and a minor fix.
It doesn't displays a tooltip over the folders but only over the links.
Attached patch Patch v2.0 (obsolete) — Splinter Review
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.
Attachment #364377 - Attachment is obsolete: true
Attachment #364693 - Flags: ui-review?(faaborg)
Attachment #364693 - Flags: review?(gavin.sharp)
Attachment #364377 - Flags: ui-review?(faaborg)
Attachment #364377 - Flags: review?(gavin.sharp)
Attachment #364377 - Flags: approval1.9.1?
Keywords: useless-UI
Duplicate of this bug: 259199
Attached image Screen after patch
This is a screen after patch 2.0 is applied
Attachment #366594 - Flags: ui-review?
Attachment #366594 - Flags: ui-review? → ui-review?(beltzner)
Attachment #364693 - Flags: ui-review?(faaborg)
Attachment #366594 - Flags: ui-review?(beltzner) → ui-review+
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.
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 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?
Attached patch Patch v2.1 (obsolete) — Splinter Review
Ok, code changed.
Attachment #364693 - Attachment is obsolete: true
Attachment #368779 - Flags: review?(gavin.sharp)
Attachment #364693 - Flags: review?(gavin.sharp)
Attached patch Patch v2.1.1 (obsolete) — Splinter Review
Ok, it is the correct one.
Attachment #368779 - Attachment is obsolete: true
Attachment #368780 - Flags: review?(gavin.sharp)
Attachment #368779 - Flags: review?(gavin.sharp)
Attachment #368780 - Attachment is patch: true
Attachment #368780 - Attachment mime type: application/octet-stream → text/plain
Attachment #368780 - Flags: review?(gavin.sharp) → review?(mak77)
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.
Keywords: useless-UIpolish
Whiteboard: [needs review mak]
Yes it does.
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).
Attachment #368780 - Flags: review?(mak77) → review-
(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 :)
Attached patch Patch v2.2 (obsolete) — Splinter Review
This patch has the same structure of Patch 2.1.1
Attachment #368780 - Attachment is obsolete: true
Attachment #391811 - Flags: review?(mak77)
Attached patch Patch v3.0 (obsolete) — Splinter Review
Attachment #391813 - Flags: review?(mak77)
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.
(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.
I've just finished the patch but I need to speak with you via IRC :)
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.
Attachment #391864 - Flags: review?(mak77)
>+    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?
Attachment #391811 - Attachment is obsolete: true
Attachment #391811 - Flags: review?(mak77)
Attachment #391813 - Attachment is obsolete: true
Attachment #391813 - Flags: review?(mak77)
Attachment #391864 - Flags: review?(mak77) → review-
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.
Attached patch Patch that doesn't work (obsolete) — Splinter Review
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.
Attachment #391864 - Attachment is obsolete: true
Attached patch Patch that doesn't work (2) (obsolete) — Splinter Review
Sorry, the previous one wasn't complete :(
Attachment #391883 - Attachment is obsolete: true
I'd like to know if now the structure is correct and if you find any bug.
Attached patch Patch v5.0 (obsolete) — Splinter Review
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.
Attachment #391884 - Attachment is obsolete: true
Attachment #391913 - Flags: review?(mak77)
Attachment #391913 - Flags: review?(mak77) → review-
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 -->
(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.
(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.
Attached patch Patch v5.1 (obsolete) — Splinter Review
Here the patch, maybe without problems :P
Attachment #391913 - Attachment is obsolete: true
Attachment #392079 - Flags: review?(mak77)
nope, i've recompiled the full source and i still see the issue... on m-c
m-c?
Attachment #392079 - Flags: review?(mak77)
Can you send me your build so I can test it? I wasn't able to reproduce this issue yet :(
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.
(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
I can see this problem since 090722 build. It's a regression because on 3.5 branch this patch still works.
Bug 510008 describes this issue
Depends on: widget-removal, 497434
Depends on: 510008
No longer depends on: widget-removal
No longer depends on: 497434
Comment on attachment 392079 [details] [diff] [review]
Patch v5.1

I should review this again now :)
Attachment #392079 - Flags: review?(mak77)
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!
Attachment #392079 - Flags: review?(mak77) → review+
Whiteboard: [needs review mak]
Attached patch Patch v5.2Splinter Review
http://hg.mozilla.org/mozilla-central/rev/029104eb567f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
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.
Status: RESOLVED → VERIFIED
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.
Attachment #398657 - Flags: approval1.9.2?
Depends on: 516088
roll-up patch including Bug 516088
Attachment #400486 - Flags: approval1.9.2?
Attachment #398657 - Flags: approval1.9.2?
Attachment #392079 - Attachment is obsolete: true
Comment on attachment 400486 [details] [diff] [review]
1.9.2 roll-up patch

a192=beltzner
Attachment #400486 - Flags: approval1.9.2? → approval1.9.2+
Depends on: tooltips
Target Milestone: Firefox 3.7a1 → Firefox 3.6b1
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Blocks: tooltips
No longer depends on: tooltips
You need to log in before you can comment on or make changes to this bug.