Last Comment Bug 332538 - port firefox's "Add a Keyword for this search" feature
: port firefox's "Add a Keyword for this search" feature
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: seamonkey2.1b2
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-02 19:35 PDT by Andrew Schultz
Modified: 2011-01-10 12:54 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (11.06 KB, patch)
2011-01-04 10:38 PST, Jens Hatlak (:InvisibleSmiley)
neil: review-
neil: feedback+
Details | Diff | Review
patch v2 (10.02 KB, patch)
2011-01-05 03:18 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review
patch v2a [Checkin: comment 17] (10.01 KB, patch)
2011-01-08 06:00 PST, Jens Hatlak (:InvisibleSmiley)
jh: review+
Details | Diff | Review

Description Andrew Schultz 2006-04-02 19:35:19 PDT
In firefox, if you right-click on a form input, one of the context menu items is "Add a Keyword for this Search".  It brings up the add bookmark dialog and you can enter a name and keyword.  It uses the form's action to generate a bookmark.

This would lower the learning curve for creating the (rather useful) bookmark keywords since currently you have to perform a pretend search and then edit the generated URL, replacing the query with %s.  The firefox feature also handles POST forms (assuming they can also take GET data).
Comment 1 Phil Ringnalda (:philor) 2006-04-02 20:15:18 PDT
Actually, no, Firefox POSTs for method="post" forms, there's no requirement that they take GET.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2010-09-23 14:29:48 PDT
It seems the initial implementation was done without a bug:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=browser.js&branch=MOZILLA_1_8_BRANCH&root=/cvsroot&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&rev1=1.295&rev2=1.296

Support for POST was added in bug 241028 then.

Until we add support in SM directly, this extension might do the trick:
<http://xsidebar.mozdev.org/modifiedmisc.html#searchenginewizard>
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-01-04 10:38:22 PST
Created attachment 501057 [details] [diff] [review]
patch

Ported from current FF code. Really easy now that we use Places for bookmarks. :-)

Please put this after Sync and before final beta on your priority list.
Comment 4 neil@parkwaycc.co.uk 2011-01-04 13:51:41 PST
Comment on attachment 501057 [details] [diff] [review]
patch

Sigh. If a small snippet of Firefox code can have 10 errors in it...

>+  var docURI = makeURI(node.ownerDocument.URL, charset);
>+  var formURI = makeURI(form.getAttribute("action"), charset, docURI);
>+  var spec = formURI.spec;
Just use form.action

>+  var isURLEncoded = form.method.toUpperCase() == "POST" &&
>+                     (form.enctype == "application/x-www-form-urlencoded" ||
>+                      form.enctype == "");
This variable is misleading. We only support encoded GET and encoded POST.

>+      return escape(aName + "=" + aValue);
This makes no sense at all. The = between the name and value is never escaped.

>+    return escape(aName) + "=" + escape(aValue);
In fact the name and value aren't escaped either. Use encodeURIComponent.

>+                                   escapeNameValuePair(el.name, "", false) + "%s");
Always use this version.

