1172937 - Action row doesn't always update correctly with unified autocomplete

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

STR:

- navigate to google.com so that it's in your history
- open a new tab
- type »goo« into the awesomebar (should autocomplete to google.com)
- press backspace so that the location bar content only says »goo«

Expected result:
- Action row should say »goo - Search with Yahoo«
- Pressing enter should search for »goo«

Actual result:
- Action row still says »Visit google.com«
- Pressing enter does nothing
- Typing any other character after pressing enter searches for the wrong term

Comment 1

[Marco Bonardo [:mak]]

I was sure this was a dupe, but I couldn't find it so far.

It might be related to our long story with broken backspace.

Comment 2

[Marco Bonardo [:mak]]

I said p2, but considered how annoying might be for the user to not understand why the locationbar doesn't work, it might well be a P1.
We likely need to assign some resources to work on this.

Comment 3

[Dave Townsend [:mossop]]

Felipe is going to take a look and see if he can help here

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: actionrow.diff

Posting current progress to the bug. This fixes the problem. It needs some cleanup, and to figure out a way to limit the new behavior of the autocomplete controller to this consumer

Comment 5

[Marco Bonardo [:mak]]

Felipe, I guess this should be reassigned, do you have an updated wip by chance?

Comment 6

[Marco Bonardo [:mak]]

let's put this back into the backlog, we have the wip patch in comment 4 to continue the work.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Sorry for the delay, Marco! I do have a final patch ready for review :)  I didn't have enough time to write a test for it but if you think that's fine I will write one when I get back.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Retrigger search (with no autofill), when autofilled value was backspaced

Comment 9

[Marco Bonardo [:mak]]

Attachment: Review in form of patch

