904731 - [browser] text entered into search bar containing dot (.) should not be treated as url address if it has whitespaces

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Unagi with Boot2Gecko 1.1.0.0-prerelease 20130812041203

When entering text with a dot and whitespaces into the address bar, it gets treated as url after pressing enter, but it should trigger a search.

Example:
Type "3. oktober 2013" and press Enter.

Comment 1

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

This patch fix below items
1. Space between any words should also be keyword search.
2. Update utilities_test.js, 'www.blah.com foo' and 'a?some b' should not be url.

Comment 2

[Dale Harvey (:daleharvey)]

So according to http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#783 'a?some b' is not a keyword formatted string

This patch is good but I am hesitant to commit it until we find out what firefox uses to handle this case, as we ported that function from firefox core and any mistakes in it can mean completely in accessible urls

Comment 3

[Dale Harvey (:daleharvey)]

Needinfoing as I am having trouble finding the logic in which this is handled by firefox desktop (finding androids behaviour may also be useful)

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java#260

Comment 5

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks Brad,

https://github.com/mozilla/mozilla-central/blob/master/mobile/android/base/util/StringUtils.java#L28

it seems that isSearchQuery will return false if the string is "abc. d" and then call openUrlAndFinish. However, almost all browsers take it as search query, even firefox browser. Please currect me if wrong.

I will try to track chromium's behavior later.

Comment 6

[Dale Harvey (:daleharvey)]

It definitely does a keyword search for me, I am happy with the patch and would like us to match desktops behaviour, but I am not comfortable landing until we understand desktops behaviour as getting this wrong means inaccessible urls

Comment 7

[Dale Harvey (:daleharvey)]

When we originally implemented this, we thought about using html5 input=url to do the validation, I forgot why we couldnt at the time (will cc ben to see if he has better memory) but it looks like it works for this case (<input type="url" value="http://abc. d" /> is an invalid url)

If there are no reasons not to, this seems like a more robust solution

Comment 8

[Dale Harvey (:daleharvey)]

Just to clarify, we couldnt use input type=url for the field because we need to enter search terms, however we can use 

    function isSearchQuery(term) { 
      var input = document.createElement('input'); 
      input.setAttribute('type', 'url'); 
      input.setAttribute('value', term); 
      return !input.validity.valid;
    }

This I wouldnt be as worried about the robustness, as if it does have any false negatives then they would need to be fixed in gecko (this example code would need to strip out schemas from the term)

Comment 9

[Ben Francis [:benfrancis]]

(In reply to Dale Harvey (:daleharvey) from comment #8)
> Just to clarify, we couldnt use input type=url for the field because we need
> to enter search terms

Yep, as discussed in IRC.

>, however we can use 
> 
>     function isSearchQuery(term) { 
>       var input = document.createElement('input'); 
>       input.setAttribute('type', 'url'); 
>       input.setAttribute('value', term); 
>       return !input.validity.valid;
>     }

Although it's a bit of a hack, that does seem preferable to trying to maintain a regular expression. I'd be interested to instrument this to see if it improves performance. Would it help if we didn't create the input element on every key press but instead kept one in the Browser object and just reset the value?

Comment 10

[(Inactive after June) George Duan [:gduan] [:喬智]]

It requires at least a scheme to validate, so we may need to check the existence of scheme and then manually add a "http://" on it.

However, it's just like a black box, I have less idea what it has checked. It fails the test in below case on my Aurora.

http://codepen.io/anon/pen/cblqE

Comment 11

[(Inactive after June) George Duan [:gduan] [:喬智]]

I should put the string I tested.
"http://abc.? d"

Comment 12

[Dale Harvey (:daleharvey)]

It being a black box is a good thing, it means we arent hardcoding duplicated url logic into the browser app, what is and isnt a url vs a keyword search has been problematic on desktop for years and in the browser app it continues to be so so if we can get rid of that code, all the better.

Then if we are absolutely certain "http://abc.? d" isnt a valid url (which I would naively agree), we can file a bug against the form validation

Comment 13

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/55559368

Comment 14

[(Inactive after June) George Duan [:gduan] [:喬智]]

Hi Dale, I updated the patch for below reasons, please kindly check again.

1. Since form require basic scheme check, I manually add scheme as 'http://' if it doesn't have.
2. And after testing, I found below cases with unexpected result, so I still add some regex validation, we can close it once fixed behind.
 bugs: 'data','a?some b','?mozilla','?site:mozilla.org docshell'
3. I move a test case 'foo:about' to 'isNotUrl => true', since I think port should be a number, unless it's another case.

Comment 15

[Dale Harvey (:daleharvey)]

Comment on attachment 790742 [details]
PR to master

Apologies, I commented on github but didnt mark it in a review, we need to be able to open data uri's which will require another special case, thats what that test was for

I am not sure we should keep the special cases for the firefox bugs as opposed to just filing bugs against gecko and waiting for them to be fixed

The rest looks good, thanks

Comment 16

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 790742 [details]
PR to master

Hi Dale, 
I just updated and add data:uri case. Please kindly check again.

Comment 17

[Dale Harvey (:daleharvey)]

Comment on attachment 790742 [details]
PR to master

Sorry for going round on this, but its important to get right, few issues:

1. This fails for plain keyword searches, 'http://foo' is a valid url, the 'plain string' regex exception has a ? in there which stops it matching

2. Data uri's start with data:, so the check should probably be a strict check for that

3. Not as important, but wondering why we are doing |(regex.exec(str) || [])[0])| instead of |regex.test(str)|

4. I definitely want us to file and fix the "?abc" and "a? b" cases in platform and remove the special cases for them, this is just going to leave dead special case code in the browser that we will forget to remove, without the special cases we still fix for a lot of cases, the rest can wait for platform

Comment 18

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 790742 [details]
PR to master

This patch has considered item 1,2,3 of comment 17.

Totally agree with you, it's important to get it right. 

I think platform side can also refer how chromium do for this.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/autocomplete/autocomplete_input.cc&q=AutocompleteInput::Type%20AutocompleteInput::Parse&type=cs&sq=package:chromium&l=122
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/autocomplete/autocomplete_input_unittest.cc

Comment 19

[Dale Harvey (:daleharvey)]

Comment on attachment 790742 [details]
PR to master

Sorry I wasnt clear previously

when you enter 'foo' as a plain string, we prepend http://foo, which is a valid url and visit that url, plain strings should definitely be keyword searches (whereas if a use entered http://foo they should visit the url, as they do)

Will also need added to tests

Comment 20

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 790742 [details]
PR to master

as discussed offline, the patch already added 'data' and 'http://foo' in tests and return correct result. Please let me know if anything I miss.

Comment 21

[Dale Harvey (:daleharvey)]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=910034 as a follow up for the platform bug

Comment 22

[Dale Harvey (:daleharvey)]

Comment on attachment 790742 [details]
PR to master

Thanks for this, looking good now

Comment 23

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/e1fb3fcc12f7517b7e76fde492cdbe9db700ce92