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

RESOLVED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Ed Hume, Assigned: ShareBird)

Tracking

({fixed1.9.1, regression})

Trunk
Firefox 3.6a1
fixed1.9.1, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

9 years ago
Request this be marked as a regression.
> 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?
Created attachment 308396 [details] [diff] [review]
add statustext to toolbar URI buttons

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
(Reporter)

Comment 4

9 years ago
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.
(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 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
Attachment #308396 - Attachment is obsolete: true

Updated

9 years ago
Summary: No way to distinguish javascript bookmarklets from normal bookmarks on Bookmarks Toolbar → No way to set an icon for javascript bookmarklets on toolbar and menupopups
(Assignee)

Comment 7

9 years ago
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...
(Assignee)

Comment 8

8 years ago
Created attachment 364913 [details] [diff] [review]
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);
}
Attachment #364913 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Assignee: nobody → pardal
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
Attachment #364913 - Flags: review?(mak77) → review-
(Assignee)

Comment 10

8 years ago
Created attachment 365733 [details] [diff] [review]
patch v1.1

Thank you! I did a new try following your points.
Attachment #364913 - Attachment is obsolete: true
Attachment #365733 - Flags: review?(mak77)
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.
Attachment #365733 - Flags: review?(mak77) → review+
per IRC, we miss menu.xml itemChanged
(Assignee)

Comment 13

8 years ago
Created attachment 365802 [details] [diff] [review]
patch v1.2

new patch touching also menu.xml
Attachment #365733 - Attachment is obsolete: true
Attachment #365802 - Flags: review?(mak77)
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.
Attachment #365802 - Flags: review?(mak77) → review+

Updated

8 years ago
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Blocks: 482166
http://hg.mozilla.org/mozilla-central/rev/3155b8134959
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Created attachment 366286 [details] [diff] [review]
as pushed
Attachment #365802 - Attachment is obsolete: true

Updated

8 years ago
Depends on: 482235
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.
(Assignee)

Comment 18

8 years ago
(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.
(Assignee)

Updated

8 years ago
No longer blocks: 482166
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).
Keywords: fixed1.9.1
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
You need to log in before you can comment on or make changes to this bug.