397263 - No way to set an icon for javascript bookmarklets on toolbar and menupopups

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ed Hume]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007092105 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007092105 Minefield/3.0a8pre

In fx2, javascript=based bookmarks such as bookmarklets could be distinguished from standard bookmarks with a selector: .bookmark-item[statustext^=javascript], allowing for a distinct icon.

Although this method still works on the Bookmarks Toolbar overflow (under the chevron) and in menus originating from the Bookmarks dropdown menu, the javascript bookmark shows up with a default icon on the Bookmarks Toolbar.

Further, on a dropdown menu coming from a bookmarks folder item that sits on the Bookmarks Toolbar, the object does not appear at all, even though it does exist and can be seen - complete with its distinct icon - in menus originating from the Bookmarks dropdown menu from the Menu Bar.

In the fx3 browser.xul, the whole block relating to this has been removed, but the missing statusbar element does not seem to have been replaced with corresponding code elsewhere.

If the issue of missing javascript bookmarklets deserves a separate bug, let me know.


Reproducible: Always

Steps to Reproduce:
1. Create a javascript bookmark (bookmarklet), or
   Import a bookmarks.html file that contains bookamrklets
2. Install a theme that supports distinct icons for javascript bookmarks
   (e.g.--SphereGnome or FormalGnome)
Actual Results:  
No distinct bookmarklet icon when the bookmarklet is on the Bookmarks Toolbar.

Bookmarklets do not show up at all in menus that come from bookmarks folders on the Bookmarks Toolbar.

Expected Results:  
Distinct icons for the bookmarklets. 
Bookmarklets should show up on all menus.

I have an developmental version of SphereGnome which works with fx 3.0a9 as of the 2007-09-22 build if someone needs a theme to see the problem with.

Comment 1

[Ed Hume]

Request this be marked as a regression.

Comment 2

[Marco Bonardo [:mak]]

> Further, on a dropdown menu coming from a bookmarks folder item that sits on
> the Bookmarks Toolbar, the object does not appear at all, even though it does
> exist and can be seen - complete with its distinct icon - in menus originating
> from the Bookmarks dropdown menu from the Menu Bar.

Ed, apart the status bar text that is an actual bug (quite easy fix, toolbar buttons are missing statustext attribute), is this problem (bookmarklet does not appear at all in toolbar folders menupopup) still valid with the current trunk?

Comment 3

[Marco Bonardo [:mak]]

Attachment: add statustext to toolbar URI buttons

Comment 4

[Ed Hume]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030904 Minefield/3.0b5pre - Build ID: 2008030904

The bookmarklet appears and works correctly (normal, hover and active) in dropdown bookmarks menus, including the overflow under the chevron and the dropdown menu originating from the Bookmarks main menu.

The bookmarklet continues show only the default image from folder-item.png.

It's great. One down, one to go.

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> The bookmarklet continues show only the default image from folder-item.png.

my patch should solve that, however i think that Michael can merge that into bug 409748

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 308396 [details] [diff] [review]
add statustext to toolbar URI buttons

ok statustext has been lost in bug 409748, so this will need a specialized new class

Comment 7

[Pardal Freudenthal (:ShareBird)]

I guess the simplest way to fix this could be at treeView.js at: 
http://mxr.mozilla.org/mozilla/source/browser/components/places/content/treeView.js#873

asking if columnType=url contains a string like "javascript:" and push a property like "bookmarklet" to the properties array. After this it should be easy to point to bookmarklets at the tree for any proposes...

Comment 8

[Pardal Freudenthal (:ShareBird)]

Attachment: patch

proposed patch

With this it will be possible to assign custom icons for bookmarklets using this code at browser.css:

.bookmark-item[bookmarklet]{
  list-style-image: url(PathToYourIcon);
}

and this at places.css

treechildren::-moz-tree-image(title, bookmarklet) {
  list-style-image: url(PathToYourIcon);
}

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 364913 [details] [diff] [review]
patch

