245597 - Keyword search should be activated for anything that isn't parseable as a URI (odd behaviour of search from address bar)

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Chris Lightfoot]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8

Firefox tries to guess whether what's being typed is a search term or something
intended to be a URL. However, it is either too clever or too stupid to get this
right. For instance, if I want to find out about (let's say) Albania, I might
type "Albania" in the address bar and press <ENTER>. Firefox attempts to load
"http://albania/". This is obviously sensible (and the same behaviour as
Mozilla), since otherwise you'd have to type eight whole extra characters to
visit a site on a machine called "albania" on your local network. So in Mozilla
you typed "Albania " and it would do a search. Firefox, by contrast, decides
that you must have typed the space by accident, and goes to "http://albania/".
You can't even get around it by typing ""albania"" (i.e. putting the word in
quotes in the address bar), since it then tries to visit "http://"albania"/".
This is plain dumb. Even more stupid, if you type in a search term which
contains a dot, EVEN IF IT CONTAINS SPACES, it assumes that it must be a URL,
and tries to visit it (see steps to reproduce).

Reproducible: Always
Steps to Reproduce:
1. Switch the address bar to do a Google search by adding
     user_pref("keyword.URL", "http://www.google.com/search?btnG=Google+Search&q=");
   to ~/.phoenix/default/LONG RANDOM STRING/user.js

2. Quite and restart the browser.

3. Type (for instance)
     firefox userChrome.css<ENTER>
   in the address bar.
Actual Results:  
Browser pops up message box saying,
  The URL is not valid and cannot be loaded.
