Closed Bug 720501 Opened 9 years ago Closed 9 years ago

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


(Toolkit :: Places, defect)

Not set





(Reporter: Gavin, Assigned: mak)




(1 file, 2 obsolete files)

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.
Blocks: 566489
STR, for clarity:
0) Make sure browser.urlbar.autoFill is enabled
1) Have an autocomplete match for
2) enter SOME in the URL bar (we will autocomplete
3) Press right arrow to "select" the autocompletion and close the popup

Expected: "" is entered in the URL bar.
Actual: "" is entered in the URL bar.
Assignee: nobody → mak77
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #591471 - Flags: review?(
Flags: in-testsuite+
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("") 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()?
Attachment #591471 - Flags: review?( → review+
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.
nevermind, nsIURI only lowercases the domain, I'll see how to handle that better.
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.
Target Milestone: --- → mozilla12
backed out, for some fancy reason this fails browser_urlbarAutoFillTrimURLs.js
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 rather than, 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.
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #591471 - Attachment is obsolete: true
Attached patch patch v1.2Splinter Review
qrefed, too
Attachment #591747 - Attachment is obsolete: true
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.