711503 - Don't trim url when inline autocomplete inserts text

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0

As soon as I type http://, it gets replaced by the inline content, with fancy results.
This patch solves the problem afaict, I tested it with both autofill enabled and disabled, when tabbing to results they are correctly trimmed, while the inline value is never messed up.

Comment 1

[Marco Bonardo [:mak]]

hm, seems to make b-c tests unhappy :(

Comment 2

[Marco Bonardo [:mak]]

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

clearing request, but still valid as an helpwanted request!

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v2.0

Another approach, that seems to pass tests

Comment 4

[Marco Bonardo [:mak]]

note this is the only blocking issue preventing inline autocomplete from landing

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v2.1

Added comments to clarify the condition.

Gavin suggested we may make a nsIAutocompleteInput2 with a setTextValue(aValue, aReason). That sounds like a possibility, though it looks a bit scarier at this stage.

Comment 6

[Marco Bonardo [:mak]]

this is building on try (apart the comments) to ensure all tests are happy
https://tbpl.mozilla.org/?tree=Try&rev=599c70126cb9

Comment 7

[Marco Bonardo [:mak]]

try results were pretty good, I'm practically ready to land the feature, once this bug is reviewed. So, please let me know what I should do to make that happen.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #5)
> Gavin suggested we may make a nsIAutocompleteInput2 with a
> setTextValue(aValue, aReason). That sounds like a possibility, though it
> looks a bit scarier at this stage.

Err, "at this stage"? Are you going to push bug 566489 before the upcoming migration to aurora? This sounds like a bad plan to me.

Comment 9

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Err, "at this stage"? Are you going to push bug 566489 before the upcoming
> migration to aurora? This sounds like a bad plan to me.

That's the plan, yes, and I don't see anything bad in it. The feature has been tested on ux branch for months, it replaces a much similar, but bogus, existing feature, and can be disabled flipping a pref. Ideally, I would have liked to land on Friday, but I was unable to get this review in time. Delaying to the next merge wouldn't bring interesting gain.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Err, "at this stage"? Are you going to push bug 566489 before the upcoming
> > migration to aurora? This sounds like a bad plan to me.
> 
> That's the plan, yes, and I don't see anything bad in it. The feature has
> been tested on ux branch for months,

... which didn't lead to bugs like this being fixed.

> it replaces a much similar, but bogus,
> existing feature,

Hidden and disabled.

> and can be disabled flipping a pref.

Except that modifications like the one in this bug can't be disabled.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #10)
> ... which didn't lead to bugs like this being fixed.

UX, as well as Nightly, has some technical audience that is unlikely to type http:// in the locationbar. I never do, thus I would have never noticed this issue.

> > it replaces a much similar, but bogus,
> > existing feature,
> 
> Hidden and disabled.

That's the point, if we eventually disable the new one by default it will be hidden as well.

> > and can be disabled flipping a pref.
> 
> Except that modifications like the one in this bug can't be disabled.

I could say the same about 90% of the fixes that landed in the last week.

Btw, at this point, I suppose we missed another train, just let me know if I should make the new interface so we can land as soon as we reopen for 12.

Comment 12

[Dão Gottwald [:dao]]

> > > it replaces a much similar, but bogus,
> > > existing feature,
> > 
> > Hidden and disabled.
> 
> That's the point, if we eventually disable the new one by default it will be
> hidden as well.

Your point here seemed to be that "it replaces a much similar, but bogus, existing feature". I don't see how this would be relevant in a positive sense. The extent to which the existing feature is bogus makes me only all the more worried.

As for disabling the new implementation, I commented on that below.

> > > and can be disabled flipping a pref.
> > 
> > Except that modifications like the one in this bug can't be disabled.
> 
> I could say the same about 90% of the fixes that landed in the last week.

Hopefully most of these fixes won't be about features with a similar track record of weird bugs.

> Btw, at this point, I suppose we missed another train, just let me know if I
> should make the new interface so we can land as soon as we reopen for 12.

I may be wrong, but presumably the API addition would make this code simpler and less error prone, so it's probably preferable.

Comment 13

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #12)
> > Btw, at this point, I suppose we missed another train, just let me know if I
> > should make the new interface so we can land as soon as we reopen for 12.
> 
> I may be wrong, but presumably the API addition would make this code simpler
> and less error prone, so it's probably preferable.

afaict, it would just avoid checking this.popup.selectedIndex in the textValue setter, since the new setter would have a reason argument. If I correctly got Gavin's idea, at least.

Comment 14

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

It's difficult to determine whether this patch would negatively affect any other setText callers. That's why I suggested the API change; I think we should probably just do that at this point.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v3.0

I still have to work on tests, though I think it's worth to start a feedback turn before going deep into code. As I anticipated you yesterday on IRC, this is a breaking change, since any binding overriding "textValue" and expecting controller to call into it, won't work. Though, as you said, if the change is good we should not be afraid of it.

note that I originally wanted to use REASON_* constants, but looks like Accessibility includes some header that defines already similar constants (at least reason_other, but future changes may conflict), so I decided to namespace them more with a TEXTVALUE_ prefix.

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v3.1

this includes a test for each code path hitting setTextValue.

we may probably still want a b-c test checking we don't regress trimming http:// for autocomplete, though that's not yet ready and can be a separate patch.

Comment 17

[Marco Bonardo [:mak]]

Attachment: bc test

this is a really trivial test that just checks the browser override respects setTextValueFor. A more complicated one may be written, though I'm not sure which level of testing we want to simulate.

Comment 18

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

Comment on attachment 582408 [details] [diff] [review]
patch v2.1

Sorry Marco - looking at this more, I think I've changed my mind. Changing nsIAutocompleteInput is kind of a rats nest, and this patch shouldn't have any negative effects on existing SetTextValue callers (I audited the ones in nsAutocompleteController) - worst case we forget to trim an autocomplete value, and if that happens we'll likely end up trimming it when it gets updated at the end of page load.

So I think this is the way to go for now, at least until we have some more time to fully re-think autocomplete (I found a couple bugs just looking at this code!).

One nit: _disableTrim, since this isn't a property that can usefully be set by third parties (we clobber it when setTextValue is called).

Comment 19

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

Comment on attachment 585606 [details] [diff] [review]
bc test

>diff --git a/browser/base/content/test/browser_urlbarAutoFillTrimURLs.js b/browser/base/content/test/browser_urlbarAutoFillTrimURLs.js

>+function test() {

>+  gBrowser.selectedBrowser.addEventListener("load", function () {
>+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

You probably just copied this, but for new tests it'd be best to avoid arguments.callee and just name the anonymous function so you can refer to it by name.

I guess you'll have to rework this test too. Can we just test the symptom (trigger autocomplete, check that http:// isn't clobbered, etc.)?

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v2.2

just changing to _disableTrim

Comment 21

[Marco Bonardo [:mak]]

Attachment: test

I hope this is more similar to what you wanted as a test. the checked values are based on new inline behavior, though if I'd do the opposite I should update it again when the other patches land, so I'd prefer to just land this with all the rest.

Comment 22

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

Comment on attachment 588190 [details] [diff] [review]
test

>diff --git a/browser/base/content/test/browser_urlbarAutoFillTrimURLs.js b/browser/base/content/test/browser_urlbarAutoFillTrimURLs.js

>+function waitForSearchComplete(aCallback) {

>+  let onSearchComplete = gURLBar.onSearchComplete.bind(gURLBar);

You shouldn't need this bind(), right? Just use .apply() below when you invoke it, to avoid modifying the code for the rest of the test run.

Comment 23

[Marco Bonardo [:mak]]

addressed comment and coalesced patches
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d06b0b645b5

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/2d06b0b645b5

Comment 25

[Florian Scholz (Open Web Docs)]

I'm pretty sure dev-doc-needed and addon-compat were set for changes in nsIAutocompleteInput in the now obsolete patches.

Please re-add if there is still anything that needs documentation here.