481836 - URLbar autocomplete fills in nonexistant URL

Status: VERIFIED FIXED

(SeaMonkey :: Autocomplete, defect)

Description

[Rolf Bode-Meyer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.9.1b3pre) Gecko/20090304 SeaMonkey/2.0b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.9.1b3pre) Gecko/20090304 SeaMonkey/2.0b1pre

Ever since this new places thingy the location bar behaves different in a way I think it's a bug. Only it's hard to find a reproducable way, but at least one small annoyance I found that works everytime I try.

Reproducible: Always

Steps to Reproduce:
1. Enter the URL "josm.openstreetmap.de/latest"
2. Make a new tab and enter "o"

Actual Results:  
I get "osm.openstreetmap.de/latest" autofilled.

Expected Results:  
Anything starting with "o" might be filled in, but not one starting with "j".

Settings in Location Bar: Autocomplete from browsing history, Only match locatios, Only on word boundaries, Automatically prefill the best match.

Comment 1

[Frank Wein [:mcsmurf]]

I can confirm this with a SeaMonkey trunk (1.9.1) build. Not setting to NEW as this might be a toolkit bug, not sure yet. When typing in "o" with a Firefox trunk build it shows "josm.openstreetmap.de/latest" in the auto-complete dropdown, but does not autocomplete. When typing in "j", it does autocomplete. Tested in FF with browser.urlbar.matchBehavior set to 2 and browser.urlbar.autoFill set to true.

Comment 2

[Frank Wein [:mcsmurf]]

Ok, confirming, this is a bug in SeaMonkey.
Problem is we end up in the else block at http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/d40f92c60170/xpfe/components/autocomplete/resources/content/autocomplete.xml#l1142. This probably was correct with the old history code which could only match at the beginning of a URL, but with Places matching in the middle of a URL, too, this is wrong. One take a look at http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/d40f92c60170/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#l1370 (toolkit/ autocomplete) to basically see what they are doing to avoid this. We also need to take a look at completeDefaultIndex, Firefox sets this.completeDefaultIndex = this.autoFill.

Comment 3

[Frank Wein [:mcsmurf]]

Attachment: Possible patch

This patch adds an if-check to the else clause so that it only enters the else clause when the typed in text in the url bar matches the beginning of the host in resultValue (this is usually the first entry entry in the search results in the dropdown, see the code for more details). This code just assumes we want a URL with http:// if we autocomplete, so the code does not autocomplete when there is an ftp:// URL for example. Actually now that I think about it, we should do a real check that the protocol is http://, so that for example it does not match "ftp://sgoogle.example.com" when typing in "google".

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: Patch

Check if the scheme is actually http.

Comment 5

[[e-mail]]

Attachment: IIRC Firefox works something like this

Comment 6

[Frank Wein [:mcsmurf]]

Attachment: Updated patch

This is Neil's patch, updated to apply on the 1.9.1 branch

Comment 7

[Frank Wein [:mcsmurf]]

Comment on attachment 366146 [details] [diff] [review]
Updated patch

Oops, this patch breaks special URLs like about:config when Places found entries and added those to the autocomplete drop-down.

Comment 8

[Frank Wein [:mcsmurf]]

Places does this (http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#1082) to prevent entering the first entry in the drop-down.

Comment 9

[Frank Wein [:mcsmurf]]

Attachment: Updated patch

This fixes the about:config issue, too (it did also happen without the previous patch). The problem with special URLs like about:config was that it always assumed there was a default match to file in. This can for example happen when you have the URL "http://kb.mozillazine.org/Firefox_:_FAQs_:_About:config_Entries" in browser history, autofill turned on and type in about:config and press enter. Then SeaMonkey went to that page instead.

Comment 10

[Rolf Bode-Meyer]

Any news on this, what's keeping it from resolving?

Comment 11

[Frank Wein [:mcsmurf]]

Need to find some time to finish this patch, it probably needs a bit of tweaking. For example with this patch "http://www.tagesschau.de/WIRT" would match the URL "http://www.tagesschau.de/wirtschaft/leitwaehrung100.html". I don't think we want that, so we'll either do a case-sensitive search or only do a case-sensitive search in the host part of the URL.

Comment 12

[Frank Wein [:mcsmurf]]

Comment on attachment 366224 [details] [diff] [review]
Updated patch

This way is currently the best we can do (without doing some major code changes I think). The autocomplete code is also used within mail compose window and there for example when typing "BaR@ExAM" should match the entry "Foo Bar <[e-mail]>" (it does that in SeaMonkey 1.x). This code will for example then also match  http://www.foo.com/example2 when typing in  http://www.foo.com/EXAM

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: New patch

New patch, also simplifies the code a bit. For case sensitive autofill (like url bar) one needs to add an attribute casesensitive="true" to the xul element. By default it uses the old behaviour (case insensitive autofill).

Comment 14

[jag (Peter Annema)]

Comment on attachment 372712 [details] [diff] [review]
New patch

Let's use the attribute name "autofillmatchcase".

I generally like your patch (how could I not? ;-)) but how about this for more refactoring and clarity:

            if (indexToUse != -1) {
              var matchedValue = this.getSessionValueAt(aSessionName, indexToUse);
              var patternValue = this.currentSearchString;

              var caseSensitive = this.getAttribute("autofillmatchcase") == "true";
              var matched = caseSensitive ? matchedValue : matchedValue.toLowerCase();
              var pattern = caseSensitive ? patternValue : patternValue.toLowerCase();

              var fillValue;

              var patternIndex = matched.indexOf(pattern);
              if (patternIndex != 0 && this.completeDefaultIndex) {
                fillValue = " >> " + matchedValue;
              }
              else if (patternIndex >= 0) {
                // If the match is a URI then we only allow the prefix
                // if it would have been assumed by URI fixup anyway.
                var prefix = matched.substr(0, patternIndex);
                if (!/^[a-z][-+.0-9a-z]*:/.test(prefix) ||
                    prefix == (/^ftp\b/.test(pattern) ? "ftp://" : "http://")) {
                  fillValue = matchedValue.substr(patternIndex + patternValue.length);
                }
              }

              if (fillValue) {
                this.ignoreInputEvent = true;
                this.setTextValue(patternValue + fillValue);
                this.mInputElt.setSelectionRange(patternValue.length, this.value.length);
                this.mDefaultMatchFilled = true;
                this.ignoreInputEvent = false;
              }

              this.mNeedToComplete = true;
            }