>diff --git a/source/browser/components/places/content/utils.js b/source/browser/components/places/content/utils.js
>--- a/source/browser/components/places/content/utils.js
>+++ b/source/browser/components/places/content/utils.js
>@@ -995,14 +995,18 @@
>       var iconURISpec = "";
>       if (iconURI)
>         iconURISpec = iconURI.spec;
> 
>       if (PlacesUtils.uriTypes.indexOf(type) != -1) {
>         element = document.createElement("menuitem");
>         element.className = "menuitem-iconic bookmark-item";
>+
>+        if (aNode.uri.indexOf("javascript:") != -1)
>+          element.setAttribute("bookmarklet", "true");
>+

please use if (aNode.uri.lastIndexOf("javascript:", 0) == 0)
and remove the last empty line


>       }
>       else if (PlacesUtils.containerTypes.indexOf(type) != -1) {
>         element = document.createElement("menu");
>         element.setAttribute("container", "true");
> 
>         if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY) {
>           element.setAttribute("query", "true");
>diff --git a/source/browser/components/places/content/toolbar.xml b/source/browser/components/places/content/toolbar.xml
>--- a/source/browser/components/places/content/toolbar.xml
>+++ b/source/browser/components/places/content/toolbar.xml
>@@ -634,14 +634,18 @@
>           if (PlacesUtils.nodeIsLivemarkContainer(aNode)) {
>             if (!element.hasAttribute("livemark"))
>               element.setAttribute("livemark", "true");
>             // If this is a livemark container check if the status menuitem has
>             // to be added or removed.
>             PlacesUIUtils.ensureLivemarkStatusMenuItem(element.firstChild);
>           }
>+          
>+          if (aNode.uri.indexOf("javascript:") != -1)
>+            element.setAttribute("bookmarklet", "true");
>+

as above, but also add an else, if the uri has changed and is no more a bookmarklet, we should remove the attribute.
put this into an else if nodeIsURI, we should not check for containers.

you should add the attribute also in InertNewItem, since toolbar buttons should have it

>diff --git a/source/browser/components/places/content/treeView.js b/source/browser/components/places/content/treeView.js
>--- a/source/browser/components/places/content/treeView.js
>+++ b/source/browser/components/places/content/treeView.js
>@@ -919,14 +919,18 @@
>         }
>       }
>       else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR)
>         properties.push(this._getAtomFor("separator"));
>       else if (itemId != -1) { // bookmark nodes
>         if (PlacesUtils.nodeIsLivemarkContainer(node.parent))
>           properties.push(this._getAtomFor("livemarkItem"));
>+        else if (PlacesUtils.nodeIsURI(node)) {
>+          if (node.uri.indexOf("javascript:") != -1)
>+            properties.push(this._getAtomFor("bookmarklet"));
>+        }

as above

Comment 10

[Pardal Freudenthal (:ShareBird)]

Attachment: patch v1.1

Thank you! I did a new try following your points.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 365733 [details] [diff] [review]
patch v1.1

>diff --git a/source/browser/components/places/content/toolbar.xml b/source/browser/components/places/content/toolbar.xml
>--- a/source/browser/components/places/content/toolbar.xml
>+++ b/source/browser/components/places/content/toolbar.xml
>@@ -215,14 +215,20 @@
>               popup._resultNode = asContainer(aChild);
> #ifndef XP_MACOSX
>               popup.setAttribute("context", "placesContext");
> #endif
>               this._containerNodesMap.push({ resultNode: aChild,
>                                              domNode: popup });
>             }
>+            else if (PlacesUtils.nodeIsURI(aChild)) {
>+              if (aChild.uri.lastIndexOf("javascript:", 0)== 0)

missing space before == 0

Looks good apart that.

I don't think this needs any kind of automatic test, but please make a some manual check that everything works as expected.

Please file a followup bug to provide a default bookmarklet icon for Firefox and hook it up using this attribute.

Comment 12

[Marco Bonardo [:mak]]

per IRC, we miss menu.xml itemChanged

Comment 13

[Pardal Freudenthal (:ShareBird)]

Attachment: patch v1.2

new patch touching also menu.xml

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 365802 [details] [diff] [review]
patch v1.2

Thanks for looking at this, if you don't have commit access to the tree please add the checkin-needed keyword so i (or someone other) will take care of pushing for you.

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/3155b8134959

Comment 16

[Marco Bonardo [:mak]]

Attachment: as pushed

Comment 17

[Marco Bonardo [:mak]]

gavin suggested a more generic way to handle this: bug 482235
i won't backout this since it's not going to branch and is marking points where to hook. but the css syntax could change there.

Comment 18

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #17)
> gavin suggested a more generic way to handle this: bug 482235
> i won't backout this since it's not going to branch and is marking points where
> to hook. but the css syntax could change there.

Yes. That's a better aopproach. With this we can also assign special icons for other types of bookmarks.

Comment 19

[Marco Bonardo [:mak]]

basically this has been replaced on 1.9.1 by bug 482235, that is merely replacing all calls this patch added. so adding fixed 1.9.1 to this since it is like this this has landed and then been replaced by bug 482235 (what did happen on m-c).

Comment 20

[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