303628 - Bookmark keywords with spaces gives "Invalid URL" error if entered in address bar (do input validation on keyword entry)

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[froodian (Ian Leue)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050531 Camino/0.9+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050531 Camino/0.9+

If you make a bookmark with a keyword with a space (eg "this keyword"), then try
to call that bookmark by entering the keyword in the address bar, Camino tries
to read the keyword as a URL.

Reproducible: Always

Steps to Reproduce:
1. Make a new bookmark to www.google.com, giving it a keword of "this keyword"
2. Type "this keyword" in address bar
Actual Results:  
Sheet error, saying that the URL is not valid.

Expected Results:  
either bring up the bookmark, or don't allow the keyword to have a space in the
first place.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Are keywords even allowed to contain spaces?  I couldn't find any documentation
that addressed the issue.

Otherwise, it sounds a little like bug 197918.

Comment 2

[Ludovic Hirlimann [:Usul]]

(In reply to comment #1)
> Are keywords even allowed to contain spaces?  I couldn't find any documentation
> that addressed the issue.
> 
> Otherwise, it sounds a little like bug 197918.
-(NSArray *)resolveBookmarksKeyword:(NSString *)keyword only knows how to deal
with two words maximum.

-(NSArray*)resolveKeyword:(NSString *)keyword withArgs:(NSString *)args

seems to be our problem.

I say we add a note to the release note about that issue, or we enforce the one
word thingy when creating such an entry in the BM.

Comment 3

[Samuel Sidler (old account; do not CC)]

Simon and I talked about this on IRC.

We don't allow two-word keywords because the second word is always assumed to 
be a string to replace %s (or whatever it is). This is a core issue, but it's 
done on purpose. We could, however, change the error message.

Otherwise, this is a WONTFIX.

Comment 4

[froodian (Ian Leue)]

What's the status on this bug?  Is it WONTFIX?  I'd prefer if the error message
were changed, since as it is it's unclear to the user what's going on.  

Even better than that (in my mind) would be an error message that comes up when
the user first tries to make a keyword with a space in it.

Comment 5

[Samuel Sidler (old account; do not CC)]

If nothing else, we should change the dialog that comes up when a user attempts to go to a two-word keyword bookmark.

CCing Simon, targeting for 1.1.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We should do input validation on the keyword field in the manager proper and in the Info panel.  Validating for lack of whitespace should be very easy, just a few lines of code per irc.

If we can fix the error sheet ("The URL is not valid and cannot be loaded."), too, we should. My guess it comes from Necko, though, or that we get a generic error message that can't be mapped to this problem specifically.

Yep, it's a chrome string for malformed URL: http://lxr.mozilla.org/mozilla/source/dom/locales/en-US/chrome/appstrings.properties#37 (which sounds like it's fairly generic, too!)

Comment 7

[Stuart Morgan]

Attachment: prevent spaces

Just say no to spaces.

Comment 8

[Chris Lawson (gone)]

Comment on attachment 243635 [details] [diff] [review]
prevent spaces

r=cl

Comment 9

[Mike Pinkerton (not reading bugmail)]

the class declaration needs comments to say what it's for and what it does.

sr=pink with that updated.

Comment 10

[Stuart Morgan]

Attachment: with comment

With comment, as checked into trunk and MOZILLA_1_8_BRANCH