Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Don't trim url when inline autocomplete inserts text

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Autocomplete
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 582292 [details] [diff] [review]
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.
Attachment #582292 - Flags: review?(dao)
(Assignee)

Comment 1

6 years ago
hm, seems to make b-c tests unhappy :(
(Assignee)

Comment 2

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

clearing request, but still valid as an helpwanted request!
Attachment #582292 - Flags: review?(dao)
(Assignee)

Comment 3

6 years ago
Created attachment 582394 [details] [diff] [review]
patch v2.0

Another approach, that seems to pass tests
(Assignee)

Comment 4

6 years ago
note this is the only blocking issue preventing inline autocomplete from landing
(Assignee)

Comment 5

6 years ago
Created attachment 582408 [details] [diff] [review]
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.
Attachment #582292 - Attachment is obsolete: true
Attachment #582394 - Attachment is obsolete: true
Attachment #582408 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

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

Comment 7

6 years ago
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.
(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.
(Assignee)

Comment 9

6 years ago
(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.
(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.
(Assignee)

Comment 11

6 years ago
(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.
> > > 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.
(Assignee)

Comment 13

6 years ago
(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.
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.
Attachment #582408 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 15

6 years ago
Created attachment 583766 [details] [diff] [review]
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.
Attachment #583766 - Flags: feedback?(gavin.sharp)

Updated

6 years ago
Keywords: addon-compat, dev-doc-needed
(Assignee)

Comment 16

6 years ago
Created attachment 584059 [details] [diff] [review]
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.
Attachment #583766 - Attachment is obsolete: true
Attachment #583766 - Flags: feedback?(gavin.sharp)
Attachment #584059 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

6 years ago
Created attachment 585606 [details] [diff] [review]
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.
Attachment #585606 - Flags: review?(gavin.sharp)
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).
Attachment #582408 - Flags: review- → review+
Attachment #584059 - Flags: review?(gavin.sharp)
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.)?
Attachment #585606 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

6 years ago
Created attachment 588189 [details] [diff] [review]
patch v2.2

just changing to _disableTrim
Attachment #582408 - Attachment is obsolete: true
Attachment #584059 - Attachment is obsolete: true
Attachment #585606 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 588190 [details] [diff] [review]
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.
Attachment #588190 - Flags: review?(gavin.sharp)
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.
Attachment #588190 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 23

6 years ago
addressed comment and coalesced patches
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d06b0b645b5
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/2d06b0b645b5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
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.
Keywords: addon-compat, dev-doc-needed
Blocks: 665580
Blocks: 758917

Updated

5 years ago
No longer blocks: 665580
You need to log in before you can comment on or make changes to this bug.