1192505 - location bar suggestions disappear if mouse moves

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Radek 'sysKin' Czyz]

If mouse moves over the suggestions dropdown while the dropdown is still being calculated, it folds down to nothing.
It started happening a few weeks ago. Happens in fresh profile (in fact more reliably than in my old profile)

Steps to reproduce:
1. Go to location bar and enter one letter of something that will bring suggestions. On fresh profile, "m" is good because there is a bunch or mozilla.org bookmarks to appear.
2. Now, the tricky part: take your mouse, hover over those suggestions, and start moving it (for example from left to right). Your pointer mustn't leave the suggestions dropdown but should move fast enough.
3. While mouse is moving, enter a second letter ("o" for the mozilla.org bookmarks of fresh profile)

Actual results: ALL suggestions, other than "visit <domain>" just disappear

Expected results: suggestions for the string "mo".

Sometimes you will see only partial suggestions, such as only bookmarks but not history. But mostly it's just empty.

Comment 1

[Marco Bonardo [:mak]]

can you still reproduce in Nightly? I tried but I cannot reproduce.
maybe a screen cast would help reproducing?

Comment 2

[Radek 'sysKin' Czyz]

(In reply to Marco Bonardo [::mak] from comment #1)
> can you still reproduce in Nightly?

Absolutely, it haunts me every day...

> I tried but I cannot reproduce.
> maybe a screen cast would help reproducing?

Hmmm ok I'll prepare one, give me a moment (or day).

Comment 3

[Radek 'sysKin' Czyz]

Attachment: moz.mp4

Screencast - fresh profile.

First, I enter "mo" to prove that suggestions for string "mo" exist.

Then, I backspace once and enter "o" again, but this time I'm moving the mouse from left to right over the "about us" suggestion entry. This time, as soon as I enter "o", suggestions disappear.

Does that help?

Comment 4

[Marco Bonardo [:mak]]

I suspect this is related to the fact as soon as the selection changes we cancel the queries, the complex steps to reproduce seem to cancel the query before the search delay has elapsed.
The reason to cancel the query is that we can insert results in the middle of others, and we don't want to surprise the user replacing a result at the last second before he clicks on it. The urlbar doesn't support using mouse and keyboard contemporary.

There may be ways to make this less annoying, but first I'd like to understand which action makes you type and select with the mouse at the same time, you said it happens everyday so maybe there's a specific action you are trying to achieve  that is not possible with just the keyword and we could improve that?

Comment 5

[Marco Bonardo [:mak]]

the related code is the "select" handler in urlbarbindings.xml

Comment 6

[Radek 'sysKin' Czyz]

(In reply to Marco Bonardo [::mak] from comment #4)
> first I'd like to
> understand which action makes you type and select with the mouse at the same
> time, you said it happens everyday so maybe there's a specific action you
> are trying to achieve  that is not possible with just the keyword and we
> could improve that?

That's an interesting question. Thinking about this, I think I just got used to opening pages by typing a few letters, and then clicking the dropdown. Since I'm moving the mouse to the expected dropdown as I'm typing (yay two hands), this now almost always triggers this bug.

The fact that I have a "gaming" mouse (relatively many smaller mouse move events compared to cheap office mice) might not be helping either.

Comment 7

[Marco Bonardo [:mak]]

I see. It's unrelated to this bug solution, but could I suggest to try using the up/down arrow keys and Enter. That would likely improve your productivity with the awesomebar

Comment 8

[Radek 'sysKin' Czyz]

While your suggestion would obviously sidestep the problem, I don't think I'll even try to fight my muscle memory like that.
Together with mousegestures (right-click-up opening of new tab) I can open a large number of tabs by hardly moving my hands (left hand keeps typing, right hand holds the mouse). Arrow keys are nowhere close, and ctrl-t is more clunky too.

Hopefully we agree that clicking on awesomebar result is not an unusual operation lol. Especially if the code is there to not reorder clickable results before user clicks.

Comment 9

[Radek 'sysKin' Czyz]

(In reply to Marco Bonardo [::mak] from comment #5)
> the related code is the "select" handler in urlbarbindings.xml

By the way, bingo! Disabling this stopSearch() makes the problem disappear.

Hopefully it's not particularly difficult to make this "select" event be ignored for dropdown entries that are from previous search and are about to be replaced by current search - because if I understand correctly, this would fix my problem while keeping this (completely reasonable) cancel logic.

Comment 11

[Marco Bonardo [:mak]]

This seems to annoy more people, so bumping the priority a bit, it's something between p2 and p3.

Comment 12

[2slow4flo]

(In reply to Radek 'sysKin' Czyz from comment #9)
> (In reply to Marco Bonardo [::mak] from comment #5)
> > the related code is the "select" handler in urlbarbindings.xml
> 
> By the way, bingo! Disabling this stopSearch() makes the problem disappear.
> 
> Hopefully it's not particularly difficult to make this "select" event be
> ignored for dropdown entries that are from previous search and are about to
> be replaced by current search - because if I understand correctly, this
> would fix my problem while keeping this (completely reasonable) cancel logic.

Does this also make the problem disappear if you trigger the bug shown in my attachment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1206414 ?

Also the detection of 'selecting a match' does not function properly if simple mouse movements trigger controller.stopSearch();

Comment 14

[Marco Bonardo [:mak]]

bumping up priority. if a user is affected by this (thus he uses mouse and keyboard at the same time), using the location bar becomes a nightmare. So it's likely something we want to evaluate sooner than later.

Comment 15

[fireattack [:fireattack]]

Thanks for report this! This bug drives me crazy recently.

Shouldn't this be considered as regression, as well (I mean for the keywords)?

Comment 16

[Marco Bonardo [:mak]]

sure

Comment 17

[Mark Banner (:standard8)]

Regression range from bug 1206414:

(In reply to 2slow4flo from comment #11)
> Ok took me ages to find the version where this was introduced, I was way too
> optimistic with the last known good date.
> 
> Last good build --->
> app_name: firefox
> build_date: 2015-07-30
> build_type: nightly
> build_url:
> https://archive.mozilla.org/pub/mozilla.org/firefox/nightly/2015/07/2015-07-
> 30-03-02-08-mozilla-central/firefox-42.0a1.en-US.win64.zip
> changeset: 62469b20ec84
> pushlog_url:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=62469b20ec84&tochange=32712cd01159
> repo_name: mozilla-central
> repo_url: https://hg.mozilla.org/mozilla-central
> 
> First bugged build --->
> app_name: firefox
> build_date: 2015-07-31
> US.win64.zip
> build_type: nightly
> build_url:
> https://archive.mozilla.org/pub/mozilla.org/firefox/nightly/2015/07/2015-07-
> 31-03-02-06-mozilla-central/firefox-42.0a1.en-US.win64.zip
> changeset: ca53d4297f02
> pushlog_url:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=62469b20ec84&tochange=ca53d4297f02
> repo_name: mozilla-central
> repo_url: https://hg.mozilla.org/mozilla-central

Comment 18

[Mark Banner (:standard8)]

[Tracking Requested - why for this release]: Confusing regression - if you happen to move the mouse whilst autocomplete is active, then you don't get the results shown.

Comment 19

[Marco Bonardo [:mak]]

I will look into this, hope to get an easy solution for uplift.

Note the reach of the bug is limited to users moving the mouse across the autocomplete popup while typing a search or just after finished typing, while the search is still ongoing in background.

Comment 20

[Marco Bonardo [:mak]]

this is complicate, so let's sum up.
The scope is to stop providing new results when the user explicitly moves the mouse over a match, cause we don't want to replace the result that is currently under the mouse.

It's clear we cannot rely on the select event, cause selection is reset to -1 and 0 by the ongoing searches, then by moving the mouse it's set back to the hovered item. There are too many select events and we can't distinguish keyboard, mouse an automatic select events.

I can try with the mouseover event to see if I can get a more reliable behavior. Then there's also the fact stopping a search may clear current results, that needs some more thoughts.

I evaluated other solutions like not-updating the selected element, but again we'd need to know how that element was selected and we may lose results that way. The many jumps between controller/search/ui don't make this easy cause often one doesn't know about status of the other.

Comment 21

[Radek 'sysKin' Czyz]

What happens in the video from comment #3 is that the hover is over a search result for string "m", but it stops the search for "ma". There is that short period of time when search for "ma" is already underway, but no results are found yet, and the dropdown is still showing old contents.

In other words, the sequence is:
- dropdown for "m" is visible and responds to hover
- search is started
- hover over dropdown fires, search is stopped
- search is finished so dropdown gets updated with search results (no results)

Perhaps suppressing query cancellation during that period would be a good way to fix this?

Comment 22

[Marco Bonardo [:mak]]

(In reply to Radek 'sysKin' Czyz from comment #21)
> Perhaps suppressing query cancellation during that period would be a good
> way to fix this?

what does that mean? This bug is not about moving from a search that has results to a search that has no results, that is supposed to show an empty popup (otherwise we'd do the wrong thing). We should generally always have results that match with the search string.

This bug is about the fact we stop a search when there WOULD be results, but we suppress them.
- search for "m"
- type "o": search for "m" is stopped, search for "mo" starts
- hover old result for "m" (still shown cause we are async): search for "mo" gets stopped
- since the search for "mo" didn't have the time to fetch anything, we end up with an empty resultset, even if it would have results

Fixing this in a perfect way would require too deep changes across the autocomplete components. I am working on a simpler fix that should cover the most annoying cases.

Will post a Try build for you to test asap.

Comment 23

[Radek 'sysKin' Czyz]

(In reply to Marco Bonardo [::mak] from comment #22)
>  an empty resultset

Sorry if I wasn't clear, by "no results" I meant this - no results because search was stopped before it returned any.

Comment 24

[Radek 'sysKin' Czyz]

(In reply to Marco Bonardo [::mak] from comment #22)
> - hover old result for "m" (still shown cause we are async): search for "mo"
> gets stopped

...and addressing this dependency right here is what I was suggesting - do not stop the search if results are from search "m" but we're now doing a different search for "mo".

(apologies for two replies)

Comment 25

[Marco Bonardo [:mak]]

yeah, that's what I am doing. The problem is that the richlistitems have no clue which search generated them, so I must use an hack-ish workaround.

Comment 26

[Radek 'sysKin' Czyz]

Yeah I wanted to write a patch myself but I thought I'd be ugly, partially because I don't know that code at all.
So what I'd probably try is:
- create extra boolean called cancelSupressed somewhere in the right scope
- when new search starts, set cancelSupressed=true
- when first result comes back (and stale dropdown gets replaced), set cancelSupressed=false
- wrap the stopSearch() with the if (!cancelSupressed) block

...just not as ugly lol :)

Comment 27

[Radek 'sysKin' Czyz]

....wait, there is already  a boolean like that, called gotResultForCurrentQuery

So my patch is just:
line 1549:
-if (!this._ignoreNextSelect && this.selectedIndex >= 0) {
+if (!this._ignoreNextSelect && this.selectedIndex >= 0 && gotResultForCurrentQuery) {

Comment 28

[Marco Bonardo [:mak]]

Attachment: 1192505.diff

(In reply to Radek 'sysKin' Czyz from comment #27)
> So my patch is just:
> line 1549:
> -if (!this._ignoreNextSelect && this.selectedIndex >= 0) {
> +if (!this._ignoreNextSelect && this.selectedIndex >= 0 &&
> gotResultForCurrentQuery) {

that's not enough. First gotResultForCurrentQuery can be true for both "old" and "new" query, then the fact we got a result with 1 match wouldn't help if we'd be selecting the third entry, the popup would collapse to 1 entry.

This patch stops only the current query and only if the user explicitly hovered a result. It works well... Indeed it works too well :(
If the user moves the mouse on the first entry, even inadvertently, just after typing, no results are returned. After all something has been selected with the mouse so the behavior is correct. But the user-facing behavior sucks.

At this point I'm putting this apart for a while and investigating the alternative, that is to just avoid replacing a match under the mouse cursor. It will require some jumps between urlbarBindings.xml and autocomplete.xml...

Then I'll see which solution is less sucky for the user.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43+ since this is a regression.  
Sylvestre, just making sure you notice this for 42 as well.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Tracking as it is a visual regression, however, we are late in the cycle, except if it is an easy fix, we will probably ship 42 with this bug.

Comment 31

[Marco Bonardo [:mak]]

I will post a trivial patch for beta, that will just remove this code. The regressing code is only needed for UnifiedComplete that is only enabled from Firefox 43 on.

Comment 32

[Marco Bonardo [:mak]]

Attachment: beta only patch

Approval Request Comment
[Feature/regressing bug #]: Unified complete
[User impact if declined]: Moving the mouse over the locationbar popup while typing may remove all the location bar results
[Describe test coverage new/current, TreeHerder]: local
[Risks and why]: the handler I'm removing is actually only needed for UnifiedComplete that is enabled only in Firefox 43. Old autocomplete code cannot cause the problem this handler is trying to prevent.
[String/UUID change made/needed]: none

Comment 33

[Marco Bonardo [:mak]]

I meant to indicate there should be no risk, I ended up explaining why, but forgot to point out the actual risk assessment

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8676941 [details] [diff] [review]
beta only patch

Taking this workaround, thanks.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Should be in 42 beta 9.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/28e133a79f2e

Comment 37

[Marco Bonardo [:mak]]

Attachment: patch v1

this one should be better, I was able to reuse the existing handlers, so it should have no perf impact.

Note: while I could try to write a test, it would be a big expense (it's complicate to emulate the very specific mouse/search situation) compared to the small case this code covers (replacing a match when it's being pointed it). I don't think it's worth the cost for now, even if the regression was quite annoying. We should remember to not stop searches from the frontend in future.

Comment 38

[Marco Bonardo [:mak]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=288bd4a6ed15

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6e8d9aee74ad

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e8d9aee74ad

Comment 41

[Marco Bonardo [:mak]]

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

Approval Request Comment
[Feature/regressing bug #]: Unified complete
[User impact if declined]: sometimes the awesomebar may not give results
[Describe test coverage new/current, TreeHerder]: Nightly, local
[Risks and why]: The change affects a very specific case, so it should be mostly safe
[String/UUID change made/needed]: none

Comment 42

[Sylvestre Ledru [:Sylvestre]]

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

Make sense, taking it.
Should be in the first devedition release & 43 beta 1.

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c670c565d728

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a27fbb191e5

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6e8d9aee74ad

Comment 46

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c670c565d728

Comment 48

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

Verified as fixed using Firefox 43 beta 1, latest Dev Edition 44.0a2 and latest Nightly 45.0a1 under Win 7 64-bit and Mac OS X 10.9.5.