Don't encode fwd-slash ( / ) when doing keyword substitution

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Location Bar & Autocomplete
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment)

2.98 KB, patch
Chris Lawson (gone)
: review+
Håkan Waara
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
In bug 326532, in addition to &, a number of other characters apparently got encoded.

Encoding / breaks a number of my keywords (Firefox does not encode /, and at least Google searches for items with / in them work properly with / not encoded).
Any idea how the fix for bug 295991 (a fix for encoding problems with keyword search at the docshell level) affects this (and the post-bug 326532 encoding situation in general)?

Comment 2

11 years ago
It seems better in general to assume that user-input should be escaped when it overlaps with reserved characters, as they are not likely to be using them in their reserved meanings--and doing too much escaping is generally fine for URLs.  Could you give an example of a keyword that breaks because of encoding / ?
(Assignee)

Comment 3

11 years ago
STR:

1. Make a keyworded bookmark to http://wiki.caminobrowser.org/Main_Page/%s
2. invoke the bookmark with a %s of Development:Reviewing

What happens: http://wiki.caminobrowser.org/Main_Page/Development%3AReviewing is loaded
What should happen: http://wiki.caminobrowser.org/Main_Page/Development:Reviewing is loaded
(Assignee)

Comment 4

11 years ago
(obviously, based on the original report, the same keyword will also break with the / for something "User:Sardisson/Dogfood")

Comment 5

11 years ago
(In reply to comment #3)
> STR:
> 
> 1. Make a keyworded bookmark to http://wiki.caminobrowser.org/Main_Page/%s
> 2. invoke the bookmark with a %s of Development:Reviewing
> 
> What happens: http://wiki.caminobrowser.org/Main_Page/Development%3AReviewing
> is loaded
> What should happen:
> http://wiki.caminobrowser.org/Main_Page/Development:Reviewing is loaded

Those are equivalent pages; I'm not seeing the bug.

(In reply to comment #4)
> (obviously, based on the original report, the same keyword will also break with
> the / for something "User:Sardisson/Dogfood")

If the page is named with a slash, it needs to be escaped in the URL, so that's not a valid case. Unless you are suggesting that users should be expected to construct valid URL fragments--in which case & and = shouldn't be escaped either in case I want to do something like do a Google search for "foo&btnI=I'm Feeling Lucky" when I want the top result.

Comment 6

11 years ago
I guess with the increasing popularity of RESTful URLs, I can see an argument for leaving '/' alone, and it should be fine in query strings according the RFC.  I think the other characters should stay escaped though, unless there are any others with compelling reasons not escape.
(Assignee)

Comment 7

11 years ago
Sorry, my examples should have all had the Main_Page/ removed from them (including the keyword).  Think before typing, think before typing.

/me takes a breather
(Assignee)

Comment 8

11 years ago
And yes, I realize now that the ":" encoding doesn't make a difference.  Maybe I'm doing more harm than good here...

(sorry for the bugspam)
(Assignee)

Comment 9

11 years ago
Created attachment 228608 [details] [diff] [review]
Doesn't Encode /

Does what it says on the package.  Consider this atonement for every other comment I've made in this bug ;)
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #228608 - Flags: review?(hwaara)

Comment 10

11 years ago
Comment on attachment 228608 [details] [diff] [review]
Doesn't Encode /

r=me, moving hwaara to second-r, asking for sr from pink.
Attachment #228608 - Flags: superreview?(mikepinkerton)
Attachment #228608 - Flags: review+

Updated

11 years ago
Attachment #228608 - Flags: review?(hwaara) → review+
Comment on attachment 228608 [details] [diff] [review]
Doesn't Encode /

sure, if everyone's happy with this. rs=pink
Attachment #228608 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Whiteboard: [needs checkin]

Comment 12

11 years ago
Baking on trunk and branch
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: regression → fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.