Closed Bug 1410240 Opened 7 years ago Closed 7 years ago

Search suggestions keep displacing awesomebar results as I'm about to click on them

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: rfkelly, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

recording of my autofill experience
25 bytes, text/plain
Details
Bug 1410240 - Search suggestions keep displacing awesomebar results as I'm about to click on them.
59 bytes, text/x-review-board-request
mak
: review+
Details
Beta 58 patch
16.15 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review 
I guess this is related to the new "show search suggestions above history items" feature discussed in Bug 1409450 and friends.  I don't have any complaint about that ordering, but I've been seeing an annoying consequence of it lately:

* I start typing something in the awesomebar
* Results from my history appear in the top of the awesomebar autocomplete list
* I visually identify the history result that I'm after, and go to click on it or down-arrow-and-enter to it
* Search suggestions suggestions suddenly appear at the top of the autocomplete list, pushing the history items down
* I end up activating one of the new search suggestion entries instead of the thing I was trying to click

I'm not sure exactly what causes this.  Do we display local autocomplete results while still waiting for search suggestions from the network?  Or am I just going too fast, visually identifying the autocomplete item that I want while still typing in the address bar, and then the list changes to reflect the last character I entered after I've already started navigating down to that item?

Either way it's pretty annoying - I've already done three accidental searches this morning as a result of this behaviour.
This happened to me again just now, so I made a short recording of it to show what I mean.  I wanted to visit the github page for https://github.com/mozilla-services/server-syncstorage which I habitually do by typing "server-" and letting it autocomplete from history.

As you can see, the autocomplete from history appears at the top of the list for a few moments, before being pushed down the list by incoming search suggestions.  I now have to mentally pause and wait for the search suggestions to come in before trying to click on anything from the autocomplete.
I'm not sure what we could do here. Apart from freezing the list when the selection changes, that on the other side could cause unwanted freezings for users who tend to start moving the selection soon. Though maybe that's just an edge case. I think Philipp in the past suggested exactly that we freeze results on selection change.

The underlying problem is that we can't predict at which time search suggestions come compared to local suggestions, and we can't wait because both network and disk times are unpredictable.
Having one local suggestion at the top may probably reduce the risk of this happening for the most common local match.
Priority: -- → P2
Whiteboard: [fxsearch]
Marco, would it be possible to push history items to the top if their frecency value is above a certain threshold (for the typed string)?

For a user typing in "fac" it seems pretty important for usability that facebook.com always ends up on top.
(Obviously, given that facebook.com is the top site for "fac" for this user.)
> This happened to me again just now, so I made a short recording of it to show what I mean.

Oh, looks like this didn't attach correctly...let me know if you want to see it and I'll figure out how to attach it properly.
> I think Philipp in the past suggested exactly that we freeze results on selection change.

FWIW this sounds like it would feel better from my perspective - if you're able to tell that I've stopped typing and started navigating to a result in the autocomplete list, freeze the state of the list so that the item I'm navigating to doesn't move around.
Actually the video from Bug 1409810 Comment 1 does a pretty good job of showing it.
bumping up, since we plan to have searches higher in the list.

