The default bug view has changed. See this FAQ.

Invoking bookmarklets by keyword no longer works (broken by Bug 656433)

VERIFIED FIXED in Firefox 6

Status

()

Firefox
Location Bar
--
major
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: al_9x, Assigned: Gavin)

Tracking

({regression})

Trunk
Firefox 7
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox6+ fixed)

Details

(Whiteboard: nominated by bz in comment 1)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.17) 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('http://www.google.com/search?q=%s','_self');open('http://www.bing.com/search?q=%s');"
keyword: kw

2. type "kw test<enter>" on the urlbar
Blocks: 656433
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.  :(
Severity: normal → major
Status: UNCONFIRMED → NEW
tracking-firefox6: --- → ?
Ever confirmed: true
Keywords: regression
Version: unspecified → Trunk
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.
Assignee: nobody → gavin.sharp
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.
(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

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

Comment 6

6 years ago
(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

6 years ago
> The fact that things work if you click "Go" is actually a bug in the patch for 
> bug 656433.

Split into bug 658383.
Summary: keyword bookmarklets don't run on enter (broken by Bug 656433), but do on go click → Invoking bookmarklets by keyword no longer works (broken by Bug 656433)
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.
Created attachment 533996 [details] [diff] [review]
patch

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).
Created attachment 533997 [details] [diff] [review]
patch

I think this is a more robust patch.
Attachment #533996 - Attachment is obsolete: true
Attachment #533997 - Flags: review?(dao)

Updated

6 years ago
Whiteboard: nominated by bz in comment 1
Comment on attachment 533997 [details] [diff] [review]
patch

>-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];
Attachment #533997 - Flags: review?(dao) → review-

Updated

6 years ago
tracking-firefox6: ? → +
Created attachment 536758 [details] [diff] [review]
patch

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
Attachment #533997 - Attachment is obsolete: true
Attachment #536758 - Flags: review?(dao)
Comment on attachment 536758 [details] [diff] [review]
patch

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.
Attachment #536758 - Flags: review?(dao) → review+
Created attachment 536907 [details] [diff] [review]
comments addressed.
Attachment #536758 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/3a8fdde338be
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox6: --- → affected
Flags: in-testsuite+
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Attachment #536907 - Flags: approval-mozilla-aurora?
Attachment #536907 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/db21d923f310
status-firefox6: affected → fixed

Comment 17

6 years ago
 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.
Status: RESOLVED → VERIFIED

Comment 18

6 years ago
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
This should work in a 7.0 beta build, yes...  What bookmarklet are you using that doesn't work?

Comment 20

6 years ago
I have a bunch of this form:

javascript:if%20('%s')%20location.href='http://www.kongregate.com/search?q=%s';%20else%20location.href='http://www.kongregate.com/badges'

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

I can't find any that work.

Comment 21

6 years ago
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
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

6 years ago
Whoops.  My fault.  The problem seems to be with the Clumsy Fingers extension.  http://avery.morrow.name/software/clumsyfingers
Bitt, thank you for following up on that!

Could you report the issue to the extension's author?

Comment 25

5 years ago
The Delicious extension (as of version 2.3.1) also breaks this functionality.

https://addons.mozilla.org/en-US/firefox/addon/delicious-extension/

Comment 26

5 years ago
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

5 years ago
Yes, I had to disable my extensions one by one to find the offending extension.

Comment 28

5 years ago
I'm still experiencing this issue -- albeit only on a new, blank tab.  I have the following bookmark, with the keyword set as 'm' :

javascript:location.href=('%s')?"http://maps.google.com/maps?q=%s":"http://maps.google.com/maps"

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

5 years ago
Filipe, can you file a new bug for that (and mention the bug number here)? Thanks!
> 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.
And yes, do file a bug.  The setup is super-broken.  :(

Comment 32

5 years ago
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 !
It's a bug, because "about:newtab" is the default value we ship with!
(Reporter)

Comment 34

5 years ago
Bug 791407
You need to log in before you can comment on or make changes to this bug.