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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: froodian (Ian Leue), Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

5.43 KB, patch
Stuart Morgan
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
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.
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
(Reporter)

Comment 4

12 years ago
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.
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.
Target Milestone: --- → Camino1.1
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!)
OS: Mac OS X 10.2 → Mac OS X 10.3
QA Contact: bookmarks
Summary: Bookmark keywords with spaces gives "Invalid URL" error if entered in address bar → Bookmark keywords with spaces gives "Invalid URL" error if entered in address bar (do input validation on keyword entry)
Whiteboard: [good first bug]
(Assignee)

Comment 7

11 years ago
Created attachment 243635 [details] [diff] [review]
prevent spaces

Just say no to spaces.
Assignee: mikepinkerton → stuart.morgan
Status: NEW → ASSIGNED
Attachment #243635 - Flags: review?

Comment 8

11 years ago
Comment on attachment 243635 [details] [diff] [review]
prevent spaces

r=cl
Attachment #243635 - Flags: review? → review+
(Assignee)

Updated

11 years ago
Attachment #243635 - Flags: superreview?(mikepinkerton)
the class declaration needs comments to say what it's for and what it does.

sr=pink with that updated.
(Assignee)

Comment 10

11 years ago
Created attachment 244805 [details] [diff] [review]
with comment

With comment, as checked into trunk and MOZILLA_1_8_BRANCH
Attachment #243635 - Attachment is obsolete: true
Attachment #244805 - Flags: superreview+
Attachment #243635 - Flags: superreview?(mikepinkerton)
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.1.1
You need to log in before you can comment on or make changes to this bug.