This addresses my comments on Felipe's patch, in form of an updated patch.
I mostly tried to reduce the impact on third party consumers, and made the names more explicative.
I also added a fix for DEL (Felipe's patch only worked with backspace), and added a mochitest-browser test covering both kind of removal.

Drew, I will be away tomorrow and probably part of Monday, would you mind giving a second look and testing to this? Since my changes are not trivial it's better to have a second opinion.

Note I applied and tested this on top of an unbitrotted version of your patch for bug 1176437, to be sure there was no bad interaction with it, I don't know if it applies cleanly to current fx-team.

Comment 10

[Marco Bonardo [:mak]]

ni? myself just for tracking purposes.

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 8631819 [details] [diff] [review]
Review in form of patch

Review of attachment 8631819 [details] [diff] [review]:
-----------------------------------------------------------------

Well, the patch looks fine to me and it works as advertised -- but only the first time you hit delete/backspace.  If you delete/backspace again, then the first result turns into "Visit google.com" again.  Do you see that too?

1. Visit google.com
2. Type goo so that google.com is autocompleted
3. Hit delete/backspace, see that first result becomes a "Search with"
4. Hit delete/backspace again, see that first result becomes "Visit google.com"

::: browser/base/content/test/general/browser_urlbar_autoFill_backspaced.js
@@ +14,5 @@
> +  yield promiseSearchComplete();
> +
> +  ok(gURLBar.popup.richlistbox.children.length > 0, "Should get at least 1 result");
> +  let result = gURLBar.popup.richlistbox.children[0];
> +  is(result.getAttribute("type"), "action " + action, "Expect right type attribute");

Wonder if this check might be less likely to break in the face of future changes with something like:

let types = result.getAttribute("type").split(/\s+/);
ok(types.includes(...));

::: browser/base/content/urlbarBindings.xml
@@ +902,5 @@
>            if (Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete") &&
>                this.popup.selectedIndex == 0) {
> +            // The value has not been updated yet, so we need to update it
> +            // manually before invoking handleText().
> +            this.value = this.textValue.substring(0, this.selectionStart) +

Crummy to have to do this manually... I'd almost prefer a setTimeout(() => this.mController.handleText()).

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +237,5 @@
> +  bool searchAgainOnAutoFillClear = mUserClearedAutoFill && mClearingAutoFillSearchesAgain;
> +
> +  if (!handlingCompositionCommit &&
> +      !searchAgainOnAutoFillClear &&
> +      newValue.Length() > 0 && newValue.Equals(mSearchString) ) {

newValue.Length() > 0 && newValue.Equals(mSearchString)

is redundant in the conditional since that expression is also part of mUserClearedAutoFill.  I don't know this code well enough to know whether you should update the conditional or update mUserClearedAutoFill, but it seems like mUserClearedAutoFill is independent of mSearchString and so should just be:

mUserClearedAutoFill = Substring(mPlaceholderCompletionString, 0, newValue.Length()).Equals(newValue); // && newValue.Length() > 0?

Maybe there should be a newValue length check too.  But I could be wrong.

::: toolkit/components/places/UnifiedComplete.js
@@ +1668,5 @@
>      // Note: We don't use previousResult to make sure ordering of results are
>      //       consistent.  See bug 412730 for more details.
>  
>      this._currentSearch = new Search(searchString, searchParam, listener,
> +                                     this, this, this._previousSearchString);

IMO you should pass `searchString == this._previousSearchString` as a prohibitAutofill or something argument.  The Search object ends up making that same comparison right?  And having one Search be aware of what the previous Search's search string was isn't great.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8631819 [details] [diff] [review]
Review in form of patch

Review of attachment 8631819 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review request for now.

Comment 13

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #11)
> ::: browser/base/content/urlbarBindings.xml
> @@ +902,5 @@
> >            if (Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete") &&
> >                this.popup.selectedIndex == 0) {
> > +            // The value has not been updated yet, so we need to update it
> > +            // manually before invoking handleText().
> > +            this.value = this.textValue.substring(0, this.selectionStart) +
> 
> Crummy to have to do this manually... I'd almost prefer a setTimeout(() =>
> this.mController.handleText()).

setTimeout it's not enough, cause something must update mSearchString yet... that's why I'm setting value. I updated the comment.

>  but it
> seems like mUserClearedAutoFill is independent of mSearchString

Actually it's not completely. Substring(mPlaceholderCompletionString, 0,  newValue.Length()).Equals(newValue) tells us that the current input value is contained in the placeholder, but that's also true for newV[alue], while we're trying to figure if a part of it has been removed.
But actually we can relax this by just checking lengths, that might also fix the second backspace issue. I'll see what I can do.

Comment 14

[Marco Bonardo [:mak]]

So I've fixed all the comments, but there's still one problem, basically we need to know when the user starts a new autocomplete task, cause after we autofilled "mo[zilla.com]" and then we are requested to search "m", we need to know if that search is still the user editing the same search (prohibit autofill), or if he opened a new tab and typed "m" (autofill).

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v2

still one edge case to figure out

1. type "m" let it fill to "m[ozilla.org]"
2. blur the urlbar clicking on something else
3. select "ozilla.org" so that only m is left out and backspace or delete
4. we don't autofill, but we suggest "Visit mozilla.org"

Comment 16

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #15)
> still one edge case to figure out
> 
> 1. type "m" let it fill to "m[ozilla.org]"
> 2. blur the urlbar clicking on something else
> 3. select "ozilla.org" so that only m is left out and backspace or delete
> 4. we don't autofill, but we suggest "Visit mozilla.org"

After investigation, I figured that to fix this properly (don't autofill) we'd need a way for urlbarbindings to communicate to the controller the difference between addition and removal of letters (something like handleBackspace()), and then a way for the controller to communicate to the search whether it can autofill or not. This looks like too much risk for the edge case here, since would change behavior for all autocomplete consumers.
So I'm going to fix this the "wrong" side, we will autofill regardless. At least the input field content will be in sync with the action row and we won't surprise the user.

Comment 17

[Marco Bonardo [:mak]]

Attachment: patch v3

From my tests this works pretty well, and it's limited to unified complete.

Comment 18

[Marco Bonardo [:mak]]

this causes a failure in toolkit/content/tests/chrome/test_autocomplete_placehold_last_complete.xul I'm looking into that.

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8636630 [details] [diff] [review]
patch v3

I have a better version incoming.

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v4

This is less hackish than the previous version, most of the doubtful changes have gone, and I also fixed the edge-case I found above.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v4.1

minor unbitrot (applies on top of bug 1179153 in my queue, but likely they don't conflict)

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 8639759 [details] [diff] [review]
patch v4.1

Review of attachment 8639759 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, works well.  Sorry for the delay.

Comment 23

[Marco Bonardo [:mak]]

unfortunately, even if the patch is almost a no-op for existing consumers, there are 2 failures:
toolkit/content/tests/chrome/test_autocomplete_with_composition_on_input.html
toolkit/components/satchel/test/test_form_autocomplete.html
I'm looking into those...

Comment 24

[Marco Bonardo [:mak]]

ugh, after I spent time trying to figure which of the conditions was failing... I ended up figuring out the problem is FormFill doesn't like multiple searchParams, it only wants the input element id there.
So the fix is a boring check for mClearingAutoFillSearchesAgain before touching searchParam.

Comment 25

[Marco Bonardo [:mak]]

Attachment: patch v4.2

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/91b6fe481685

Comment 27

[Marco Bonardo [:mak]]

Ryan backed this out due to failures still existing in
toolkit/content/tests/chrome/test_autocomplete_with_composition_on_input.html
The satchel failure is fixed instead, strange cause I ran both tests before landing, likely my tree was in a fuzzy state.
https://hg.mozilla.org/integration/fx-team/rev/39fc38df750d

Comment 28

[Marco Bonardo [:mak]]

The problem is that those failures are only reproducible on Tinderboxes, I cannot reproduce any of them locally.

So far I have reduced the failures to:

Testing on HTML input (asynchronously search), canceled compositionend should seach the result with the latest value: popupOpen - got false, expected true
Testing on HTML input (synchronously search), canceled compositionend should seach the result with the latest value: popupOpen - got false, expected true

This test seems to synthesize an Escape compositionend event:

synthesizeComposition({ type: "compositioncommitasis",
                        key: { key: "KEY_Escape", code: "Escape" } }, aWindow);

I tried installing the japanese ime and trying to escape when the ime popup appears, but I don't see any difference with or without the patch.

Comment 29

[Marco Bonardo [:mak]]

Masayuki-san, could you please help me figuring out the failure in comment 28?
How can I reproduce locally what the test is doing, I installed the japanese IME and tried using some autocomplete fields but I can't reproduce the test behavior.
Any idea why is it failing on Tinderbox but not locally?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not sure what platform do you use, though, if you use Windows, typing "a" twice and "Escape" once is same as the test.

The test is same as:

1. Type "Mozi" (without IME)
2. Select "zi" with Shift + ArrowLeft
3. Type "zi" with IME but cancel the composition string.

Then, the result should be found by only "Mo" (i.e., popup should be reopened).

Comment 31

[Marco Bonardo [:mak]]

Attachment: additional fix

I think I figured it out and it took far too much time!

The fact is (I suspect for formfill performance reasons) the controller is actually used as a service, as such local properties are basically passed from one input field to another, and the only filter in the middle is a setInput() call.
This is a sad consequence of having the same code driving "everything" and it's quite error prone. At least this code should build a disposable state object for each input field and replace it as a whole, instead of resetting tens of local properties :(

Basically I was "leaking" mClearingAutoFillSearchesAgain across input fields, from the awesomebar to the formfill test input field. I couldn't reproduce locally because the bug existed only when using the form field after the awesomebar, like "oth" does.

The other changes are mostly improving the test and making variable names more readable.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8369c1bd527

Comment 32

[Marco Bonardo [:mak]]

(you can ignore the X failures, that actually come from fx-team)

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/397afd7d1e6b

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/397afd7d1e6b

Comment 35

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Nightly 42.0a1 2015-08-09 under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OSX 10.9.5.