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

VERIFIED FIXED in Firefox 42

Status

()

Firefox
Location Bar
P1
normal
Rank:
2
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: Elbart, Assigned: mak)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {regression})

unspecified
Firefox 42
x86_64
Windows 7
regression
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox41 unaffected, firefox42 verified)

Details

(Whiteboard: [unifiedautocomplete][suggestions][fxsearch])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8466420 [details]
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
(Reporter)

Updated

3 years ago
Blocks: 1040335
off-hand looks like due to completeDefaultIndex
Blocks: 995091
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Points: --- → 5
Flags: firefox-backlog+

Updated

3 years ago
QA Whiteboard: [qa+]
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Iteration: --- → 34.2
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida
Created attachment 8473139 [details] [diff] [review]
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?
Attachment #8473139 - Flags: review?(mak77)
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?
Flags: needinfo?(ttaubert)
Flags: needinfo?(paolo.mozmail)
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.
Attachment #8473139 - Flags: review?(mak77) → review-
Blair might also be able to help here.

Comment 6

3 years ago
(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.
Flags: needinfo?(paolo.mozmail)
yes, we need some sort of "buffering" at the UI level.
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?
Flags: needinfo?(ttaubert) → needinfo?(philipp)
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!
Assignee: smacleod → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mmucci)
Thanks Steven.  I've made the updates.
Iteration: 34.2 → ---
Flags: needinfo?(mmucci)
(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.
Flags: needinfo?(philipp)

Updated

3 years ago
QA Whiteboard: [qa+]
Flags: qe-verify+

Comment 12

3 years ago
> 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
(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

3 years ago
(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|.)
Duplicate of this bug: 1072688
Blocks: 1071461
No longer blocks: 995091, 1040335
Depends on: 995091
Duplicate of this bug: 1079801
(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??)
Duplicate of this bug: 1080707

Updated

3 years ago
Depends on: 1089565
Duplicate of this bug: 1086543

Comment 20

3 years ago
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?
(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.

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: 1176078
[Tracking Requested - why for this release]: unified complete is enabled for this version.
tracking-firefox41: --- → ?
Whiteboard: [unifiedautocomplete]
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.
status-firefox41: --- → affected
tracking-firefox41: ? → +
(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
(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.)
Whiteboard: [unifiedautocomplete] → [unifiedautocomplete][suggestions]
Priority: -- → P1
Whiteboard: [unifiedautocomplete][suggestions] → [unifiedautocomplete][suggestions][fxsearch]

Updated

2 years ago
Rank: 2
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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.
Flags: needinfo?(gijskruitbosch+bugs)
this is no more an issue after disabling unified complete in Firefox 41 bug 1180205
status-firefox41: affected → unaffected
status-firefox42: --- → affected
tracking-firefox41: + → ---

Updated

2 years ago
Iteration: --- → 42.2 - Jul 27
Attachment #8473139 - Attachment is obsolete: true
Created attachment 8634193 [details] [diff] [review]
wip

still tweaking code, but results look pretty good in a debug build already
Created attachment 8634739 [details] [diff] [review]
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
Attachment #8634193 - Attachment is obsolete: true
Attachment #8634739 - Flags: review?(adw)
Component: Places → Location Bar
Product: Toolkit → Firefox

Comment 31

2 years ago
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.
Attachment #8634739 - Flags: review?(adw) → review+
(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.
(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

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/4a1d6222e2f7
https://hg.mozilla.org/mozilla-central/rev/4a1d6222e2f7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Comment 36

2 years ago
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.
(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

2 years ago
(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

2 years ago
(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.
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
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?
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
Flags: qe-verify+ → needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
(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.
Blocks: 1089565
No longer depends on: 1089565

Updated

2 years ago
Depends on: 1222432

Updated

a year ago
Depends on: 1257642

Updated

a year ago
Depends on: 1291927
You need to log in before you can comment on or make changes to this bug.