Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 658220 - Invoking bookmarklets by keyword no longer works (broken by Bug 656433)
: Invoking bookmarklets by keyword no longer works (broken by Bug 656433)
nominated by bz in comment 1
: regression
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- major with 4 votes (vote)
: Firefox 7
Assigned To: :Gavin Sharp [email:]
: Marco Bonardo [::mak]
Depends on:
Blocks: 656433
  Show dependency treegraph
Reported: 2011-05-19 03:04 PDT by al_9x
Modified: 2012-09-14 16:29 PDT (History)
21 users (show) in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (4.06 KB, patch)
2011-05-20 09:15 PDT, :Gavin Sharp [email:]
no flags Details | Diff | Splinter Review
patch (5.33 KB, patch)
2011-05-20 09:15 PDT, :Gavin Sharp [email:]
dao+bmo: review-
Details | Diff | Splinter Review
patch (12.51 KB, patch)
2011-06-01 15:30 PDT, :Gavin Sharp [email:]
dao+bmo: review+
Details | Diff | Splinter Review
comments addressed. (12.55 KB, patch)
2011-06-02 09:52 PDT, :Gavin Sharp [email:]
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description al_9x 2011-05-19 03:04:14 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20110420 Firefox/3.6.17
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110518 Firefox/6.0a1

keyword bookmarklets (invoked by entering a keyword and an argument, followed by enter, on the urlbar) don't run, after Bug 656433.  They do run if go is clicked instead of enter.  Bug 656433 was not intended to disable bookmarklets, so this functionality should be restored.

Reproducible: Always

Steps to Reproduce:
1. create a keyword bookmarklet:

location: "javascript:open('','_self');open('');"
keyword: kw

