Closed Bug 1303781 Opened 8 years ago Closed 7 years ago

Sometimes selecting a history entry opens a Google search instead of navigating to the entry

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: marco, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(2 files)

Schermata del 2016-09-19 17-52-20.png
286.87 KB, image/png
Details
Bug 1303781 - Sometimes selecting a history entry opens a Google search instead of navigating to the entry.
59 bytes, text/x-review-board-request
mak
: review+
Details 
STR:
1) Open a new tab
2) Write the first characters of one of your history entries
3) Press the down arrow
4) Press enter

Firefox should navigate to the history entry, instead it often opens a Google search. This is a recent regression and usually happens when you do things quickly (step 2 and 3 almost at the same time).

I'm not sure step (3) is required, but it definitely makes it way easier to reproduce (I can do it pretty consistently).

I've sometimes noticed that when this happens, both the history entry and the Google icon are selected. See the attached image.
Summary: Sometimes selecting on a history entry opens a Google search instead of navigating to the entry → Sometimes selecting a history entry opens a Google search instead of navigating to the entry
Keywords: regression
The problem is that we don't buffer down/tab, so you get what is there at the time you press them.
What actually happens is that we show the popup with ONLY the first result and one off buttons, the other results still have to arrive (they are async) you press DOWN + ENTER, at that point you are effectively selecting the first one-off button and confirming with Enter.
note that, without one-off buttons, your DOWN+ENTER would have been interpreted as just ENTER, so it would have been wrong yet.
This is basically the same underlying problem as bug 1303462
Priority: -- → P2
Whiteboard: [fxsearch]
Hi Marco,
Is this only reproduced in nightly because one-off searches only enabled in nightly?
Flags: needinfo?(mcastelluccio)
yes, the behavior could also be wrong in other versions (it may not go the expected result) but it won't open the search from the first one-off button
Flags: needinfo?(mcastelluccio)
If this is nightly only, it does not affect 51, but does affect 52 (nightly.)  Is this a blocker for one-off search riding the trains?  What bug is that?
status-firefox51: affected → unaffected
status-firefox52: --- → affected
Flags: needinfo?(mak77)
(In reply to Milan Sreckovic [:milan] from comment #7)
> If this is nightly only, it does not affect 51, but does affect 52
> (nightly.)  Is this a blocker for one-off search riding the trains?  What
> bug is that?

The bug actually affects any version, but with different outcomes.
If one-off buttons are enabled it is more visible because we run a search instead of opening a url. But if they are disabled, we'd still act wrongly, indeed it may look like we ignored the DOWN/TAB and just opened whatever is the first result (that may be less infuriating for a user).

This would likely be a blocker for releasing one-off buttons, but there's no such bug cause management didn't decide when we should be releasing them.
We could maybe create one, but it wouldn't have an ETA until someone decides we can release the feature.
Flags: needinfo?(mak77)
This is fairly infuriating, and has now made it to Aurora, and a larger population of users. Please can it be fixed?

Gerv
one-off buttons are disabled in Aurora, even if they landed there.
Well, I'm definitely seeing a problem and I'm told that this bug is it. Do I need to copy in here my STR from firefox-dev?

Gerv
As I explained in comment 8, the problem exists at least from Firefox 43 (when unified complete was enabled) because the autocomplete behavior changed from sync to async to avoid UI jank. So results come asynchronously, but input is, by its own nature, synchronous. Thus we need to buffer input when we get it. I'm not sure why you only noticed it now, did you upgrade from an old version, or some of your add-ons became incompatible?

The one-off buttons make this more visible and frustrating because down and tab also go through them and run a search. In Aurora and further those buttons are disabled (if they are not, you may have flipped the browser.urlbar.oneOffSearches pref?), so what happens at a maximum is that instead of executing the second result, your may end up executing the first one.
This is annoying still, but less surprising than running a search with a random search engine.

Fwiw, this is non-trivial and it's already a P2, we'll come to it as soon as we have someone to work on it.
I have not changed the browser.urlbar.oneOffSearches pref. It's false. This doesn't seem connected to those. I've been using Developer Edition for ages, and it upgrades itself regularly. 

Executing the first result rather than the second is very annoying because the first is a search, and the second is the history item I wanted. So when this happens, I have to start the whole process again and do it slower so as not to confuse poor Firefox. Also, I don't particularly want to send a search engine extra information about what I'm doing.

Gerv
the only temporary workaround I can suggest for privacy, is to set keyword.enabled to false. But that basically disables searching from the urlbar, so it's a feature-loss.
I search from the URL bar _all_ the time. That's precisely why I don't want Down-Arrow+Enter to search - because just pressing Enter does a search! Having those two actions do the same thing is pointless. So thank you for your suggestion, but I won't be doing that.

Gerv
Priority: P2 → P1
Depends on: 1351936
No longer depends on: 1351936
Marco (B.), I don't know whether you had anything in mind, but here's a stab at it to get the ball rolling.  Seems to work OK.
Attachment #8854490 - Flags: review?(mak77) → feedback?(mak77)
Comment on attachment 8854490 [details]
Bug 1303781 - Sometimes selecting a history entry opens a Google search instead of navigating to the entry.

https://reviewboard.mozilla.org/r/126440/#review129356

My idea was to handle it the same as delayed Enter, so storing whether we got the second result for the query, and store the buffered event until that event OR a timeout, since the second result may never come. One difference would be that as soon as we get the second result we could handle it without waiting 100ms, on the other side the timeout would be larger than in your case, so in all the cases where there isn't a second result it would take longer.
The problem with your approach may be that we must guess a delay, we can't tell if 100ms is too small or too large, it also depends on the machine it is running on.
The problem with my approach is that it only handles the most common case, a single down.
I don't think one of the 2 approaches is much better than the other one, this will be a fancy hack regardless.

But, don't we risk, this way, to handle Enter before DOWN?
The problem expressed in this bug is that the user wants DOWN+ENTER to go the second result, converting DOWN+ENTER to ENTER+DOWN will probably avoid Enter on one-offs, but basically ignores DOWN. We will also need to further delay Enter if down/tab are buffered.

::: browser/base/content/urlbarBindings.xml:1745
(Diff revision 1)
>          <parameter name="aEvent"/>
>          <body><![CDATA[
> +          if (this.input.controller.matchCount <= 1 &&
> +              this._isFirstResultHeuristic &&
> +              (aEvent.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_DOWN ||
> +               aEvent.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_UP) &&

I think we should handle DOWN and TAB, not UP.
UP will move to the one offs regardless and it's quite uncommon for someone to quickly type+UP+Enter, since he should be able to predict the LAST result, that seems unlikely.

::: browser/base/content/urlbarBindings.xml:1754
(Diff revision 1)
> +              this._lastInvalidateFirstResultTime + delay - Date.now();
> +            if (remainingDelay > 0) {
> +              this._bufferedEvents = this._bufferedEvents || [];
> +              this._bufferedEvents.push(aEvent);
> +              if (!this._bufferedEventsTimer) {
> +                this._bufferedEventsTimer = setTimeout(() => {

I wonder if for any event we should cancel this timeout... My fear is that we may have a race condition where we handle a down or tab after something completely changed the expectations.

like type a letter,down,type a letter. So probably at least input should cancel it.

::: browser/base/content/urlbarBindings.xml:1756
(Diff revision 1)
> +              this._bufferedEvents = this._bufferedEvents || [];
> +              this._bufferedEvents.push(aEvent);
> +              if (!this._bufferedEventsTimer) {
> +                this._bufferedEventsTimer = setTimeout(() => {
> +                  for (let event of this._bufferedEvents) {
> +                    setTimeout(() => {

If we really need to setTimeout for each keypress, it would be better to add a comment explaining why.

::: toolkit/content/widgets/autocomplete.xml:481
(Diff revision 1)
>            if (aEvent.target.localName != "textbox")
>              return true; // Let child buttons of autocomplete take input
>  
>            // XXXpch this is so bogus...
>            if (aEvent.defaultPrevented)
> -            return false;
> +            //XXXadw defaultPrevented is true on the buffered events -- why?

Since we didn't handle the event, maybe something else handled it, I wonder if it's the input field... You may need to annotate the event somehow to let it go through, maybe just with an expando.
Fwiw this commenting moves the var cancel inside the if, but luckily it's a var!
This seems to work pretty well...  It extends the handleEnter stuff with a more generic deferred event queue.  So it also reverts back to the simpler handleEnterSearchString that we used to have, since now the delayed enter is handled by this new queue.  When the first non-heuristic result arrives, it replays the events.  It still sets up a timeout to handle the case where there aren't any non-heuristic results.
The interdiff isn't accurate since I updated my tree.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Comment on attachment 8854490 [details]
Bug 1303781 - Sometimes selecting a history entry opens a Google search instead of navigating to the entry.

https://reviewboard.mozilla.org/r/126440/#review132930

I'm not sure why, but it doesn't seem to work for me.
I'm with a debug build in a new profile, I'm on about:home. if I type "moz" the second result is Planet Mozilla. If I type "moz[down][enter]", it visits a google search.
If you cannot reproduce, let me know and I could spend some time trying to debug why that happens. I wonder if the problem is with the "this.popup.selectedIndex == 0" condition (but didn't verify that).

::: browser/base/content/urlbarBindings.xml:231
(Diff revision 2)
>          ]]></body>
>        </method>
>  
> +      <!--
> +        Search results arrive asynchronously, which means that keypresses may
> +        arrive before the first non-hueristic result and therefore not have the

typo hueristic

::: browser/base/content/urlbarBindings.xml:1236
(Diff revision 2)
>            if (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery) {
>              this.maybeCanonizeURL(event, this.value);
>              let rv = this.mController.handleEnter(false, event);
> -            this.handleEnterInstance = null;
> +            this.handleEnterSearchString = null;
>              this.popup.overrideValue = null;
>              return rv;

nit: I now figured how rv is confusing in our code base, since I'd expect it to be an nsresult... maybe it should be renamed to "handled"

::: browser/base/content/urlbarBindings.xml
(Diff revision 2)
> -              // loop where the controller sets popupOpen and re-enters here.
> -              setTimeout(() => {
> -                // Safety check: handle only if the search string didn't change.
> -                let { event, searchString } = instance;
> -                if (this.input.mController.searchString == searchString) {
> +              // Try to replay deferred key events.  If a non-heuristic result
> +              // still hasn't arrived (gotNonHeuristicResultForCurrentQuery is
> +              // still false), then this will set up a timeout in case no result
> +              // ever arrives.
> +              this.input.maybeReplayDeferredKeyEvents();
> -                  this.input.maybeCanonizeURL(event, searchString);

this maybeCanonizeURL call disappeared from _replayDeferredKeyEvents.... That doesn't look right.

::: toolkit/content/widgets/autocomplete.xml:479
(Diff revision 2)
>          <parameter name="aEvent"/>
>          <body><![CDATA[
>            if (aEvent.target.localName != "textbox")
>              return true; // Let child buttons of autocomplete take input
>  
> -          // XXXpch this is so bogus...
> +          // Re: urlbarDeferred, see the discussion in urlbarBindings.xml.

s/discussion/comment/
Attachment #8854490 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #23)
> I'm not sure why, but it doesn't seem to work for me.

I can't reproduce that.  Are you typing moz[down][enter] very quickly?  I tried slowly, quickly, and pasting moz instead of typing it so that I could be even quicker.  Hmm.

If `this.popup.selectedIndex == 0` is a problem in your case, it can't be the only problem because at the point you hit enter there's already a one-off selected and a selection in the popup.  That's already wrong.  If you repeat your steps but don't hit enter, is there a one-off selected?  (You're not pressing alt+down are you? :-)

What happens if you quickly type a single letter + down + enter?

What OS are you using?

What's the current tip of your tree?  I tried with an older tree and a fresh tree and couldn't reproduce with either.

> this maybeCanonizeURL call disappeared from _replayDeferredKeyEvents....
> That doesn't look right.

_replayDeferredKeyEvents simply calls handleEnter again with this event, so maybeCanonizeURL will be called as part of the normal (non-deferred) path in that method.  In other words, before this patch, there were two duplicate (or close to duplicate) code paths for handling enter, and now there only has to be one.  Does that make sense, or am I misunderstanding you?
Flags: needinfo?(mak77)
It sounds like this feature hasn't shipped so it's only enabled pre-beta, but there are still some negative effects even with the feature disabled.
Will one-off searches be enabled in beta 54?
status-firefox51: fix-optional → wontfix
status-firefox52: fix-optional → wontfix
status-firefox53: affected → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected
It's only enabled on Nightly, but there may be bugs even on non-Nightly.  I don't think this is one of them, though.  The plan is to enable them on non-Nightly starting in 55.
I can reproduce more easily in a non-optimized debug build. This is what I see when it happens:

onKeyPress 40 [DOWN]
this.popup.popupOpen: false (here we already bail out from defer)
this.popup.selectedIndex: -1 (moreover)
this.gotNonHeuristicResultForCurrentQuery: false (ok)

handleEnter [ENTER]
this.popup.selectedIndex: -1
this.gotResultForCurrentQuery: true

Looks like I can type and confirm even before the popup starts opening.

If instead I go slower, this is what I see:

onKeyPress 40 [DOWN]
this.popup.popupOpen: true
this.popup.selectedIndex: 0
this.gotNonHeuristicResultForCurrentQuery: true

handleEnter [ENTER]
this.popup.selectedIndex: 1
this.gotResultForCurrentQuery: true
Flags: needinfo?(mak77)
Thanks, I can reproduce on a debug non-optimized build too.  It's pretty easy when I hold down the down arrow key.  I'll try to figure out what's going on.
Actually holding the down arrow key even on an optimized build reproduces it pretty easily.  I have to hold it noticeably longer than is comfortable or what I usually do when I press a key.
I think I was thinking about it wrong.  The important thing is not whether a non-heuristic result has arrived.  It's whether there are potentially more results yet to arrive and the selected result is currently the last result.  If so, then defer the keypress.  (Unless we've waited long enough.)

This seems to work OK.  At least I can't reproduce.  But I'm having a harder time getting the problem to show up now than I was before.  One thing that's important is the timeout value.  If it's too small, or it's too small for the speed of your machine or Firefox, then you're more likely to see the problem.

I think I refreshed my tree since the last patch, so the interdiff probably won't be accurate.
Comment on attachment 8854490 [details]
Bug 1303781 - Sometimes selecting a history entry opens a Google search instead of navigating to the entry.

https://reviewboard.mozilla.org/r/126440/#review137236

::: browser/base/content/urlbarBindings.xml:214
(Diff revision 3)
>                // Reset the selected index so that nsAutoCompleteController
>                // simply closes the popup without trying to fill anything.
>                this.popup.selectedIndex = -1;
>                break;
>            }
> -          if (this.popup.popupOpen &&
> +          if (this.popup.popupOpen && !this.popup.disableKeyNavigation) {

It still doesn't seem to work for me.
To be sure I bumped the timeout to 2 seconds, since debug builds are so slow.
Based on comment 27, I thin kwe are hitting a false popupOpen.
There is nothing that tells us the popup is open by the time the user presses down.
Attachment #8854490 - Flags: review?(mak77)
Could you try this please?  It moves the popupOpen check after the defer check, so even if the popup isn't open, we'll still defer.

https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-3232cf9553938d9f571f4cd182073936354009d3/

One thing I noticed this time is that when the popup opens, it sometimes first briefly shows the contents that were present the last time the popup opened.  If the new query has fewer results than the old one, and if you hold down the down arrow key, you can reach the one-offs without going through the old results.  And then right after that, the old results are removed.  Which can give you the impression that your selection skipped some results.  I don't know whether that's part of what you're seeing (and I'm sure if it is you would have realized that anyway).
Blocks: 1303462
No longer depends on: 1303462
Comment on attachment 8854490 [details]
Bug 1303781 - Sometimes selecting a history entry opens a Google search instead of navigating to the entry.

https://reviewboard.mozilla.org/r/126440/#review143490

With the patch as-is I'm still able to easily trick the system to run a search, even in an --enable-debug --enable-optimize build.
But overall with the changes I'm suggesting below the situation is *much* improved, to a point where we can probably land and consider P3 any race conditions. In all honesty I could yet trick the system in some rare cases, but it's harder and I have a quick keyboard probably :)
Both changes below seemed to be required to improve the handling.

::: browser/base/content/urlbarBindings.xml:273
(Diff revision 4)
> +            return false;
> +          }
> +          let maxResultsRemaining =
> +            this.popup.maxResults - this.popup._matchCount;
> +          let lastResultSelected =
> +            this.popup.selectedIndex + 1 == this.popup._matchCount;

return !this.gotResultForCurrentQuery || (maxResultsRemaining > 0 && lastResultSelected);

::: browser/base/content/urlbarBindings.xml:1271
(Diff revision 4)
>              if (this.mController.handleText()) {
>                this.gotResultForCurrentQuery = false;
> +              this._searchStartDate = Date.now();
> +              this._deferredKeyEventQueue = [];
> +              if (this._deferredKeyEventTimeout) {
> +                clearTimeout(this._deferredKeyEventTimeout);

we should probably clear the timeout also in the binding destructor, so we don't try to handle something later than expected. And maybe also after a Enter? at that point we don't want to delay a keypress like down and risk to pollute the next search.

::: browser/base/content/urlbarBindings.xml:1301
(Diff revision 4)
> -          this.handleEnterInstance = {
> -            searchString: this.mController.searchString,
> -            event
> -          };
>  
>            if (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery) {

Shouldn't we also check the if there's anything deferred at the moment? it's clear that if we are going to replay a deferred queue, enter should be at the end of it.
I was thinking something like
if (!this._deferredKeyEventQueue.length &&
   (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery)) {
Attachment #8854490 - Flags: review?(mak77) → review+
(In reply to Drew Willcoxon :adw from comment #34)
> One thing I noticed this time is that when the popup opens, it sometimes
> first briefly shows the contents that were present the last time the popup
> opened. 

That's sort-of expected, to avoid flickering we don't immediately remove results, there's a bug filed about that, but this bug it not about what I see, indeed this bug should probably be reproduced while closing your eyes ;)
The point is not what the user sees, but the fact he "knows" typing "mo+down+enter" will go to planet mozilla (for example). and that's why heuristics based on what is present in the popup are not working precisely.
PS: with that I'm not excluding that showing previous results for some users may be part of the problem. So basically there are 2 different causes for a similar issue here, fixing the muscle memory problem won't completely fix the clearing problem.
See bug 1291927.
(In reply to Marco Bonardo [::mak] from comment #35)
> ::: browser/base/content/urlbarBindings.xml:273
> (Diff revision 4)
> > +            return false;
> > +          }
> > +          let maxResultsRemaining =
> > +            this.popup.maxResults - this.popup._matchCount;
> > +          let lastResultSelected =
> > +            this.popup.selectedIndex + 1 == this.popup._matchCount;
> 
> return !this.gotResultForCurrentQuery || (maxResultsRemaining > 0 &&
> lastResultSelected);

Done, but I added an early `return true` when !this.gotResultForCurrentQuery to avoid doing the unnecessary other logic.

> ::: browser/base/content/urlbarBindings.xml:1271
> (Diff revision 4)
> >              if (this.mController.handleText()) {
> >                this.gotResultForCurrentQuery = false;
> > +              this._searchStartDate = Date.now();
> > +              this._deferredKeyEventQueue = [];
> > +              if (this._deferredKeyEventTimeout) {
> > +                clearTimeout(this._deferredKeyEventTimeout);
> 
> we should probably clear the timeout also in the binding destructor, so we
> don't try to handle something later than expected.

Done

> And maybe also after a Enter? at that point we don't want to
> delay a keypress like down and risk to pollute the next search.

Hmm, I'm not sure about this.  We don't want to affect the next search, but we may still be processing the current search even after you press enter.  The result you expect may not have arrived yet when you press enter.  In that case, the timeout still needs to fire and replay your events.

Another way to look at it is, when you press enter, either we need to defer the event or we don't.  If we do, then the timeout needs to be alive after that (so that the enter can be replayed if we don't get more results).  If we don't, then there won't be a timeout at all: either all deferred events have been replayed and the timeout was cleared, or nothing has been deferred at all and no timeout was set.

> ::: browser/base/content/urlbarBindings.xml:1301
> (Diff revision 4)
> > -          this.handleEnterInstance = {
> > -            searchString: this.mController.searchString,
> > -            event
> > -          };
> >  
> >            if (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery) {
> 
> Shouldn't we also check the if there's anything deferred at the moment? it's
> clear that if we are going to replay a deferred queue, enter should be at
> the end of it.
> I was thinking something like
> if (!this._deferredKeyEventQueue.length &&
>    (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery)) {

Hmm, it couldn't hurt, so I made this change, but can handleEnter get called without going through onKeyPress?  onKeyPress will defer the enter if necessary, so I think _deferredKeyEventQueue.length would always be 0 in handleEnter.

But if this change really made a difference for you then there must be something I'm not accounting for.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e53686543601
Sometimes selecting a history entry opens a Google search instead of navigating to the entry. r=mak
That test does involve the urlbar.  I'll take a look.
I think the problem is related to the tab key.  Tab is one of the keys we defer, and the test tabs out of the urlbar.  When the test fails, it's when tabbing out of the urlbar doesn't actually tab out.
I pushed to try.  If that looks OK, I'll ask for review again since I made some non-trivial changes. :-(
I'll go ahead and ask for review again in the interest of shortening the review cycle, and try looks OK so far...

Sorry for asking again Marco, but I've made some non-trivial changes.  The main problem was that the tab key was getting deferred and not replayed.  It wasn't getting replayed because replay only happens when we get results, and when the test fires the tab event, the urlbar is actually empty.  So I fixed that by moving the replay timeout into _deferKeyEvent.  Clearly that seems right: the timeout existing should depend on whether an event has been deferred, not whether we tried to replay events.

But that still didn't fix the problem.  I made sure the tab was being replayed and reaching autocomplete's handleKeyPress.  I think the problem is that the event has defaultPrevented=true, like we saw before, so the focus manager or whatever ultimately reacts to the tab key ignores it.  That's just an educated guess though.

So instead I added a check to _shouldDeferKeyEvent specifically for this case.

Working on this made me realize that we should probably be canceling the timeout and emptying the queue when the urlbar is blurred, so this also does that.

Finally, another thing I realized is that maybeReplayDeferredKeyEvents only tried to replay one event, even if it can replay multiple events.  I don't think there should be two methods that replay events: one that's called when we get results, and one that's called when the timeout fires.  Instead, there should be a single method that tries to replay as many events as it can.
ok, looking into this.
Comment on attachment 8854490 [details]
Bug 1303781 - Sometimes selecting a history entry opens a Google search instead of navigating to the entry.

https://reviewboard.mozilla.org/r/126440/#review144472

::: browser/base/content/urlbarBindings.xml:285
(Diff revisions 5 - 6)
>            let waitedLongEnough =
>              this._searchStartDate + this._deferredKeyEventTimeoutMs < Date.now();
>            if (waitedLongEnough && !this._deferredKeyEventTimeout) {
>              return false;
>            }
> +          if (event && event.keyCode == KeyEvent.DOM_VK_TAB && !this.value) {

What if the user opened the location bar history dropdown? in that case the field is empty, but Tab should cycle through results.

::: browser/base/content/urlbarBindings.xml:297
(Diff revisions 5 - 6)
>            }
>            let maxResultsRemaining =
>              this.popup.maxResults - this.popup._matchCount;
>            let lastResultSelected =
>              this.popup.selectedIndex + 1 == this.popup._matchCount;
>            return maxResultsRemaining > 0 && lastResultSelected;

There is still one case where we constantly fail. Very often I end up here with:
this.popup.selectedIndex = -1 (looks like we didn't set the initial selection yet)
and this.popup._matchCount = 1

indeed we set selectedIndex when we open the popup, that happens after the result is added.
So basically this runs between the result addition and the popup opening.
Maybe we could change
if (!this.gotResultForCurrentQuery) {
            return true;
to
if (!this.gotResultForCurrentQuery || !this.popupOpen) {
            return true;
            
That seems to do the job for me
(In reply to Marco Bonardo [::mak] from comment #49)
> ::: browser/base/content/urlbarBindings.xml:285
> (Diff revisions 5 - 6)
> >            let waitedLongEnough =
> >              this._searchStartDate + this._deferredKeyEventTimeoutMs < Date.now();
> >            if (waitedLongEnough && !this._deferredKeyEventTimeout) {
> >              return false;
> >            }
> > +          if (event && event.keyCode == KeyEvent.DOM_VK_TAB && !this.value) {
> 
> What if the user opened the location bar history dropdown? in that case the
> field is empty, but Tab should cycle through results.

I changed this to:

if (event && event.keyCode == KeyEvent.DOM_VK_TAB && !this.popupOpen) {

i.e., check popupOpen.  I think that's what I should really be testing?

> ::: browser/base/content/urlbarBindings.xml:297
> (Diff revisions 5 - 6)
> >            }
> >            let maxResultsRemaining =
> >              this.popup.maxResults - this.popup._matchCount;
> >            let lastResultSelected =
> >              this.popup.selectedIndex + 1 == this.popup._matchCount;
> >            return maxResultsRemaining > 0 && lastResultSelected;
> 
> There is still one case where we constantly fail. Very often I end up here
> with:
> this.popup.selectedIndex = -1 (looks like we didn't set the initial
> selection yet)
> and this.popup._matchCount = 1
> 
> indeed we set selectedIndex when we open the popup, that happens after the
> result is added.
> So basically this runs between the result addition and the popup opening.
> Maybe we could change
> if (!this.gotResultForCurrentQuery) {
>             return true;
> to
> if (!this.gotResultForCurrentQuery || !this.popupOpen) {
>             return true;
>             
> That seems to do the job for me

Fixed
Try looks OK on the latest patch.
looks like you're good to go.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68fd11cad655
Sometimes selecting a history entry opens a Google search instead of navigating to the entry. r=mak
https://hg.mozilla.org/mozilla-central/rev/68fd11cad655
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1257508
I could not reproduce the initial issue on Windows 10 x64 using Nightly 52.0a1 (Build ID: 20161020030211) and Nighlty 55.0a1 (BuilD ID: 20170520030204).

Marco, could you please verify if this issue is fixed on your end on the latest Nightly 55?
Flags: needinfo?(mcastelluccio)
Yes, I can't reproduce anymore.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mcastelluccio)
I can still reproduce when there's something CPU intensive going on, e.g. a WebRTC call.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(In reply to Marco Castelluccio [:marco] from comment #58)
> I can still reproduce when there's something CPU intensive going on, e.g. a
> WebRTC call.

note that this bug is not expected to go to the result you see, it's expect to go the result you expect by muscle memory. We have a separate bug for the fact we keep showing results from a previous query. If that's the problem, we are tracking that apart.
There's also a timeout for which we can't do much, if your system is so busy to hit the timeout we'll still handle input, or we could block it forever.
(In reply to Marco Bonardo [::mak] from comment #59)
> (In reply to Marco Castelluccio [:marco] from comment #58)
> > I can still reproduce when there's something CPU intensive going on, e.g. a
> > WebRTC call.
> 
> note that this bug is not expected to go to the result you see, it's expect
> to go the result you expect by muscle memory. We have a separate bug for the
> fact we keep showing results from a previous query. If that's the problem,
> we are tracking that apart.
> There's also a timeout for which we can't do much, if your system is so busy
> to hit the timeout we'll still handle input, or we could block it forever.

What's happening is that the Google one-off button is being selected instead of the first entry in the awesomebar.
(In reply to Marco Castelluccio [:marco] from comment #60)
> What's happening is that the Google one-off button is being selected instead
> of the first entry in the awesomebar.

I assume you mean the second entry (since the first is preselected), for a search that you are sure it would return a second entry.
Is this the first urlbar usage in the session?

Would you have a chance to make a build with a larger value in _deferredKeyEventTimeoutMs than the current one? It'd be of great value understanding if it's just a timeout problem.

The times that I was able to reproduce the bug after the patch, the events were ordered as "a", ENTER, DOWN while I really thought to have pressed "a", DOWN, ENTER (so I was far too quick with keys). A good way to check this is to make handleKeyPress print out the key codes it receives (IIRC DOWN is 40, ENTER is 13).
I'd also be quite interested into seeing if bug 1291927 would make a difference, the perception of seeing results that I could potentially select, when they really don't exist, is pretty much negative.

Finally, I think that for tracking sanity we should still resolve this and file a new "Selecting a history entry may open a Google search instead of navigating to the entry, yet" bug. We are unlikely to backout these patches since they pose the basis of a future final fix, whatever it will be, plus they actually improved the situation by handling the most common cases.
Please file a new bug to track any remaining shortcomings of the current implementation.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Blocks: 1369016
(In reply to Marco Bonardo [::mak] from comment #61)
> (In reply to Marco Castelluccio [:marco] from comment #60)
> > What's happening is that the Google one-off button is being selected instead
> > of the first entry in the awesomebar.
> 
> I assume you mean the second entry (since the first is preselected), for a
> search that you are sure it would return a second entry.
> Is this the first urlbar usage in the session?

Yeah, second entry. It isn't the first urlbar usage in the session.

> Would you have a chance to make a build with a larger value in
> _deferredKeyEventTimeoutMs than the current one? It'd be of great value
> understanding if it's just a timeout problem.
>
> The times that I was able to reproduce the bug after the patch, the events
> were ordered as "a", ENTER, DOWN while I really thought to have pressed "a",
> DOWN, ENTER (so I was far too quick with keys). A good way to check this is
> to make handleKeyPress print out the key codes it receives (IIRC DOWN is 40,
> ENTER is 13).
> I'd also be quite interested into seeing if bug 1291927 would make a
> difference, the perception of seeing results that I could potentially
> select, when they really don't exist, is pretty much negative.

I'll try, but I can't reproduce this as easily as before.

> Finally, I think that for tracking sanity we should still resolve this and
> file a new "Selecting a history entry may open a Google search instead of
> navigating to the entry, yet" bug. We are unlikely to backout these patches
> since they pose the basis of a future final fix, whatever it will be, plus
> they actually improved the situation by handling the most common cases.

Agreed, filed bug 1369016 as a clone of this.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: