1047613 - Awesomebar-results-pane repeatedly opening up when typing a visited domain

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Elbart]

Attachment: screencap.mp4

When typing a previously visited domainname (i.e. 'mozilla.org'), the results-pane of the locationbar is reopening after every typed latter.

Setting browser.urlbar.unifiedcomplete to false reverts this problem.

Screencap:
First typing the domain before it's visited (no problem), then after it's visited (problem), then after disabling the pref (no problem).

Regression-range:

m-c:
Last good revision: a91ec42d6a9c (2014-07-24)
First bad revision: 613e79262240 (2014-07-25)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a91ec42d6a9c&tochange=613e79262240

m-i:
Last good revision: b0a916157c9a
First bad revision: de374e83ed83
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b0a916157c9a&tochange=de374e83ed83

Comment 1

[Marco Bonardo [:mak]]

off-hand looks like due to completeDefaultIndex

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Patch - Stop executing the host query first to avoid a jumpy results pane

What's happening here is that the hostQuery is returning much faster than the other queries and notifying with a single result. Eventually the other queries finish and populate the pane with more results causing it to expand. With each new keystroke this is repeated, causing the results pane to jump back and forth.

The hostQuery was executing before the sleep on Prefs.delay, so by default 50ms earlier, but I still see the jumpy results pane even if I execute the hostQuery after this sleep, but before the other queries.

Instead of executing the hostQuery last, another approach would be to just cache the result and only notify once the next set of results has also been received.

I'm not sure if there is a great way to avoid jumpiness but also try and return at least some result as fast as possible. It seems like Chrome always shows the same number of results even if they're not relevant, and then replaces them when the new results come in. I wonder if it would be worth re-visiting the UX around how we show the results?

Comment 3

[Steven MacLeod [:smacleod]]

It appears Marco will be away starting tomorrow so I'm not sure if he'll get to this.

Tim, Paolo, would either of you be able to weigh in on this or know who to
r?

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8473139 [details] [diff] [review]
Patch - Stop executing the host query first to avoid a jumpy results pane

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

we cannot do this because the inline queries run while the user is typing, thus it should be as-instant-as-possible (it was synchronous before). We just had a bug in Nightly where it was wrongly delayed by 50ms and users immediately started filing bugs cause they were blindly pressing enter and finishing on the wrong result, due to the slight delay.
Unfortunately we need a solution that doesn't require changing order or delay of the queries.

Comment 5

[Marco Bonardo [:mak]]

Blair might also be able to help here.

Comment 6

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #4)
> Unfortunately we need a solution that doesn't require changing order or
> delay of the queries.

It seems to me that the Chrome solution from comment 2 could be our best bet, which requires caching previous results and replacing them as new results come in, but I'm not sure how easy it is to implement.

Given the number of edge cases that this code has to deal with, I don't feel confident reviewing any changes on my own, but feel free to flag me for a first pass on the next versions.

Comment 7

[Marco Bonardo [:mak]]

yes, we need some sort of "buffering" at the UI level.

Comment 8

[Steven MacLeod [:smacleod]]

I'd like to get some UX feedback on how we should proceed.

The current state can be seen in the video in attachment 8466420 [details] (The popup isn't repeatedly opening, just jumping between 1 result and many). We need to return the host matching as quickly as possible so that typing a few letters and hitting enter will take you to the correct page (See Comment 4)

We could change the UI so that jumping between 1 result and many isn't distracting, or possibly leave stale entries, but with an updated host entry, in the autocomplete until the new results come in.

Philipp, any thoughts on leaving the previous results with the one entry updated? or ways we could modify the UI to make this work?

Comment 9

[Steven MacLeod [:smacleod]]

Marco, this bug is waiting on UX input. I won't be finishing it this iteration so could you please remove my assignment in the spreadsheet and allow someone else to pick it up during this iteration. Thanks!

Comment 10

[Marco Mucci [:MarcoM]]

Thanks Steven.  I've made the updates.

Comment 11

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

(In reply to Steven MacLeod [:smacleod] from comment #8)
> I'd like to get some UX feedback on how we should proceed.
> 
> The current state can be seen in the video in attachment 8466420 [details]
> (The popup isn't repeatedly opening, just jumping between 1 result and
> many). We need to return the host matching as quickly as possible so that
> typing a few letters and hitting enter will take you to the correct page
> (See Comment 4)
> 
> We could change the UI so that jumping between 1 result and many isn't
> distracting, or possibly leave stale entries, but with an updated host
> entry, in the autocomplete until the new results come in.
> 
> Philipp, any thoughts on leaving the previous results with the one entry
> updated? or ways we could modify the UI to make this work?

Hey! Sorry this took a while.
It sounds like leaving the old entries in there is the most feasible solution, so let's do that.

Comment 12

[Julian Viereck]

> yes, we need some sort of "buffering" at the UI level.

Fly by comment: should the buffering really happen at the UI level? Caching the results sounds more like something the controller should do and therefore maybe implement this in the nsAutoCompleteController?

My understanding of the autocomplete code is still limited, but here my rough idea. Basically, let's cache the search results from last time and add a new filter method, which give a new search string determines which entries of the searchCache are still valid for the new search string.

1) completion results are cached in the nsAutoCompleteController
2) the nsAutoCompleteController can take a new callback function, call this |filterCache| for now
3) as a new search query comes in and the |filterCache| function is available, the |filterCache| function is called like

  filterCache(currentSearchInput, currentCache) -> updatedCacheList

