Closed Bug 1304501 Opened 3 years ago Closed 3 years ago

Properly disable trimUrl on autofill

Categories

(Toolkit :: Autocomplete, defect, P3)

defect

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.
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 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+
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.
ops, I forgot to override the textValue getter as it was before! That's why it reads wrong in the tests.
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.
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/アイウエオ"
and I actually think the test is wrong, it is setting textValue directly, so it is expected to go through the trim function.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c8201d8297d5
Properly disable trimUrl on autofill. r=adw
https://hg.mozilla.org/mozilla-central/rev/c8201d8297d5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.