2. type "kw test<enter>" on the urlbar
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 06:12:53 PDT
Yeah, this is bad; it breaks my pushlog range bookmarklet, so I'm having to stay off of nightlies with the fix for bug 656433 for now.  :(
Comment 2 :Gavin Sharp [email:] 2011-05-19 11:53:23 PDT
Bookmark keywords entered into the location bar aren't really what I would call "bookmarklets". The fact that things work if you click "Go" is actually a bug in the patch for bug 656433.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 12:05:14 PDT
I'd be fine to run the bookmark keywords against a new about:blank document (so they're different from bookmarklets in that sense).  At least for my bookmark keywords.
Comment 4 Daniel Veditz [:dveditz] 2011-05-19 13:26:43 PDT
(In reply to comment #3)
> I'd be fine to run the bookmark keywords against a new about:blank document

I wouldn't. I use Jesse's various dev/bugzilla bookmarklets all the time and they make no sense unless run in the page context. Finding them in the bookmark toolbar is a PITA for things I run all the time so they're all keyworded.
Comment 5 Jesse Ruderman 2011-05-19 13:35:10 PDT
This isn't just about bookmarklets that use %s and can only be used with keywords. I have my Instapaper "Read Later" bookmarklet assigned to the keyword "r" and I can no longer use it with the keyword.
Comment 6 al_9x 2011-05-19 13:43:18 PDT
(In reply to comment #2)
> Bookmark keywords entered into the location bar aren't really what I would
> call "bookmarklets". The fact that things work if you click "Go" is actually
> a bug in the patch for bug 656433.

A bookmarklet is a javascript: bookmark.  A keyword is just a means to invoke it, as is a click on a bookmarklet without a keyword.  Bookmarklets are intended to still work, why are you attempting to redefine bookmarklets invoked by a keyword as not bookmarklets?  So you can leave this useful feature broken?
Comment 7 Jesse Ruderman 2011-05-19 14:04:29 PDT
> The fact that things work if you click "Go" is actually a bug in the patch for 
> bug 656433.

Split into bug 658383.
Comment 8 :Gavin Sharp [email:] 2011-05-19 15:30:35 PDT
I'm not trying to defend "breaking" anyone, I'm just pointing out that the primary means of using bookmarklets (not accessed via a keyword) still works fine. I assigned this bug to me for a reason; I'll fix it.
Comment 9 :Gavin Sharp [email:] 2011-05-20 09:15:15 PDT
Created attachment 533996 [details] [diff] [review]

This is one approach I tried. I don't like it because it's fragile - the fact that the URL changed is not necessarily an indication that it is safe (even though it currently probably is).
Comment 10 :Gavin Sharp [email:] 2011-05-20 09:15:46 PDT
Created attachment 533997 [details] [diff] [review]

I think this is a more robust patch.
Comment 11 Dão Gottwald [:dao] 2011-05-29 02:09:36 PDT
Comment on attachment 533997 [details] [diff] [review]

>-function getShortcutOrURI(aURL, aPostDataRef) {
>+function getShortcutOrURI(aURL, aPostDataRef, aURLObj) {
>   var shortcutURL = null;
>   var keyword = aURL;
>   var param = "";
>   var offset = aURL.indexOf(" ");
>   if (offset > 0) {
>     keyword = aURL.substr(0, offset);
>     param = aURL.substr(offset + 1);
>@@ -2305,16 +2305,19 @@ function getShortcutOrURI(aURL, aPostDat
>   else if (param) {
>     // This keyword doesn't take a parameter, but one was provided. Just return
>     // the original URL.
>     aPostDataRef.value = null;
>     return aURL;
>   }
>+  // This URL came from a bookmark, so it's safe to let it inherit the current
>+  // document's principal.
>+  aURLObj.isSafeToInherit = true;
>   return shortcutURL;
> }

This looks like it should take into account aURLObj being null. (I don't like "aURLObj" as a name. Reminds me of documentURIObject.)

>       <method name="_canonizeURL">
>         <parameter name="aTriggeringEvent"/>
>+        <parameter name="aURLObj"/>
>         <body><![CDATA[
>           var url = this.value;
>           if (!url)
>             return ["", null];
>           // Only add the suffix when the URL bar value isn't already "URL-like",
>           // and only if we get a keyboard event, to match user expectations.
>           if (!/^\s*(www|https?)\b|\/\s*$/i.test(url) &&
>@@ -397,17 +403,17 @@
>               } else
>                 url = url + (existingSuffix == -1 ? suffix : "/");
>               url = "http://www." + url;
>             }
>           }
>           var postData = {};
>-          url = getShortcutOrURI(url, postData);
>+          url = getShortcutOrURI(url, postData, aURLObj);
>           return [url, postData.value];

How about:

var postData = {};
var urlObj = {};
url = getShortcutOrURI(url, postData, urlObj);
return [url, postDate.value, urlObj.isSafeToInherit];
Comment 12 :Gavin Sharp [email:] 2011-06-01 15:30:53 PDT
Created attachment 536758 [details] [diff] [review]

I added some tests:
- in browser_getshortcutoruri, made it check that isSafeToInherit is set correctly. I changed the expected results for search keywords. We could mark such searches as "safe" as well, but there doesn't seem to be much of a valid use case for that, and it's easier/more common to add a search engine without seeing its URL than to do the same with a bookmark+keyword, I think.
- added a small test specifically for keyword bookmarklets
Comment 13 Dão Gottwald [:dao] 2011-06-02 01:29:59 PDT
Comment on attachment 536758 [details] [diff] [review]

safeToInherit / isSafeToInherit seem inconveniently imprecise. I'd probably add "Principal" in all instances. safeTo / isSafeTo could probably be just "may".

>       <method name="handleCommand">
>         <parameter name="aTriggeringEvent"/>
>         <body><![CDATA[
>           if (aTriggeringEvent instanceof MouseEvent && aTriggeringEvent.button == 2)
>             return; // Do nothing for right clicks
>-          var url = this.value;
>+          let url = this.value;
>+          let isSafeToInherit = false;
>           var postData = null;
>           var action = this._parseActionUrl(url);

Should probably stick with var here for consistency. The blank line before postData seems to be added arbitrarily.
Comment 14 :Gavin Sharp [email:] 2011-06-02 09:52:17 PDT
Created attachment 536907 [details] [diff] [review]
comments addressed.
Comment 15 :Gavin Sharp [email:] 2011-06-03 10:27:12 PDT
Comment 16 :Gavin Sharp [email:] 2011-06-10 12:25:44 PDT
Comment 17 George Carstoiu 2011-06-28 08:57:26 PDT
 Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110627 Firefox/7.0a1

Verified this on the latest trunk using Ubuntu 11.04, Mac OS X 10.6, Win 7, WinXP. Issue no longer reproducible -> setting status to Verified Fixed.
Comment 18 Bitt Faulk 2011-09-05 07:37:25 PDT
Am I wrong in thinking that this should be fixed under the current 7.0 beta?  Because it's still not working for me.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2011-09-05 10:37:34 PDT
This should work in a 7.0 beta build, yes...  What bookmarklet are you using that doesn't work?
Comment 20 Bitt Faulk 2011-09-05 15:54:00 PDT
I have a bunch of this form:


But I also created the exact one in the initial report.

I can't find any that work.
Comment 21 Bitt Faulk 2011-09-05 15:56:01 PDT
I should have pointed out that this is the error that I'm getting in the error console:

Error: uncaught exception: ReferenceError: location is not defined
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2011-09-05 18:30:10 PDT
I just tried the bookmarklet from comment 20 in a 7.0beta build with a clean profile by adding a bookmark with a bookmark keyword and then using that keyword, both with and without an argument.  Both work for me...

Do you see the problem in safe mode?
Comment 23 Bitt Faulk 2011-09-06 16:21:11 PDT
Whoops.  My fault.  The problem seems to be with the Clumsy Fingers extension.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-09-06 19:52:12 PDT
Bitt, thank you for following up on that!

Could you report the issue to the extension's author?
Comment 25 Richard Davies 2012-01-04 13:38:34 PST
The Delicious extension (as of version 2.3.1) also breaks this functionality.
Comment 26 Jon B 2012-07-11 13:20:50 PDT
By what mechanism are extensions able to break this functionality?  What would the extension author do to fix it?  How do you determine which extension it is?  Disable them one by one?
Comment 27 Richard Davies 2012-07-11 14:23:26 PDT
Yes, I had to disable my extensions one by one to find the offending extension.
Comment 28 Filipe 2012-09-13 19:50:50 PDT
I'm still experiencing this issue -- albeit only on a new, blank tab.  I have the following bookmark, with the keyword set as 'm' :


When "m" or "m test" is typed into the location bar in an existing tab, the javascript executes properly.

However, when a new, blank tab is created, typing "m" or "m test" into the location bar does nothing (after pressing enter of course).

It should be noted that I do have browser.newtabpage.enabled = false.

In fact, this and other javascript bookmarklets do not work when clicked via the bookmarks toolbar.

Am I doing something wrong? Can somebody please enlighten me?

Firefox v15.0.1
Windows Vista SP2
Comment 29 Jesse Ruderman 2012-09-13 20:57:33 PDT
Filipe, can you file a new bug for that (and mention the bug number here)? Thanks!
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2012-09-13 23:58:24 PDT
> I'm still experiencing this issue -- albeit only on a new, blank tab

That's because it's not blank.  It's about:home.

You probably want to set "browser.newtab.url" to about:blank.

> It should be noted that I do have browser.newtabpage.enabled = false.

That sadly doesn't affect what gets loaded in "blank" tabs, just how it looks to the user.
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2012-09-13 23:58:41 PDT
And yes, do file a bug.  The setup is super-broken.  :(
Comment 32 Filipe 2012-09-14 16:06:27 PDT
Thanks to Boris, it seems I've found the cause/culprit.  I had browser.newtab.url = about:newtab

Changing it to "about:blank" seems to fix the issue.  Now, invoking (javascript) keyword bookmarks on the location bar works; as well as clicking on bookmarklets on the bookmarks toolbar (on a new/blank tab).

Additionally, setting the newtab.url to null or any valid link will also allow javascript bookmarks to work.

So I don't know if my original issue could actually be considered a bug, or just PEBKAC. =]

Thanks Boris !
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2012-09-14 16:14:54 PDT
It's a bug, because "about:newtab" is the default value we ship with!
Comment 34 al_9x 2012-09-14 16:29:56 PDT
Bug 791407

Note You need to log in before you can comment on or make changes to this bug.