where

  currentSearchInput = nsAutoCompleteController::mSearchString

4) the |filterCache| function then returns a subset of the passed in list |currentCache|, which is still valid for the new |currentSearchInput| (aka. |updatedCacheList|)
5) the |updatedCacheList| is used as source for the popup content until the new results come in

Example:

a) let's assume we do a simple autocompletion and for the input "a" we get the search results {"asap", "awesome", "alasca"}
b) the user types a new character and the input is now "as"
c) update the current cache: currentCache <- filterCache("as", {"asap", "awesome", "alasca"})
  HERE: |currentCache| == {"asap"}
d) show the result of {"asap"} until the full results come back

Comment 13

[Marco Bonardo [:mak]]

(In reply to Julian Viereck from comment #12)
> My understanding of the autocomplete code is still limited, but here my
> rough idea. Basically, let's cache the search results from last time and add
> a new filter method, which give a new search string determines which entries
> of the searchCache are still valid for the new search string.

This is not that simple cause we have adaptive results, that means "g" might match google first, while "go" might match godaddy first (cause the user many types typed "go" to visit godaddy). So it's not enough to filter results, we might also have to reorder them.

> a) let's assume we do a simple autocompletion and for the input "a" we get
> the search results {"asap", "awesome", "alasca"}
> b) the user types a new character and the input is now "as"
> c) update the current cache: currentCache <- filterCache("as", {"asap",
> "awesome", "alasca"})
>   HERE: |currentCache| == {"asap"}
> d) show the result of {"asap"} until the full results come back

This indeed assumes the new results will come after the old ones, that we can't guarantee.

Comment 14

[Julian Viereck]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #13)
> 
> This is not that simple cause we have adaptive results, that means "g" might
> match google first, while "go" might match godaddy first (cause the user
> many types typed "go" to visit godaddy). So it's not enough to filter
> results, we might also have to reorder them.

Good point, thanks for raising it. In this case, let's rename |filterCache| to |computeCompletionFromCache|. This function can return a subset of the current cached values and also permute them. If the search results have enough information associated to them, it is then possible to compute the correct first entry for a new input value - like the "go" example you pointed out above.

Note that I don't know if there is enough of this information stored on the search results or not. Depending on this answer this approach might not work out.

> > a) let's assume we do a simple autocompletion and for the input "a" we get
> > the search results {"asap", "awesome", "alasca"}
> > b) the user types a new character and the input is now "as"
> > c) update the current cache: currentCache <- filterCache("as", {"asap",
> > "awesome", "alasca"})
> >   HERE: |currentCache| == {"asap"}
> > d) show the result of {"asap"} until the full results come back
> 
> This indeed assumes the new results will come after the old ones, that we
> can't guarantee.

I am not sure if I understand you right on this. The setup as I understand it from your reply is like this:

- the computedValuesFromCache = ["asap"]
- the search results come back as ["foo", "asap", "bar"]

so the new search results "foo" is not comming after the old one "asap".

Does this agree with what you have on your mind? In case it does, I don't see the problem here. The entire search result is used (in this case ["foo", "asap", "bar"]) and therefore it doesn't matter what the |computedValuesFromCache| looked like before. (E.g. it doesn't matter if there are entries before the first entry in |computedValuesFromCache|.)

Comment 17

[Ed Lee :Mardak]

(In reply to Julian Viereck from comment #12)
> a) let's assume we do a simple autocompletion and for the input "a" we get
> the search results {"asap", "awesome", "alasca"}
> b) the user types a new character and the input is now "as"
> c) update the current cache: currentCache <- filterCache("as", {"asap",
> "awesome", "alasca"})
>   HERE: |currentCache| == {"asap"}
> d) show the result of {"asap"} until the full results come back
This still results in the list shrinking because it's removing some items that were shown before, so there can still be flickering. It would be less common though in some situations such as typing out "mozilla" and all results already match mozilla.

One way to keep the old items shown is to not collapse them immediately on invalidate:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#1016