(the URL in question being "

Expected Results:  
Browser should search Google for
  firefox userChrome.css

There's no reason to have separate search and address boxes. Input to a search
engine is not generally a valid. Mozilla's behaviour here was suboptimal, but at
least let the user decide: on typing in something which didn't look like a URL,
it popped up a little button thingy under the address bar with the label "Google
search". Clicking on this would let you perform a search. Firefox should try to
interpret the contents of the address bar as a URL or hostname, and if it cannot
(for instance if either contains a space or quotes or whatever) should assume
that they are search terms. It should never pop up the "URL not valid" box when
search-from-address-bar is switched on.

Comment 1

[Dimitrios]

behaviour in latest branch builds is different. But if you ask to do away with
the search bar, then this is an enhancement request, not a bug.

Comment 2

[Chris Lightfoot]

(In reply to comment #1)
> behaviour in latest branch builds is different.

ok, I've tried the latest nightly that's been built for Linux (the one for
2004-06-04-15-09, but it needs a later version of libstdc++, failing with the
error:
 error while loading shared libraries: libstdc++.so.5: cannot open shared object
file: No such file or directory
(That's not mentioned in the system requirements in README.txt, BTW)

> But if you ask to do away with
> the search bar, then this is an enhancement request, not a bug.

Well, it's already possible to remove the search bar -- just put
  #search-container { display: none; }
in userChrome.css. So I don't think an enhancement request is needed. The problem
is that doing so loses functionality, but there's no reason for it to do so.

Comment 3

[Chris Lightfoot]

Attachment: If a URL can't be parsed from what's entered in the address bar, force interpretation as a "keyword" (search) string

Comment 4

[Chris Lightfoot]

ok, the above patch fixes this problem (it doesn't fix the stripping-of-spaces
thing, but that's easy too). I haven't added new config options.

Here's another bug: type 2 + 2 in the address bar and hit enter. This should do
a Google search for 2 + 2, which tells you the answer is 4. Instead somewhere
along the line, the + gets turned into a space (presumably the search terms aren't
properly escaped, so there's a literal + in the URL which is passed to Google,
which it unescapes back to a space) and instead you get a Google search for 2   2
(with three spaces). This is especially confusing because the first search result
for that query is titled "2+2=5"....

Comment 5

[Chris Lightfoot]

Attachment: Updated version of previous patch, now against current Firefox CVS; also fixes escaping in keyword searches

OK. This patch:
  - forces interpretation of anything in the address bar as a keyword search,
if
    it can't be interpreted as a URI;
  - fixes escaping of parameters passed up to the keyword module so that
searches
    containing "+" work.

I haven't altered the trimming of spaces from both ends of the URI.

Comment 6

[David P James]

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

Comment 7

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

Chris, how does this compare with the fix in bug 261608? It was just checked in.
Please look into, and mark this a dupe if it fixes the issue.

Comment 8

[Chris Lightfoot]

(In reply to comment #7)
> Chris, how does this compare with the fix in bug 261608? It was just checked in.
> Please look into, and mark this a dupe if it fixes the issue.

I haven't had a chance to try it out yet but....

Similar issue, different approach. My patch assumes that anything which isn't
a parseable URL should be interpreted for keyword search; the patch in
bug 261608 looks for particular patterns in the entered text to see whether the
thing is likely to be a keyword search. It doesn't fix the "www.foo.com " case
(used to do google search for "www.foo.com", now tries to access "www.foo.com".)

Comment 9

[Dave Townsend [:mossop]]

Chris, it looks like your patch may resolve bug 295991 which I have been working
on and probably in a slightly better way than I am doing. Can you tell me though
why you are using url_XAlphas for the escaping and not url_XPAlphas, I was under
the impression that the latter correctly worked for query strings.

Also, is this bug likely to get resolved anytime soon?

Comment 10

[Dave Townsend [:mossop]]

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

Comment 11

[Peter van der Woude [:Peter6]]

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

Comment 12

[Andrew Neitsch]

To get a search from the address bar, prefix it with a question mark, for example
"?firefox userChrome.css". The search URL found in Keywork.URL is used; by default this is the "I'm feeling lucky" google search, but this can easily be changed in about:config.

To remove the search box, right-click on the toolbar, select "Customize..." and drag it off.

Comment 13

[Peter Kasting]

Taking with intent to un-bitrot patch and see if I can get some traction with Ben et. al.  This bug has irritated me for a LONG time.

Comment 14

[Peter Kasting]

Attachment: Simpler, more correct fix

This patch should address both this bug and bug 295991.  It does the following things:

(1) Fall back to a keyword search if we were unable to construct a valid URL from the user's input.  Input like "hello. how are you?" will trigger this.  This is better than the old "The URI is not valid" error dialog that would occur here.
(2) Properly escape search strings so that a search for "c++" will actually be interpreted as a search for "c++" and not "c  ".  See also other testcases in bug 295991.
(3) Remove the crazy special-case functionality that we would not include the word "go" in a keyword search if it was the first word of the search.  The surrounding keyword-detection code does not check for this, and it prevents users from (as) easily searching for strings starting with "go".
(4) Don't unescape input strings before re-escaping them and sending them out.  If a user actually types "c%2f" as their search string, we should just search for "c%2f" instead of assuming they meant "c/".

Darin suggested I just mark him as r+ on this, but the patch does a bit more than what he saw, so I want to give him a chance to glance over it again if he wishes.

If this is deemed safe enough, I'd like to get it in the branch for Firefox 2.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Don't unescape input strings before re-escaping them and sending them out. 

Won't this break some of our code?  For example, the keyword fixup that happens in nsWebShell::EndLoad is working with a string that Necko helpfully escaped, no?

I'll try to get to this patch sometime in the next week, I guess... I don't know this code so well.  :(

Comment 16

[Darin Fisher]

Boris, the unescaping was being done to detect any "go" prefix (or "?" prefix) of the input string.  In nsDefaultURIFixup::KeywordURIFixup, we don't unescape before analyzing the input string, so I think we shouldn't need to do so here.  Moreover, it seems that we are not consistently checking for "go" as a keyword invoking prefix.  Finally, I don't think the "go" prefix is very useful.  What if I type "go go gadget arms" in the URL bar?  I unfortunately will end up searching for "go gadget arms", which does not give quite the same results.

Are you worried about double-escaping where none was expected?  What we really need is to get access to what the user typed :-/

Comment 17

[Peter Kasting]

I would think the real concern is, in fact, double-escaping.  My testcase was to try "español" in the urlbar, and make sure that it only got singly escaped rather than doubly escaped (so that when it got to google, it was searched as the escaped form of español instead of the escaped form of espa%C3%B1ol).

If there is a better testcase to trigger that code I'll be happy to try it.

Comment 18

[Darin Fisher]

Comment on attachment 223849 [details] [diff] [review]
Simpler, more correct fix

Well, I think this will work.  Let's give it a try.

nsIURI::GetHost should not return something %-escaped, so unescaping shouldn't be necessary anyways.

r=darin

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 223849 [details] [diff] [review]
Simpler, more correct fix

> nsIURI::GetHost should not return something %-escaped

Ok.  That was basically what I wanted to know.  I guess nsIURI does document this.  ;)

>Index: docshell/base/nsDefaultURIFixup.cpp

> nsresult nsDefaultURIFixup::KeywordURIFixup(const nsACString >+    if (aForce || qMarkLoc == 0 ||
>+        (dotLoc == kNotFound || (spaceLoc > 0 && spaceLoc < dotLoc)) &&

So this has the form:  A || B || (C && D && E ... ), right?

Could you put in those parens around all the && stuff, just to make it clear what's going on?  Yes, I know they didn't use to be there....

>Index: docshell/base/nsDefaultURIFixup.h
>+    nsresult KeywordURIFixup(const nsACString &aStringURI, nsIURI** aURI,
>+                             PRBool aForce = PR_FALSE);

Please don't use a default value here.  There are exactly two callers (one passing true, one passing false); adding one more line to the diff won't hurt anything and will make the code clearer.

Also, document exactly what the aForce arg forces?

sr=bzbarsky with those nits.

Comment 20

[Darin Fisher]

> > nsIURI::GetHost should not return something %-escaped
> 
> Ok.  That was basically what I wanted to know.  I guess nsIURI does document
> this.  ;)

OK, let's do that in bug 309671.

Comment 21

[Peter Kasting]

Attachment: patch, final version

Added the parens, and bypassed all the default arg stuff altogether by just calling straight through to where I really wanted to go in the case of forcing a keyword fixup.  I don't know why I didn't think of this before, it's definitely cleaner.

I'd like this to make it on to the branch, but maybe it should bake on the trunk for a bit first.

Comment 22

[Boris Zbarsky [:bzbarsky]]

You still have the default arg in the header...  With this patch, in fact, none of the changes to KeywordURIFixup are needed, right?  I'd prefer we didn't make them, if so.

And yes, this will need to bake on the trunk for a few weeks before I'd consider it OK for branch.

Comment 23

[Peter Kasting]

No I don't... the new patch doesn't change the .h file at all.  I can attach a new patch that doesn't do the cleanup fix to KeywordURIFixup.

Comment 24

[Peter Kasting]

Attachment: patch, REALLY final version (I hope)

Don't cleanup KeywordURIFixup().

Comment 25

[Peter Kasting]

Nominating for Fx2 B1.  This needs to bake on the trunk before going in, but it makes some nice escaping fixes and makes keyword search a little less dumb, so I think it'd be nice to have if it's safe enough.

Comment 26

[Peter Kasting]

Adding a target milestone to increase likelihood of search queries finding me

Comment 27

[Darin Fisher]

Not going to block 1.8.1 for this, but we'll happily consider a baked-on-trunk patch.

Comment 28

[Brett Wilson]

Fixed on trunk.

Comment 29

[Peter Kasting]

This patch should have also fixed bug 245597 (which I've just marked FIXED).  When this is fixed-on-branch that bug probably should be as well.

Comment 30

[Dave Townsend [:mossop]]

Is there a reason this isn't marked as FIXED?

Comment 31

[Peter Kasting]

Because brettw and I are trying not to forget to check in on branch once this gets approval.

I'll mark it fixed once it gets approval+ or approval-.

Comment 32

[Peter Kasting]

Attachment: branch version of final patch

Since various things have moved around on the branch, the trunk doesn't apply cleanly.  Here's a branch version of the patch.

Comment 33

[Peter Kasting]

Brett just checked this in on branch.

Also, in comment #29, I meant bug 295991.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Argh.  I need to land bug 337339 on branch, so this is going to cause me merge hell.  :(  I'll try to make sure I don't just back this patch out as I go... :(

Comment 35

[Peter Kasting]

Actually, if you want, back out my patch, merge your changes in, and then commit the trunk version of my patch.  That feels safer (to me anyway) and hopefully causes you less pain.