Closed Bug 481836 Opened 11 years ago Closed 11 years ago

URLbar autocomplete fills in nonexistant URL

Categories

(SeaMonkey :: Autocomplete, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.0b2

People

(Reporter: robome, Assigned: mcsmurf)

Details

Attachments

(3 files, 6 obsolete files)

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.
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Attached patch Possible patch (obsolete) — Splinter Review
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".
Attached patch PatchSplinter Review
Check if the scheme is actually http.
Attachment #366115 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
This is Neil's patch, updated to apply on the 1.9.1 branch
Attachment #366138 - Attachment is obsolete: true
Attachment #366146 - Attachment is patch: true
Attachment #366146 - Attachment mime type: application/octet-stream → text/plain
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.
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.
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #366146 - Attachment is obsolete: true
Any news on this, what's keeping it from resolving?
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 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 <bar@example.com>" (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
Attachment #366224 - Flags: review?(jag)
Attached patch New patch (obsolete) — Splinter Review
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).
Attachment #366224 - Attachment is obsolete: true
Attachment #372712 - Flags: review?(jag)
Attachment #366224 - Flags: review?(jag)
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.
Attachment #372712 - Flags: review?(jag) → review-
Attached patch Patch with suggested changes (obsolete) — Splinter Review
Attachment #372712 - Attachment is obsolete: true
Attachment #375419 - Flags: review?(jag)
Looks good! ;-)

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

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

Yes, I tested with the URL bar and MailNews address book autocomplete.
Well, you have r=jag on your changes. r=you on mine? :-)
r=me :)
Attachment #375419 - Flags: superreview?(neil)
Attachment #375419 - Flags: review?(jag)
Attachment #375419 - Flags: review+
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?
Are we just adding case sensitive autofill as a bonus?
(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.
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.
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 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?
Erh, yeah.
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" :-(
Attachment #375419 - Flags: superreview?(neil) → superreview-
Attached patch Alternative ideaSplinter Review
Sigh. I just realised that this won't fix the open location dialog...
Attached patch PatchSplinter Review
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.
Attachment #375419 - Attachment is obsolete: true
Attachment #377266 - Flags: superreview?(neil)
Attachment #377266 - Flags: review+
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).
Gah...help?
Hrm, I guess I wasn't thinking out loud this one time. You could strip the scheme from "matched", solve things that way ;-)
Requesting blocker.
Flags: blocking-seamonkey2.0b2?
Attachment #376508 - Flags: review?(neil)
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 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.
Attachment #376508 - Flags: review?(neil) → review?(bugzilla)
Flags: blocking-seamonkey2.0b2? → blocking-seamonkey2.0b2+
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.
Attachment #376508 - Flags: review?(bugzilla) → review+
Pushed changeset 092911915d60 to comm-central.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b2
Assignee: nobody → bugzilla
Works great for me and seems to have also fixed problem described in bug 481843.
Status: RESOLVED → VERIFIED
Attachment #377266 - Flags: superreview?(neil)
Comment on attachment 377266 [details] [diff] [review]
Patch

Canceling obsolete review request as the patch does not work fine in every case.
You need to log in before you can comment on or make changes to this bug.