Closed Bug 1172937 Opened 9 years ago Closed 9 years ago

Action row doesn't always update correctly with unified autocomplete

Categories

(Firefox :: Address Bar, defect, P1)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- verified

People

(Reporter: phlsa, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [unifiedautocomplete][fxsearch])

Attachments

(2 files, 7 obsolete files)

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
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.
Points: --- → 5
Flags: qe-verify+
Flags: firefox-backlog+
Priority: -- → P2
Whiteboard: [unifiedautocomplete][fxsearch]
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.
Flags: needinfo?(dtownsend)
Rank: 4
Keywords: regression
Priority: P2 → P1
Felipe is going to take a look and see if he can help here
Assignee: nobody → felipc
Flags: needinfo?(dtownsend)
Blocks: 1173202
Attached patch actionrow.diff (obsolete) — Splinter Review
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
Felipe, I guess this should be reassigned, do you have an updated wip by chance?
Flags: needinfo?(felipc)
let's put this back into the backlog, we have the wip patch in comment 4 to continue the work.
Assignee: felipc → nobody
Flags: needinfo?(felipc)
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.
Assignee: nobody → felipc
Attached patch Review in form of patch (obsolete) — Splinter Review
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.
Attachment #8630865 - Attachment is obsolete: true
Attachment #8630865 - Flags: review?(mak77)
Attachment #8631819 - Flags: review?(adw)
ni? myself just for tracking purposes.
Flags: needinfo?(mak77)
Blocks: 1054914
Iteration: --- → 42.2 - Jul 27
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.
Blocks: 1183921
Comment on attachment 8631819 [details] [diff] [review]
Review in form of patch

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

Clearing review request for now.
Attachment #8631819 - Flags: review?(adw)
Assignee: felipc → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
(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.
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).
Attached patch patch v2 (obsolete) — Splinter Review
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"
Attachment #8631819 - Attachment is obsolete: true
(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.
Attached patch patch v3 (obsolete) — Splinter Review
From my tests this works pretty well, and it's limited to unified complete.
Attachment #8635377 - Attachment is obsolete: true
Attachment #8636630 - Flags: review?(adw)
QA Contact: petruta.rasa
this causes a failure in toolkit/content/tests/chrome/test_autocomplete_placehold_last_complete.xul I'm looking into that.
Comment on attachment 8636630 [details] [diff] [review]
patch v3

I have a better version incoming.
Attachment #8636630 - Attachment is obsolete: true
Attachment #8636630 - Flags: review?(adw)
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #8638887 - Flags: review?(adw)
Blocks: 1187753
Attached patch patch v4.1 (obsolete) — Splinter Review
minor unbitrot (applies on top of bug 1179153 in my queue, but likely they don't conflict)
Attachment #8638887 - Attachment is obsolete: true
Attachment #8638887 - Flags: review?(adw)
Attachment #8639759 - Flags: review?(adw)
Blocks: 1186393
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.
Attachment #8639759 - Flags: review?(adw) → review+
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...
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.
Attached patch patch v4.2Splinter Review
Attachment #8639759 - Attachment is obsolete: true
Flags: in-testsuite+
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
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
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.
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?
Flags: needinfo?(masayuki)
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).
Flags: needinfo?(masayuki)
Attached patch additional fixSplinter Review
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
Attachment #8642449 - Flags: review?(adw)
(you can ignore the X failures, that actually come from fx-team)
Attachment #8642449 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/397afd7d1e6b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1192211
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.