(In reply to David Naylor from comment #3)
> Marco, would it be possible to push history items to the top if their
> frecency value is above a certain threshold (for the typed string)?

The mixture of results is not matter of this bug, this is about the selection surprising the user.
Anything else should be discussed elsewhere. But notice we have experiments running continuously to analyze different mixtures, and it's not matter of guessing a better one, it's a complicate matter to get right.

Back on topic, there are 2 possible solutions we can implement here:
1. freeze the results list when the user changes the selection. One problematic thing is that also the code sometimes changes the selection, but it's likely workardoundable. Another problem is users inadvertently changing the selection and freezing their searches.
2. make the selection follow the result, so everything we add a new result above the selection, bump the selection index by 1.
Priority: P2 → P1
s/everything/everytime/
See Also: → 1409810
(In reply to Ryan Kelly [:rfkelly] from comment #1)
> Created attachment 8920414 [details]
> recording of my autofill experience
This is showing up as just text for me, but I've also got a video of this bug attached to a different bug:
https://bug1409810.bmoattachments.org/attachment.cgi?id=8919848
Based on recent discussions with Panos, I think we should probably go for the solution 1 in comment 9. The reason is that solution 2 may be visually distracting (the selection is very visible and it would move around.
The downside of solution 1 is much smaller than it was before, because now we don't select anymore with the mouse. Previously the risk was higher for those users who tend to move the mouse on the autocomplete popup while typing.
Additionally stopping the search when selection index becomes >= 1 should not be too expensive.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Marking as affected for 58, from the duplicate bug 1410674.
status-firefox58: --- → affected
Unconditionally stopping the search in the selectedIndex setter doesn't quite work.  The setter ends up getting called right as the search begins, and stopping the search on a stack that includes nsAutoCompleteController::BeforeSearches screws everything up.  So instead this patch stops it in autocomplete-rich-result-popup's selectBy() method, which is called when you arrow up and down to select a result.  That's the only place where selectedIndex is set that's relevant to this bug I think.

For posterity, note too that we already freeze a result (we don't "re-use" it) when you mouse over it, so we don't need to handle that in this bug.  Maybe we ought to revisit that in light of this bug.  It might make more sense to keep these two cases (mouse over and selection) the same, by stopping the search and freezing all results, as this patch does.
Comment on attachment 8929143 [details]
Bug 1410240 - Search suggestions keep displacing awesomebar results as I'm about to click on them.

https://reviewboard.mozilla.org/r/200438/#review207410

::: browser/base/content/test/urlbar/browser_urlbarStopSearchOnSelection.js:1
(Diff revision 1)
> +/* eslint-disable mozilla/no-arbitrary-setTimeout */

nit: please add a PD license boilerplate, until we are told otherwise we are still supposed to provide a license header where possible.

::: browser/base/content/test/urlbar/browser_urlbarStopSearchOnSelection.js:12
(Diff revision 1)
> +// This should match the `timeout` query param used in the suggestions URL in
> +// searchSuggestionEngineSlow.xml.
> +const TEST_ENGINE_SUGGESTIONS_TIMEOUT = 1000;
> +
> +add_task(async function init() {
> +  await PlacesTestUtils.clearHistory();

nit: Please await PlacesUtils.history.clear(); instead, the PlacesTestUtils shortcut is deprecated.
Here and below in the rest of the file.

::: browser/base/content/test/urlbar/browser_urlbarStopSearchOnSelection.js:42
(Diff revision 1)
> +  // work in this case because it causes the popup to close and re-open with the
> +  // new input text.
> +  EventUtils.synthesizeKey("1", {});
> +
> +  // Now press the Down arrow key to select the first suggestion.
> +  EventUtils.synthesizeKey("VK_DOWN", {});

there are possible pitfalls here that may cause intermittent failures. We type 1, and immediately DOWN, this could succeed or fail as well as it could end up at index 0 or 1.
Can we use waitForAutocompleteResultAt(1) to wait for the second row (that is what I supposed the expected target since row 0 is auto-selected) before DOWN?

::: toolkit/content/widgets/autocomplete.xml:1401
(Diff revision 1)
>              this.selectedIndex = this.getNextIndex(aReverse, amount, this.selectedIndex, this._matchCount - 1);
> +
> +            // This method is called when the user keys up or down to change
> +            // the selection.  Stop the search (if there is one) now so that the
> +            // results do not change while the user is making a selection.
> +            this.mInput.controller.stopSearch();

is there a reason to stop the search here, rather than in the controller, just before it invokes selectBy?
https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/toolkit/components/autocomplete/nsAutoCompleteController.cpp#505

note: this looks a bit scary, but we have the full cycle, in the worst case we can move it up the chain in urlbarbinding. But I'm ok to try the full solution first.
Attachment #8929143 - Flags: review?(mak77) → review+
note: since various tests simulate a VK_DOWN in the location bar, please run mochitest and mochitest-browser tests on Try, maybe with --rebuild 5, to check for intermittents.
(In reply to Marco Bonardo [::mak] from comment #17)
> nit: please add a PD license boilerplate, until we are told otherwise we are
> still supposed to provide a license header where possible.

Done

>> nit: Please await PlacesUtils.history.clear(); instead, the PlacesTestUtils
> shortcut is deprecated.
> Here and below in the rest of the file.

Done

> browser/base/content/test/urlbar/browser_urlbarStopSearchOnSelection.js:42
> (Diff revision 1)
> > +  // work in this case because it causes the popup to close and re-open with the
> > +  // new input text.
> > +  EventUtils.synthesizeKey("1", {});
> > +
> > +  // Now press the Down arrow key to select the first suggestion.
> > +  EventUtils.synthesizeKey("VK_DOWN", {});
> 
> there are possible pitfalls here that may cause intermittent failures. We
> type 1, and immediately DOWN, this could succeed or fail as well as it could
> end up at index 0 or 1.
> Can we use waitForAutocompleteResultAt(1) to wait for the second row (that
> is what I supposed the expected target since row 0 is auto-selected) before
> DOWN?

Done

> ::: toolkit/content/widgets/autocomplete.xml:1401
> (Diff revision 1)
> >              this.selectedIndex = this.getNextIndex(aReverse, amount, this.selectedIndex, this._matchCount - 1);
> > +
> > +            // This method is called when the user keys up or down to change
> > +            // the selection.  Stop the search (if there is one) now so that the
> > +            // results do not change while the user is making a selection.
> > +            this.mInput.controller.stopSearch();
> 
> is there a reason to stop the search here, rather than in the controller,
> just before it invokes selectBy?
> https://searchfox.org/mozilla-central/rev/
> 797c93d81fe446f78babf20894f0729f15f71ee6/toolkit/components/autocomplete/
> nsAutoCompleteController.cpp#505

True, I moved this to the controller.

(In reply to Marco Bonardo [::mak] from comment #18)
> note: since various tests simulate a VK_DOWN in the location bar, please run
> mochitest and mochitest-browser tests on Try, maybe with --rebuild 5, to
> check for intermittents.

Will do
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f955a43accb4
Search suggestions keep displacing awesomebar results as I'm about to click on them. r=mak
https://hg.mozilla.org/mozilla-central/rev/f955a43accb4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Is it worth getting this in beta?
Flags: needinfo?(adw)
I don't think so.  It's not a regression, Firefox has behaved like this for a while (I think), and this is the kind of behavior change that should ride the trains and gather feedback.
Flags: needinfo?(adw)
Well from what I understand the "search suggestions come before history" change which triggers this issue is new in 57.  But ok.
status-firefox58: affected → wontfix
(In reply to Julien Cristau [:jcristau] from comment #30)
> Well from what I understand the "search suggestions come before history"
> change which triggers this issue is new in 57.  But ok.

we undid that change. It's unclear but likely we may change that pref through experiments and the bug would hit in such a case. Not by default though.
(In reply to Marco Bonardo [::mak] from comment #31)
> we undid that change. It's unclear but likely we may change that pref
> through experiments and the bug would hit in such a case. Not by default
> though.

Maybe we should uplift this then?  Any such experiment would be less valid otherwise.
Attached patch Beta 58 patch — — Splinter Review 
Let's uplift this then.  I think running experiments against it is a good reason.

Approval Request Comment
[Feature/Bug causing the regression]:
Not a regression, but if we run experiments with the pref mentioned in this bug turned on, then those users in the experiment population may experience regressed behavior

[User impact if declined]:
See previous point

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Not really...

[Why is the change risky/not risky?]:
We just stop changing urlbar results once you make a selection

[String changes made/needed]:
None
Attachment #8933809 - Flags: approval-mozilla-beta?
See Also: → 1422512
Hi :adw,
If this is not a regression, I don't think we need to take this in Beta58. Do you know what experiments we want to run?
Flags: needinfo?(adw)
(In reply to Gerry Chang [:gchang] from comment #34)
> Hi :adw,
> If this is not a regression, I don't think we need to take this in Beta58.
> Do you know what experiments we want to run?

We will run experiments that change the results mixture in the Awesomebar. Actually I think there's also a plan to change the mixture for a subset of users (unified bar ones), Javaun may have more details about the involved versions and timeframes.
If we don't plan to do those changes in 58, then we could skip the uplift.
Flags: needinfo?(adw) → needinfo?(jmoradi)
We already have many users live who are affected. The overwhelming majority of new installs in 57 have search results first, so the majority of new 57 installs would benefit from this change. 

The current 57 mechanism is this: 
* In 57, new installs get Unified via Firefox (existing installs keep 2 search bars). 
While the hardcoded pref is history-first in the address bar, the overwhelming majority of new installs (unified bar) get search-first. This is the bulk of new 57 downloads.

Additionally, we do have new Shield experiments planned for 58. We will start testing unification on a very small population of existing users. Existing power users with a history-first workflow are the most likely to have Ryan's same issue that caused him to open this bug.

I would recommend this one for 58 uplift. Ryan Kelly, can you confirm this one suits your needs by trying it in Nightly?
Flags: needinfo?(jmoradi) → needinfo?(rfkelly)
> Ryan Kelly, can you confirm this one suits your needs by trying it in Nightly?

I'll give it a shot.  How can I restore the previous search-results-on-top behavior in order to ensure a fair comparison to last time?  The correct about:config foo wasn't clear from linked bugs.
Flags: needinfo?(rfkelly) → needinfo?(jmoradi)
See Also: 1422512 → 
> How can I restore the previous search-results-on-top behavior in order to ensure a fair comparison to last time?

I didn't manage to pref it on, but from watching for the search results to appear (or not) in their new position at the bottom of the list, this patch does seem to do what I'd expect and avoid changing the selection out from under me.  Thanks all!
Flags: needinfo?(jmoradi)
Ryan, this should do it if you'd still like to verify:

browser.urlbar.matchBuckets="suggestion:4,general:5"
Thanks!  I tried it and the behavior is definitely much improved.
Comment on attachment 8933809 [details] [diff] [review]
Beta 58 patch

Let's take this to support running experiments. Beta58+.
Attachment #8933809 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Did this change again? I'm seeing search suggestions displacing awesomebar results again.
See Also: → 1426276
(In reply to Marco Castelluccio [:marco] from comment #43)
> Did this change again? I'm seeing search suggestions displacing awesomebar
> results again.

Do you have the search bar in your chrome?  If not, you're probably seeing bug 1423247, which was backed out today.
(In reply to Drew Willcoxon :adw (Away 12/20–1/3) from comment #44)
> (In reply to Marco Castelluccio [:marco] from comment #43)
> > Did this change again? I'm seeing search suggestions displacing awesomebar
> > results again.
> 
> Do you have the search bar in your chrome?  If not, you're probably seeing
> bug 1423247, which was backed out today.

No, I only have the awesomebar. I'll test again on the next Nightly and close bug 1426276 if I can't reproduce anymore.
This is happening again in the latest Nightly.
See Also: → 1429729
See Also: → 1524510
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: