476838 - Missing tooltip and statusbar text on "Open <siteURI>" in live bookmarks

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Marco Bonardo [:mak]]

Actually it's impossible to know where we are going when clicking this option, we should add them.

Looks good as a polish bug for 3.1.

Comment 1

[Jason Bullen]

I'm interested in this as a first bug.

Can any one explain how I can change the status and assign it to my self? I don't think I have permissions to do so.

Thanks!

Comment 2

[Michael Kohler [:mkohler]]

Assigning to Jason as demanded in comment 1.

Comment 3

[Romain BEZUT]

Attachment: proposed patch

Since there is no activity for a few months and this bug is flagged "wanted-FF3.6", I allowed myself to take it.

This patch simply adds a member "node" on the menu-item, so that when "fillInBHTooltip" is called, information are displayed like on any bookmark item (tooltip and status bar).

I though of removing completely "siteURI" attribute, but it wasn't clear if it was still used elsewhere. In doubt and since it causes no harm, I've preferred not to remove it.

This piece of code could look cleaner, target._endOptOpenSiteURI is very redundant here, but since it was like that before, I hasn't changed it.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 404514 [details] [diff] [review]
proposed patch

adding a node is not possible, we use the presence of a node property to distinguish static menu contents from Places contents, so if you add a node where it should not be you make assumptions on other code.

Comment 5

[Romain BEZUT]

Attachment: proposed patch V2

Since we can't use the node property here, I just added a tooltiptext attribute, and two listeners to display the URL in the status bar.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 405726 [details] [diff] [review]
proposed patch V2

>diff -r 8862815409ab browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js	Sat Oct 10 11:01:12 2009 -0700
>+++ b/browser/base/content/browser-places.js	Sun Oct 11 03:28:28 2009 +0200
>@@ -885,16 +885,25 @@ var BookmarksEventHandler = {
>       target.appendChild(target._endOptSeparator);
>     }
> 
>     if (siteURIString && !target._endOptOpenSiteURI) {
>       // Add "Open (Feed Name)" menuitem if it's a livemark with a siteURI
>       target._endOptOpenSiteURI = document.createElement("menuitem");
>       target._endOptOpenSiteURI.className = "openlivemarksite-menuitem";
>       target._endOptOpenSiteURI.setAttribute("siteURI", siteURIString);
>+      // Display information for the link
>+      target._endOptOpenSiteURI.setAttribute("tooltiptext", siteURIString);
>+      target._endOptOpenSiteURI.addEventListener("DOMMenuItemActive", function() {
>+        window.XULBrowserWindow
>+              .setOverLink(this.getAttribute("siteURI"), null);
>+      }, false);
>+      target._endOptOpenSiteURI.addEventListener("DOMMenuItemInactive", function() {
>+        window.XULBrowserWindow.setOverLink("", null);
>+      }, false);

Sorry for late.
The problems about this implementation is that:
- the menu should already have DOMMEnuItemInactive from menu.xml (so the added one looks useless at first glance)
- the generated tooltip would not be coherent with other Places' Tooltip

what about changing fillInBHTooltip in browser-places.js and the DOMMenuItemActive handler in menu.xml to detect special static uri nodes and skip Places node check for such elements, using label as title and the uri attribute for uri?
We could replace "siteURI" attribute name with a more generic "targetURI" attribute and use getAttribute("targetURI") to detect static uri nodes.

Comment 7

[Marco Bonardo [:mak]]

cc-ing Gabriele that has worked on other tooltips related bugs and could have some interesting idea to suggest.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 405726 [details] [diff] [review]
proposed patch V2

btw, would need new patch either case, clearing request

Comment 9

[Gabriele Best [:Gabri]]

One month ago I tried to write a patch for that but I saw some "strange behaviors". I'll take a look to that again but I don't think it will be included in the next release.

Comment 10

[Gabriele Best [:Gabri]]

Attachment: patch, v.0.1

Comment 11

[Gabriele Best [:Gabri]]

Attachment: patch, v.0.2: another approach

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 411234 [details] [diff] [review]
patch, v.0.2: another approach

i already R- this approach before, we base on the fact node is present for various Places views code paths, creating fake nodes can generate obscure bugs, so i prefer not doing that.

Comment 13

[Gabriele Best [:Gabri]]

Attachment: patch not clean, sorry

Comment 14

[Gabriele Best [:Gabri]]

Attachment: patch, v0.3 (v0.1 based + some comments)

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 411507 [details] [diff] [review]
patch, v0.3 (v0.1 based + some comments)

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js

> 
>   fillInBHTooltip: function(aDocument, aEvent) {
>     var node;
>     var cropped = false;
>+    var targetURI;
> 
>     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, {});
>       if (row.value == -1)
>         return false;
>       node = tree.view.nodeForTreeIndex(row.value);
>       cropped = tbo.isCellCropped(row.value, column.value);
>     }
>-    else
>-      node = aDocument.tooltipNode.node;
>+    else {
>+      node = aDocument.tooltipNode;
>+      targetURI = node.getAttribute("targetURI");
>+      if (!targetURI)
>+        node = node.node;

ugh, node = node.node is unreadable and error-prone
also you should not set node if it's not a Places node AND targetURI is not set, otherwise the next if(!node) check is useless
also i prefer not calling node.getAttribute("targetURI") for valid Places nodes (for perf reasons)

so, to avoid confusion, node should always be a Places node, while we should retain a tooltipNode var.
I don't want to have the same node var contain 2 different kind of data.

let tooltipNode = aDocument.tooltipNode;

if (tooltipNode.node)
  node = tooltipNode.node;
else {
  // This is a static non-Places node.
  targetURI = tooltipNode.getAttribute("targetURI");
}

>     if (!node)
>       return false;

should become if (!targetURI && !node)

> 
>-    var title = node.title;
>-    var url;
>+    // Show node.label as tooltip's title for non places' nodes.
>+    var title = targetURI ? node.label : node.title;

s/non places'/non-Places/
var title = node ? node.title : tooltipNode.label;

>-    // Show URL only for URI-type nodes.
>+    // Show URL only for nodes with a defined targetURI or URI-type.

// Show URL only for Places URI-nodes or nodes with a targetURI attribute.

>+    var url = targetURI;
>     if (PlacesUtils.nodeIsURI(node))
>       url = node.uri;

hm, this should probably be

var url;
if (targetURI || PlacesUtils.nodeIsURI(node)) {
  url = targetURI || node.uri;
}

notice the order here is important since if targetURI is true node is null

please double check the code i posted and the remaining code, i just wanted to show a way rather than writing perfect code

>diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml
>--- a/browser/components/places/content/menu.xml
>+++ b/browser/components/places/content/menu.xml
>@@ -505,18 +505,21 @@
>             return;
> 
>           node = node.parentNode;
>         }
> #endif
> 
>         if (window.XULBrowserWindow) {
>           var nodeItem = event.target.node;
>-          if (nodeItem && PlacesUtils.nodeIsURI(nodeItem))
>-            window.XULBrowserWindow.setOverLink(nodeItem.uri, null);
>+          var targetURI = node.getAttribute("targetURI");
>+
>+          var linkURI = nodeItem ? nodeItem.uri : targetURI;
>+          if ((nodeItem && PlacesUtils.nodeIsURI(nodeItem)) || targetURI)
>+            window.XULBrowserWindow.setOverLink(linkURI, null);

ditto: let's avoid to read the attribute for valid Places nodes

Comment 16

[Gabriele Best [:Gabri]]

Attachment: patch, v0.4

Comment 17

[Gabriele Best [:Gabri]]

Attachment: patch, v0.4.1 trailing spaces removed

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 411709 [details] [diff] [review]
patch, v0.4.1 trailing spaces removed

>diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc

>@@ -917,39 +934,50 @@ var BookmarksEventHandler = {
>             gNavigatorBundle.getString("menuOpenAllInTabs.label"));
>         target.appendChild(target._endOptOpenAllInTabs);
>     }
>   },
> 
>   fillInBHTooltip: function(aDocument, aEvent) {

>-    else
>-      node = aDocument.tooltipNode.node;
>+    else {
>+      // Check whether the tooltipNode is a Places-node.

s/Places-node/Places node/

>+    // Show node.label as tooltip's title for non Places-nodes.

s/non Places-nodes/non-Places nodes/

looks good otherwise, i assume you tested this manually.

Comment 19

[Gabriele Best [:Gabri]]

Attachment: patch, v0.4.2

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/f0066bea7103