Closed Bug 476838 Opened 11 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3.7a1

People

(Reporter: mak, Assigned: Gabri)

References

Details

(Keywords: polish)

Attachments

(1 file, 8 obsolete files)

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.
Whiteboard: [good first bug]
Priority: -- → P4
Flags: wanted-firefox3.1+
Target Milestone: Firefox 3.1 → ---
Flags: wanted-firefox3.5+ → wanted-firefox3.6+
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!
Assigning to Jason as demanded in comment 1.
Assignee: nobody → raknorviper
Status: NEW → ASSIGNED
Attached patch proposed patch (obsolete) — Splinter Review
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.
Assignee: raknorviper → romain
Attachment #404514 - Flags: review?(gavin.sharp)
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.
Attachment #404514 - Flags: review?(gavin.sharp) → review-
Attached patch proposed patch V2 (obsolete) — Splinter Review
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.
Attachment #404514 - Attachment is obsolete: true
Attachment #405726 - Flags: review?(mak77)
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.
cc-ing Gabriele that has worked on other tooltips related bugs and could have some interesting idea to suggest.
Comment on attachment 405726 [details] [diff] [review]
proposed patch V2

btw, would need new patch either case, clearing request
Attachment #405726 - Flags: review?(mak77)
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.
Attached patch patch, v.0.1 (obsolete) — Splinter Review
Assignee: romain → gabri.best
Attachment #405726 - Attachment is obsolete: true
Attachment #411187 - Flags: review?(mak77)
Attached patch patch, v.0.2: another approach (obsolete) — Splinter Review
Attachment #411234 - Flags: review?(mak77)
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.
Attachment #411234 - Flags: review?(mak77) → review-
Attached patch patch not clean, sorry (obsolete) — Splinter Review
Attachment #411187 - Attachment is obsolete: true
Attachment #411234 - Attachment is obsolete: true
Attachment #411506 - Flags: review?(mak77)
Attachment #411187 - Flags: review?(mak77)
Attachment #411506 - Attachment description: patch, v0.3 (v0.1 based + some comments) → patch not clean, sorry
Attachment #411506 - Flags: review?(mak77)
Attachment #411506 - Attachment is obsolete: true
Attachment #411507 - Flags: review?(mak77)
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
Attachment #411507 - Flags: review?(mak77) → review-
Attached patch patch, v0.4 (obsolete) — Splinter Review
Attachment #411507 - Attachment is obsolete: true
Attachment #411707 - Flags: review?(mak77)
Attachment #411707 - Attachment is obsolete: true
Attachment #411709 - Flags: review?(mak77)
Attachment #411707 - Flags: review?(mak77)
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.
Attachment #411709 - Flags: review?(mak77) → review+
Attached patch patch, v0.4.2Splinter Review
Attachment #411709 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/f0066bea7103
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → Firefox 3.7a1
Depends on: tooltips
Blocks: tooltips
No longer depends on: tooltips
You need to log in before you can comment on or make changes to this bug.