Closed Bug 658220 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox :: Address Bar, defect)

Product:
Firefox
Component:
Address Bar
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 7
Tracking Flags:
Tracking Status
firefox6 + fixed

People

(Reporter: al_9x, Assigned: Gavin)

References

Details

(Keywords: regression, Whiteboard: nominated by bz in comment 1)

Attachments

(1 file, 3 obsolete files)

comments addressed.
(deleted), patch
johnath
: approval-mozilla-aurora+
Details | Diff | Splinter Review
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.
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.
(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?
> 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.
Attached patch patch (obsolete) (deleted) — — Splinter 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).
Attached patch patch (obsolete) (deleted) — — Splinter Review
I think this is a more robust patch.
Attachment #533996 - Attachment is obsolete: true
Attachment #533997 - Flags: review?(dao)
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-
Attached patch patch (obsolete) (deleted) — — Splinter 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
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+
Attached patch comments addressed. (deleted) — — Splinter Review
Attachment #536758 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/3a8fdde338be
Status: NEW → RESOLVED
Closed: 14 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+
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
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?
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.
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?
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?
The Delicious extension (as of version 2.3.1) also breaks this functionality. https://addons.mozilla.org/en-US/firefox/addon/delicious-extension/
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?
Yes, I had to disable my extensions one by one to find the offending extension.
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
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. :(
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!
Bug 791407
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: