720501 - urlInlineComplete should not attempt to case-preserve results, since that interferes with the controller's case-preservation

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

It's the controller's job to make sure that case is preserved while typing, but then the underlying case from the autocomplete result is used when the result is "accepted" (either by selection in the popup, or by using an arrow key after autofilling). If the search does its own case preserving, the controller can't restore the proper case.

urlInlineComplete's handleResult() should not try to preserve the case of its results by appending them to this._currentSearchString.

Comment 1

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

STR, for clarity:
0) Make sure browser.urlbar.autoFill is enabled
1) Have an autocomplete match for somedomain.com
2) enter SOME in the URL bar (we will autocomplete domain.com)
3) Press right arrow to "select" the autocompletion and close the popup

Expected: "somedomain.com/" is entered in the URL bar.
Actual: "SOMEdomain.com/" is entered in the URL bar.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 3

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

Comment on attachment 591471 [details] [diff] [review]
patch v1.0

>diff --git a/toolkit/components/places/tests/inline/head_autocomplete.js b/toolkit/components/places/tests/inline/head_autocomplete.js

>+    // Now force completion and check correct casing of the result.
>+    // This ensures the controller is able to do its magic case-preserving
>+    // stuff and correct replacement of the user's casing with result's one.
>+    controller.handleEnter(false);
>+    do_check_eq(input.textValue, aExpectedValue.toLowerCase());

Hmm, this kind of assumes that the underlying data being autocompleted is all-lowercase, and nothing really enforces that (someone could call addBookmark("http://Mozilla.org") in their setup function, and then I think this check would fail). I guess you could add an additional optional param like "aActualValue" and use that for the case-checking tests, instead of aExpectedValue.toLowerCase()?

Comment 4

[Marco Bonardo [:mak]]

as stated on IRC, all Places uris go through nsIURI, so using toLowercase() is a valid shortcut for the test, since nsIURI lower-cases the url. I'll add a comment about that.

Comment 5

[Marco Bonardo [:mak]]

nevermind, nsIURI only lowercases the domain, I'll see how to handle that better.

Comment 6

[Marco Bonardo [:mak]]

I decided to use a variant-like argument, if string will keep doing what it was, if object it has botg autoFilled and completed values.
https://hg.mozilla.org/integration/mozilla-inbound/rev/691af0af89da

Comment 7

[Marco Bonardo [:mak]]

backed out, for some fancy reason this fails browser_urlbarAutoFillTrimURLs.js

Comment 8

[Marco Bonardo [:mak]]

hm, I figured it out, we strip the protocol and www for the search and I have to add it back when returning the result. I added 4 additional tests to catch this. Luckily the domain and the protocol are lowercased by nsIURI so adding them back lowercased is fine.

Though, while testing this I found another bug, typing http://A suggests http://arewefastyet.com/ rather than http://Arewefastyet.com/, removing the http everything works fine. I initially suspected trimURL but disabling it doesn't help. I can also reproduce in current Nightly, so I'll file this apart.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.2

qrefed, too

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/572cfaeede5c

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/572cfaeede5c