Note: untested, but since it's mostly search+replace and refactoring, barring typos it should work just the same way.

Comment 15

[Frank Wein [:mcsmurf]]

Attachment: Patch with suggested changes

Comment 16

[jag (Peter Annema)]

Looks good! ;-)

I'd line the ? up over the
             : like so

Does it work?

Comment 17

[Frank Wein [:mcsmurf]]

(In reply to comment #16)
> Does it work?

Yes, I tested with the URL bar and MailNews address book autocomplete.

Comment 18

[jag (Peter Annema)]

Well, you have r=jag on your changes. r=you on mine? :-)

Comment 19

[Frank Wein [:mcsmurf]]

r=me :)

Comment 20

[[e-mail]]

Comment on attachment 375419 [details] [diff] [review]
Patch with suggested changes

>+                if (!/^[a-z][-+.0-9a-z]*:/.test(prefix) ||
Hmm... should this be testing matched rather than prefix?

Comment 21

[[e-mail]]

Are we just adding case sensitive autofill as a bonus?

Comment 22

[Frank Wein [:mcsmurf]]

(In reply to comment #21)
> Are we just adding case sensitive autofill as a bonus?

Places also returns "http://www.google.com" as history autocomplete entry when you type in "http://www.GOOgle". SeaMonkey 1.x did not do this.

Comment 23

[Frank Wein [:mcsmurf]]

btw: I know it does not matter for the host part, but for the path after it. Separating those two cases seemed to be too difficult and a bit of overkill.

Comment 24

[jag (Peter Annema)]

Ah, the patch doesn't include the changes to suite where we add autofillmatchcase="true" to the URL bar (and location dialog?) autocompletes.

So sorta a bonus, yeah ;-)

As for prefix vs. matched, I guess the question is what happens if the user types "tp://go". That can't exactly auto-fill, since the "ht" or "f" is missing from the start. Testing the regexp on matched (e.g. http://google.com or ftp://goods.net) would succeed, the not would take us into the or part of the expression which would then fail. Testing on prefix fails, and the not takes us into the auto-fill code.

So yeah, it should be matched, not prefix.

Comment 25

[[e-mail]]

Comment on attachment 375419 [details] [diff] [review]
Patch with suggested changes

>+                if (!/^[a-z][-+.0-9a-z]*:/.test(prefix) ||
>+                    prefix == (/^ftp\b/.test(pattern) ? "ftp://" : "http://")) {
So do these tests need to be case insensitive too?

Comment 26

[jag (Peter Annema)]

Erh, yeah.

Comment 27

[[e-mail]]

Comment on attachment 375419 [details] [diff] [review]
Patch with suggested changes

With the patch as is, "p" tried to complete to "p://planet.mozilla.org" :-(

Comment 28

[[e-mail]]

Attachment: Alternative idea

Sigh. I just realised that this won't fix the open location dialog...

Comment 29

[Frank Wein [:mcsmurf]]

Attachment: Patch

Ok, this replaces "prefix" with "matched" and so fixes the "p" issue mentioned. Comment 25 does not really apply as far as I can tell as History always stores the protocol as lowercase.

Comment 30

[[e-mail]]

Ah, but now p never completes at all, because it always matches the "p" in "http", and therefore never the "p" in "planet.mozilla.org" (or whatever).

Comment 31

[Frank Wein [:mcsmurf]]

Gah...help?

Comment 32

[jag (Peter Annema)]

Hrm, I guess I wasn't thinking out loud this one time. You could strip the scheme from "matched", solve things that way ;-)

Comment 33

[Rolf Bode-Meyer]

Requesting blocker.

Comment 34

[Frank Wein [:mcsmurf]]

Comment on attachment 376508 [details] [diff] [review]
Alternative idea

Neil, could we maybe take this patch in the meantime, so that at least the URL bar works fine? My approach seems to be a bit difficult to fix so that it works fine in all cases.

Comment 35

[[e-mail]]

Comment on attachment 376508 [details] [diff] [review]
Alternative idea

Wait, isn't this *my* patch? Also, we apparently never auto fill in the open location dialog, so that issue fortunately never arises.

Comment 36

[Frank Wein [:mcsmurf]]

Comment on attachment 376508 [details] [diff] [review]
Alternative idea

r+ with this
+ var match = result.toLowerCase();
+  var entry = this.value.toLowerCase();

changed to case-sensitive, otherwise the problem mentioned in Comment 22 occurs again.

Comment 37

[[e-mail]]

Pushed changeset 092911915d60 to comm-central.

Comment 38

[Rolf Bode-Meyer]

Works great for me and seems to have also fixed problem described in bug 481843.

Comment 39

[Frank Wein [:mcsmurf]]

Comment on attachment 377266 [details] [diff] [review]
Patch

Canceling obsolete review request as the patch does not work fine in every case.