>+      for (var j=0; j < el.options.length; j++) {
Just picking on this example of incorrect spacing around =

>+    spec += "?" + formData.join("&");
Need to use & if spec already contains a ? (can't remember offhand what you need to do if spec already contains a #!)

>+    if (!form || aNode.type == "password")
IMHO we should use mozIsTextField(true) instead.

>+    var method = form.method.toUpperCase();
But form.method is already forced to lower case...

>+    // method   encoding type       can create keyword
>+    // GET      *                                 YES
>+    //          *                                 YES
>+    // POST                                       YES
Can't have blank method or enctype, so just check that method is one of the two we support and enctype is the one we support.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-01-04 14:02:55 PST
See bug 570266, bug 359809, and bug 620565
Comment 6 Jens Hatlak (:InvisibleSmiley) 2011-01-05 03:18:29 PST
Created attachment 501295 [details] [diff] [review]
patch v2

(In reply to comment #4)
> >+  var docURI = makeURI(node.ownerDocument.URL, charset);
> >+  var formURI = makeURI(form.getAttribute("action"), charset, docURI);
> >+  var spec = formURI.spec;
> Just use form.action

Really? What about bug 570266 and bug 264607?

> >+    spec += "?" + formData.join("&");
> Need to use & if spec already contains a ? (can't remember offhand what you
> need to do if spec already contains a #!)

No idea about the hash thing.
Comment 7 neil@parkwaycc.co.uk 2011-01-05 03:55:00 PST
(In reply to comment #6)
> (In reply to comment #4)
> > >+  var docURI = makeURI(node.ownerDocument.URL, charset);
> > >+  var formURI = makeURI(form.getAttribute("action"), charset, docURI);
> > >+  var spec = formURI.spec;
> > Just use form.action
> Really? What about bug 570266 and bug 264607?
Haven't looked at 570266 but 264607 was solved with XPCNativeWrappers (except for a silly bug where the action property is blank when the attribute is, when it should instead be the document's URI).
Comment 8 neil@parkwaycc.co.uk 2011-01-05 04:16:23 PST
(In reply to comment #5)
> See bug 570266,
OK, so form.action is weird. Should we also check the current element in case it has a formAction, see bug 607145?

> bug 359809,
Interesting... I'd have to wade through the form submission C++ code, which presumably gets this right. But the patch is still wrong, since it needs to convert from unicode before escaping.

> and bug 620565
OK, so this is a known bug.
Comment 9 Jens Hatlak (:InvisibleSmiley) 2011-01-05 06:43:56 PST
(In reply to comment #7)
> Haven't looked at 570266 but 264607 was solved with XPCNativeWrappers (except
> for a silly bug where the action property is blank when the attribute is, when
> it should instead be the document's URI).

IIUC that means I can in fact use form.action. How about this:

  var node = document.popupNode;
  var doc = node.ownerDocument;
  var charset = doc.characterSet;
  var form = node.form;
  var spec = form.action || makeURI(doc.URL, charset).spec;

(In reply to comment #8)
> (In reply to comment #5)
> > See bug 570266,
> OK, so form.action is weird. Should we also check the current element in case
> it has a formAction, see bug 607145?

I don't think that makes much sense. AFAIU formAction is mainly used for submit buttons but we only offer adding a keyword for text boxes.
Comment 10 neil@parkwaycc.co.uk 2011-01-05 13:14:28 PST
(In reply to comment #9)
>   var spec = form.action || makeURI(doc.URL, charset).spec;
Not sure what the effect of charset is here but the spec always seems to be the same as the doc.URL (or better still doc.documentURI in nsIDOM3Document).
Comment 11 Jens Hatlak (:InvisibleSmiley) 2011-01-05 14:41:51 PST
(In reply to comment #10)
> Not sure what the effect of charset is here but the spec always seems to be the
> same as the doc.URL (or better still doc.documentURI in nsIDOM3Document).

See originCharset on https://developer.mozilla.org/en/nsIURI#Attributes, if that helps. Unfortunately I don't know enough about fancy charset/URI combinations, I never came across anything but ASCII (including urlencoded) which is contained in UTF-8 (completely?). And punycode is ASCII, too, but I don't know whether doc.URL/doc.documentURI would be punycode or something else. Maybe Ratty or someone at Mozilla knows more about that topic or could create a test case. Otherwise I think the makeURI/charset approach should be safe. In any case we should choose something here and maybe leave that issue for a follow-up if need be.
Comment 12 neil@parkwaycc.co.uk 2011-01-06 05:11:14 PST
Comment on attachment 501295 [details] [diff] [review]
patch v2

>--- a/suite/browser/navigator.js
>+++ b/suite/browser/navigator.js
Bah, context menus in text fields in mailnews are already broken because it fails to check spelling, so I can't tell whether this does something reasonable (e.g. just ignoring it because the item is never available).

>+    type = el.type.toLowerCase();
IIRC .type is already lower case
Comment 13 neil@parkwaycc.co.uk 2011-01-06 05:20:58 PST
(In reply to comment #11)
> (In reply to comment #10)
> > Not sure what the effect of charset is here but the spec always seems to be the
> > same as the doc.URL (or better still doc.documentURI in nsIDOM3Document).
> See originCharset on https://developer.mozilla.org/en/nsIURI#Attributes, if
> that helps. Unfortunately I don't know enough about fancy charset/URI
> combinations, I never came across anything but ASCII (including urlencoded)
> which is contained in UTF-8 (completely?). And punycode is ASCII, too, but I
> don't know whether doc.URL/doc.documentURI would be punycode or something else.
It's not, I tried. But for fancy characters in the path, they're always UTF-8 encoded by the time they each doc.documentURI, even on Windows-1252 pages. So we can just use || doc.documentURI (which, unlike document.location, also doesn't have a #, so that's one less thing to think about.)

I believe the character set might make a difference for POST data, but we'll cross that bridge when we get to it.
Comment 14 Jens Hatlak (:InvisibleSmiley) 2011-01-06 07:05:01 PST
(In reply to comment #12)
> Bah, context menus in text fields in mailnews are already broken because it
> fails to check spelling

Such things don't tend to fix themselves, so I filed bug 623590 for you.

> so I can't tell whether this does something reasonable
> (e.g. just ignoring it because the item is never available).

If I comment out the lines that break the MailNews context menu, it appears normally, and produces no error messages related to the new code (as you said, the item is never available since it's defined in the same file where the item the absence of which breaks the context menu is defined).

> >+    type = el.type.toLowerCase();
> IIRC .type is already lower case

So it seems, and allows us to get rid of the temp variable altogether (s/type/el.type/).

(In reply to comment #13)
> So we can just use || doc.documentURI (which, unlike document.location, also
> doesn't have a #, so that's one less thing to think about.)

Applied locally. Anything issues left open? Want a new patch?
Comment 15 neil@parkwaycc.co.uk 2011-01-07 16:47:25 PST
(In reply to comment #14)
> (In reply to comment #12)
> > >+    type = el.type.toLowerCase();
> > IIRC .type is already lower case
> So it seems, and allows us to get rid of the temp variable altogether
Well, it does get used several times...

> (In reply to comment #13)
> > So we can just use || doc.documentURI (which, unlike document.location, also
> > doesn't have a #, so that's one less thing to think about.)
> Applied locally. Anything issues left open? Want a new patch?
Only for posterity.
Comment 16 Jens Hatlak (:InvisibleSmiley) 2011-01-08 06:00:57 PST
Created attachment 502231 [details] [diff] [review]
patch v2a [Checkin: comment 17]

Fear not, I added a semicolon after "let el = null". :-)
Comment 17 Jens Hatlak (:InvisibleSmiley) 2011-01-08 06:05:20 PST
Comment on attachment 502231 [details] [diff] [review]
patch v2a [Checkin: comment 17]

http://hg.mozilla.org/comm-central/rev/d31ec9269f65
Comment 18 neil@parkwaycc.co.uk 2011-01-09 06:57:25 PST
(In reply to comment #16)
> Created attachment 502231 [details] [diff] [review]
> patch v2a [Checkin: comment 17]
> 
> Fear not, I added a semicolon after "let el = null". :-)

You also added a bunch of trailing spaces :-(
Comment 19 Jens Hatlak (:InvisibleSmiley) 2011-01-10 12:54:51 PST
(In reply to comment #18)
> You also added a bunch of trailing spaces :-(

Yes, in two otherwise empty lines. Sorry for that. I blame our policy, though (at work I can let my editor program remove all trailing white space, for Mozilla-related stuff I have to maintain exceptions just so that the existing ugly code stays blame-friendly). Anyway, I'm digressing. If you like I can push another white space fix (same bug and similar time, so should be blame-friendly).

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