217363 - bookmark keywords ending in space or containing spaces aren't recognized when used with search terms (smart keywords)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Joachim Vance]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030728 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030728 Mozilla Firebird/0.6.1

When entering bookmark keywords (also called keymarks by Eric A. Meyer in the
devedge link http://devedge.netscape.com/viewsource/2002/bookmarks/ ) an
accidental space at the end causes the keyword to not be recognized when used in
the location bar with a search term to fill the %s of the URL. Instead the words
get used in a google "I'm Feeling Lucky" search as if there were no keyword
present.  

The keyword by itself (without a search term) seems to work fine to load a
bookmark even if it has a space.

Reproducible: Always

Steps to Reproduce:
1. Setup a bookmark with a %s in the URL.
2. Set a keyword that ends in a space
3. Use the keyword with search terms in the location bar to attempt to access
the bookmark and fill the %s.

Actual Results:  
Google "I'm Feeling Lucky" search occurs

Expected Results:  
Mozilla (Firebird) should ignore the space at the end of the keyword when
matching keyword and filling in the search terms in the %s of the URL. 
Additionally, keywords (keymark) containing a space in the middle should also
work (a keyword such as "search this").

Comment 1

[Mike Connor [:mconnor]]

okay, so I think I have this one tested out fully.  Since the string set is "foo
" not "foo", using "foo" won't find a match in keywords and will proceed to the
URL search terms.  "search this" as a simple bookmark keyword works fine, but
breaks on search bookmarks, due to how the arguments get called. "search " works
as a search bookmark using the syntax "search  foo" but not with "search foo"
due to there not being a space between the keyword string and the search
arguments (since the first space is part of the string).

The simple solution is to prevent spaces from being entered in keywords,
although this would exclude a simple bookmark keyword such as "my bugs" from
being used for non-search bookmarks, however the inconsistency is something that
should be avoided.  Preventing people from getting any impression, however
misguided, that Firebird is broken is a good thing.

Comment 2

[Mike Connor [:mconnor]]

we could prevent use of spaces directly in the properties window
by doing:
document.getElementById("shortcut").value =
document.getElementById("shortcut").value.split(" ").join("");
via the onkeyup event in the keyword field.

or we could just transparently strip them out (possibly confusing people trying
to set multiple word keywords, but should they even do this?)

      if (newvalue && gProperties[i] == (NC_NS + "ShortcutURL")) {
        // shortcuts are always lowercased internally
        newvalue = newvalue.toLowerCase();
+       // strip spaces
+       newvalue = newvalue.split(" ").join("");
      }

personally, I like the second option better, since in the most part, people
won't think about or try to use multiple word keywords.  Those who do will
probably go back in and see the spaces removed and figure it out.

Comment 3

[Joachim Vance]

Of the two options you give, I think it is better to prevent spaces from being
entered at all in the keywords.  Transparently deleting spaces for those that
intentionally enter them to create a multi-word keyword will just frustrate
those users because they know what they thought they did and Firebird changes it
on them without informing them. Preventing spaces from being entered gives no
such illusion but could be frustrating for those who don't understand why their
spacebar doesn't work. :)

I still think that the ideal is:
1. Transparently trim any trailing spaces from the keyword. This solves the
hanging space problem.
2. Consider changing how search bookmarks get called to allow multi-word
keywords to be set with spaces to correctly pass the URL search terms after the
multi-word keyword.

Comment 4

[Phil Ringnalda (:philor)]

*** Bug 274634 has been marked as a duplicate of this bug. ***

Comment 5

[Phil Ringnalda (:philor)]

--> mconnor, per comment in bug 274634

Comment 6

[Mike Connor [:mconnor]]

blocking entry of spaces seems to make more sense right now, will clean this up
for 1.1.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

*** Bug 304415 has been marked as a duplicate of this bug. ***

Comment 8

[Adam Guthrie]

Attachment: el patch

Shouts go out to mconnor for giving me something easy to search LXR for to and to logan for help with the regexp.

Comment 9

[Adam Guthrie]

Filed bug 341795 for doing this for Places.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/bookmarks/content/bookmarksProperties.js 	1.25
mozilla/browser/components/bookmarks/content/bookmarksProperties.js 	1.22.4.3