Closed
Bug 1304501
Opened 8 years ago
Closed 8 years ago
Properly disable trimUrl on autofill
Categories
(Toolkit :: Autocomplete, defect, P3)
Toolkit
Autocomplete
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
Details
(Whiteboard: [fxsearch])
Attachments
(1 file)
The one implemented in bug 711503 was mostly a workaround for lack of time. And it almost broke. To detect autoFill we were checking selectedIndex == -1... but now we HAVE an autoFill entry at selectedIndex == 0, that is also the normal behavior. It didn't break cause we fixed it in urlbarbindings checking the "autofill" style in the selected result. We can do better though, the controller always knows the reason it's setting the textValue, and can communicate it to the view. This is sort-of what Gaving suggested originally in bug 711503, but I don't plan to introduce another interface, I'll just add a setTextValueWithReason and ensure the controller invokes it, if available, or fallbacks to the old setter.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
Drew, what do you think of the approach? I didn't go with the usual nsIAutoCompleteInput2 trick to retain add-ons (setting textValue on gURLBar) compat.
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8793472 [details] Bug 1304501 - Properly disable trimUrl on autofill. https://reviewboard.mozilla.org/r/80206/#review79290 I like it!
Attachment #8793472 -
Flags: review?(adw) → review+
Assignee | ||
Comment 5•8 years ago
|
||
hm yeah, unfortunately a bunch of tests are failing, for some reasons. I suspect they are expecting trimmed uris, but we defaultcomplete, or we relied on the fact we didn't trim when selectedIndex was not set. browser/components/sessionstore/test/browser_522545.js | Address bar's value set after loading URI - Got http://example.com/, expected example.com browser/base/content/test/urlbar/browser_URLBarSetURI.js | URL bar had user/pass stripped initially - Got http://example.com/, expected example.com browser/base/content/test/general/browser_readerMode.js | gURLBar is displaying original article URL - Got about:reader... browser/base/content/test/urlbar/browser_bug304198.js | gURLBar.textValue should be testURL after initial switch to fullURLTab browser/base/content/test/urlbar/browser_urlbarEnter.js | Urlbar should preserve the value on return keypress - Got http://example.com/÷?÷, expected example.com/÷?÷ browser/base/content/test/urlbar/browser_urlbarOneOffs.js | Expected textValue - "http://example.com/browser_urlbarOneOffs.js/?9" == "example.com/browser_urlbarOneOffs.js/?9" I guess I'll look again at this next week and see the reason.
Assignee | ||
Comment 6•8 years ago
|
||
ops, I forgot to override the textValue getter as it was before! That's why it reads wrong in the tests.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
note the interdiff is broken cause I rebased this on top of bug 1306639. The only change is the addition of onBeforeTextValueGet that overrides the textValue getter as it was before my changes.
Assignee | ||
Comment 9•8 years ago
|
||
one failure to go: browser/base/content/test/urlbar/browser_urlbarDecode.js | The string displayed in the textbox should not be escaped - "example.com/アイウエオ" == "http://example.com/アイウエオ"
Assignee | ||
Comment 10•8 years ago
|
||
and I actually think the test is wrong, it is setting textValue directly, so it is expected to go through the trim function.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/c8201d8297d5 Properly disable trimUrl on autofill. r=adw
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c8201d8297d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•