I'm thinking setting a timer to collapse in a little bit and annotates the item as to-be-collapsed that gets cleared on appendCurrentResult. It seems that new items are created and appended currently, so that'll need to insert instead. (Where do items get removed??)

Comment 20

[Nickolay_Ponomarev]

Am I right in assuming this bug is wider in scope than its summary says? The flickering happens for me whenever:
- there's more than 1 result and
- the first result is "Visit <domain>" or "... - Search with Google"
...so pretty much always.

I see Marco changed this bug from blocking the UnifiedComplete bug to being dependent on it. Does it mean UC might ship it in the current state?

Comment 21

[Marco Bonardo [:mak]]

(In reply to Nickolay_Ponomarev from comment #20)
> Am I right in assuming this bug is wider in scope than its summary says?

The flickering happens every time we have new results, the summary looks correct to me. It's pretty much always, yes.

> I see Marco changed this bug from blocking the UnifiedComplete bug to being
> dependent on it. Does it mean UC might ship it in the current state?

yes, that's correct.

Comment 23

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: unified complete is enabled for this version.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for FF41 as this is related to unified complete. Based on the fact that this bug was filed almost a year ago, I think this affects several Firefox releases in the past.

Comment 25

[Marco Bonardo [:mak]]

(In reply to Ritu Kothari (:ritu) from comment #24)
> Adding a tracking flag for FF41 as this is related to unified complete.
> Based on the fact that this bug was filed almost a year ago, I think this
> affects several Firefox releases in the past.

no it doesn't, this only affects builds where unified complete is enabled, that currently is just 41

Comment 26

[Daniel Holbert [:dholbert]]

(The unified autocomplete pref was enabled on trunk in the past, in bug 1040335 -- that's around when this bug was filed. The pref was turned off in bug 1105456. It's now turned on again, as of bug 1168811.)

Comment 27

[Marco Bonardo [:mak]]

fwiw, I also think we should disable unified complete in Aurora and just release it in version 42, along with search suggestions that it's the first feature making use of it.
We still have other regressions and work in progress to improve it and I don't think at this point makes sense to just proceed blindly.
I'll file a bug for that.

Comment 28

[Marco Bonardo [:mak]]

this is no more an issue after disabling unified complete in Firefox 41 bug 1180205

Comment 29

[Marco Bonardo [:mak]]

Attachment: wip

still tweaking code, but results look pretty good in a debug build already

Comment 30

[Marco Bonardo [:mak]]

Attachment: patch v1

This removes various causes of flickering and pointless work we were doing.
From my testing it works quite well, but I only tried in a debug build. The delay I choose is based on telemetry we have to collect the first 6 results, 250ms should cover slightly more than 90% of cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41abe9a0b634

Comment 31

[Drew Willcoxon :adw]

Comment on attachment 8634739 [details] [diff] [review]
patch v1

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

Nice, it works well.  Have you tested this with the patch from bug 959567?  I don't think they should substantively conflict, but it would be good to know, particularly around the CSS transitions.

::: browser/base/content/browser.xul
@@ +715,5 @@
>                       autocompletesearch="urlinline history"
>                       autocompletesearchparam="enable-actions"
>                       autocompletepopup="PopupAutoCompleteRichResult"
>                       completeselectedindex="true"
> +                     shrinkdelay="250"

Should this be on the popup instead?  But, we already have all these attributes on the textbox that actually apply to the popup, like showimagecolumn and maxrows, so maybe not.

::: toolkit/content/widgets/autocomplete.xml
@@ +1062,5 @@
>            if (!this.hasAttribute("height")) {
>              // collapsed if no matches
>              this.richlistbox.collapsed = (this._matchCount == 0);
>  
> +            // Dynamically update the richlistbox height.

"Dynamically" isn't really doing anything in this sentence.

@@ +1071,5 @@
> +              clearTimeout(this._shrinkTimeout);
> +            }
> +            this._adjustHeightTimeout = setTimeout(() => this.adjustHeight(), 0);
> +          } else {
> +            this._collapseUnusedItems();

Could you please add a comment saying what the height attribute is for?  The way you explained it in the other bug helped me understand why you need to collapseUnusedItems() here in the else-branch: because the popup consumer wants to force a height and therefore we shouldn't adjust it.

@@ +1145,5 @@
> +            this._shrinkTimeout = setTimeout(() => {
> +              this.richlistbox.style.height = height + "px";
> +              if (this._rlbAnimated) {
> +                this.addEventListener("transitionend", function onEnd(event) {
> +                  this.removeEventListener("transitionend", onEnd, true);

This won't work as expected: onEnd isn't actually the listener function, onEnd.bind(this) is.  Right?  fn.bind() returns a new function.

Comment 32

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #31)
> Nice, it works well.  Have you tested this with the patch from bug 959567? 

I did initially, I'm going to do again before landing with the current version of that patch.

> ::: browser/base/content/browser.xul
> @@ +715,5 @@
> >                       autocompletesearch="urlinline history"
> >                       autocompletesearchparam="enable-actions"
> >                       autocompletepopup="PopupAutoCompleteRichResult"
> >                       completeselectedindex="true"
> > +                     shrinkdelay="250"
> 
> Should this be on the popup instead?  But, we already have all these
> attributes on the textbox that actually apply to the popup, like
> showimagecolumn and maxrows, so maybe not./*

Yeah, the input field has become de-facto container of attributes for the whole autocomplete stuff, the popup has only generic panel/popup attributes, so I didn't change that.

> @@ +1145,5 @@
> > +            this._shrinkTimeout = setTimeout(() => {
> > +              this.richlistbox.style.height = height + "px";
> > +              if (this._rlbAnimated) {
> > +                this.addEventListener("transitionend", function onEnd(event) {
> > +                  this.removeEventListener("transitionend", onEnd, true);
> 
> This won't work as expected: onEnd isn't actually the listener function,
> onEnd.bind(this) is.  Right?  fn.bind() returns a new function.

Yeah, thanks for pointing that out.

Comment 33

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #32)
> > This won't work as expected: onEnd isn't actually the listener function,
> > onEnd.bind(this) is.  Right?  fn.bind() returns a new function.
> 
> Yeah, thanks for pointing that out.

Actually, I notice various parts of our code are using bind(this) in addEventListener and then trying to removeEventListener the function name, so I think this is not well known around. Maybe you could blog post about that!

I will also file a bug to fix the broken usage around.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4a1d6222e2f7

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a1d6222e2f7

Comment 36

[Mehmet]

Attachment: flicker while delete

@Marco Bonardo [::mak]:

Thanks for fixing it. But I noticed that the blue highlighted result is still flickering, when you delete letters in the location bar with the delete key.

Please find enclosed a screencast.

Comment 37

[Marco Bonardo [:mak]]

(In reply to Mehmet Sahin from comment #36)
> Created attachment 8637268 [details]
> flicker while delete
> 
> @Marco Bonardo [::mak]:
> 
> Thanks for fixing it. But I noticed that the blue highlighted result is
> still flickering, when you delete letters in the location bar with the
> delete key.
> 
> Please find enclosed a screencast.

Yes, removing chars from the end is a different beast and I preferred to not touch it here (Also cause it involves selection), so it should be handled apart.
could you please file a separate bug for that?

Comment 38

[Mehmet]

(In reply to Marco Bonardo [::mak] from comment #37)
> (In reply to Mehmet Sahin from comment #36)
> > Created attachment 8637268 [details]
> > flicker while delete
> > 
> > @Marco Bonardo [::mak]:
> > 
> > Thanks for fixing it. But I noticed that the blue highlighted result is
> > still flickering, when you delete letters in the location bar with the
> > delete key.
> > 
> > Please find enclosed a screencast.
> 
> Yes, removing chars from the end is a different beast and I preferred to not
> touch it here (Also cause it involves selection), so it should be handled
> apart.
> could you please file a separate bug for that?

Thanks for your feedback. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1186471.

Comment 39

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #33)
> I will also file a bug to fix the broken usage around.

Did you? I would love to help get that addressed.

Comment 40

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #39)
> (In reply to Marco Bonardo [::mak] from comment #33)
> > I will also file a bug to fix the broken usage around.
> 
> Did you? I would love to help get that addressed.

yes, bug 1185893.

Comment 41

[Andrei Vaida [:avaida]]

This is verified fixed on Nightly 42.0a1 (2015-08-03) using Windows 10 Pro x64 (10240), Mac OS X 10.10.4 and Ubuntu 14.04 x64.

Apart from Bug 1186471, I noticed that two error messages get thrown in the Browser Console while trying to reproduce this bug: https://www.dropbox.com/s/p3ja2nc3d4kh4pw/Bug1047613.txt?dl=0. Marco, is this expected?

Comment 42

[Marco Bonardo [:mak]]

(In reply to Andrei Vaida, QA [:avaida] from comment #41)
> Apart from Bug 1186471, I noticed that two error messages get thrown in the
> Browser Console while trying to reproduce this bug:
> https://www.dropbox.com/s/p3ja2nc3d4kh4pw/Bug1047613.txt?dl=0. Marco, is
> this expected?

that's bug 1188688, should be fixed in tomorrow's nightly.

Comment 43

[Andrei Vaida [:avaida]]

(In reply to Marco Bonardo [::mak] from comment #42)
> that's bug 1188688, should be fixed in tomorrow's nightly.

Thanks for the prompt reply. I'll keep an eye on Bug 1188688 tomorrow then.