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
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: Gavin, Assigned: mak)
References
Details
Attachments
(1 file, 2 obsolete files)
7.67 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #591471 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite+
Reporter | ||
Comment 3•9 years ago
|
||
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()?
Attachment #591471 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
nevermind, nsIURI only lowercases the domain, I'll see how to handle that better.
Assignee | ||
Comment 6•9 years ago
|
||
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
Target Milestone: --- → mozilla12
Assignee | ||
Comment 7•9 years ago
|
||
backed out, for some fancy reason this fails browser_urlbarAutoFillTrimURLs.js
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #591471 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/572cfaeede5c
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/572cfaeede5c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•