Closed Bug 332538 Opened 18 years ago Closed 13 years ago

port firefox's "Add a Keyword for this search" feature

Categories

(SeaMonkey :: Bookmarks & History, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b2

People

(Reporter: ajschult784, Assigned: InvisibleSmiley)

Details

Attachments

(1 file, 2 obsolete files)

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).
Actually, no, Firefox POSTs for method="post" forms, there's no requirement that they take GET.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #501057 - Flags: review?(neil)
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.
Attachment #501057 - Flags: review?(neil)
Attachment #501057 - Flags: review-
Attachment #501057 - Flags: feedback+
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #501057 - Attachment is obsolete: true
Attachment #501295 - Flags: review?(neil)
(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).
(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.
(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.
(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).
(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 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
(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.
(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?
(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.
Attachment #501295 - Flags: review?(neil) → review+
Fear not, I added a semicolon after "let el = null". :-)
Attachment #501295 - Attachment is obsolete: true
Attachment #502231 - Flags: review+
Comment on attachment 502231 [details] [diff] [review]
patch v2a [Checkin: comment 17]

http://hg.mozilla.org/comm-central/rev/d31ec9269f65
Attachment #502231 - Attachment description: patch v2a → patch v2a [Checkin: comment 17]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
(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 :-(
No longer blocks: FF2SM
(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).
You need to log in before you can comment on or make changes to this bug.