337386 - search suggestion autocomplete latency/delay can make search slow to respond or seem broken

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

If network latency between the client and the search suggestion data centre is high (or if that data centre is otherwise unavailable) then typing in the suggests box is sluggish, and clicking the "search" button or hitting enter fails to actually send the search query. After a few seconds, the suggestion terms pop into view and the search continues. Seems to only happen once a session. If needed, I can research more detailed STR.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Resummarizing; mconnor informs me that network latency might not be the culprit, but rather our own autocomplete code might be.

Comment 2

[Mike Connor [:mconnor]]

This seems to be the oddball autocomplete behaviour Ben's comments in the suggest code referenced.  Will try reimplementing his enter key handler.

Comment 3

[Wayne Woods]

Seeing this on Win XP trunk builds as well. And not only once per session, but frequently. At first I thought it was just that the search bar no longer responded to the enter key, but now I see it's just very slow.

It seems like the more complex the search term (especially gibberish), the more likely that it's going to be delayed or fail.

Also adding to summary to make it easier to find.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

*** Bug 338579 has been marked as a duplicate of this bug. ***

Comment 5

[:Gavin Sharp [email: [e-mail]]]

*** Bug 340030 has been marked as a duplicate of this bug. ***

Comment 6

[:Gavin Sharp [email: [e-mail]]]

*** Bug 342355 has been marked as a duplicate of this bug. ***

Comment 7

[:Gavin Sharp [email: [e-mail]]]

*** Bug 343045 has been marked as a duplicate of this bug. ***

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

From the original version of suggest.js (pre-release, 2005 - i.e. not the one that was released earlier):

  /** 
   * Work around a KeyEvent bug in Firefox 1.0's AutoComplete implementation.
   * The AutoComplete widget in Firefox 1.0 works as follows:
   * If the user starts typing some text, the AutoComplete Search operation is
   * initiated. If the user presses ENTER before the search is complete, the
   * search continues and nothing happens but the fact that they did is noted
   * so that when the search is complete, navigation can occur automatically 
   * rather than showing results in a popup. 
   *
   * So, rather than invoking the ENTER-handler immediately, the VK_RETURN
   * KeyEvent is cached on the AutoComplete textbox until the search is 
   * complete, then the cached event is passed through to the async handler.
   * Unfortunately, the nsEvent referred to by the cached DOMEvent is stack
   * allocated, and the pointer held by the DOMEvent will point to random
   * memory after event dispatch is complete. The effect as far as the user
   * is concerned in this case is that really weird stuff can happen.
   *
   * The solution is to ignore the AutoComplete widget's odd behavior of
   * waiting for a search to complete before processing the result of a
   * user's Enter keypress and simply start loading the URL or search 
   * query right away - the user wanted it now, not at some later time after
   * the search result returns. To do this we must handle enter key presses.
   *
   * See https://bugzilla.mozilla.org/show_bug.cgi?id=281859 for more info.
   *
   * TODO(beng): special-case this for affected versions once the bug is fixed
   *             in an official release.
   * @private
   */
  _initEventHack: function() {
    var searchbar = document.getElementById("searchbar");
    var urlbar = document.getElementById("urlbar");

    var self = this;
    /** 
     * Replacement for the AutoComplete textbox's onTextEntered function.
     * Required to override default async Enter Key Event handler. We must
     * replace this method because some conditions cause it to be called
     * and as a result spurious load events fired with the trashed 
     * memory. 
     * @return true to tell any handling code that everything is swell.
     */
    function onTextEntered() {
      this.mEnterEvent = null;
      return true;
    }
    searchbar.onTextEntered = onTextEntered;
    urlbar.onTextEntered = onTextEntered;
    searchbar.addEventListener("keypress", 
                               function(event) { self._onFieldKeyPress(event); },
                               false);
    urlbar.addEventListener("keypress", 
                            function(event) { self._onFieldKeyPress(event); },
                            false);
  },
  
  /** 
   * Handle VK_RETURN KeyEvents in the Location Bar and Search Bar by loading
   * the result of the user's input and stopping any active searches immediately.
   * @param event nsIDOMKeyEvent representing the KeyEvent
   * @private
   */
  _onFieldKeyPress: function(event) {
    if (event.keyCode != KeyEvent.DOM_VK_RETURN)
      return;
    var field = event.target;
    // Identify the search service being used by this autocomplete field and 
    // stop any pending searches. 
    var contractIDKey = field.getAttribute("autocompletesearch")
    var contractID = this._searchServiceContractID + contractIDKey;
    var searchService = Components.classes[contractID]
                                  .getService(Components.interfaces.nsIAutoCompleteSearch);
    field.fireEvent("textentered", event);
    searchService.stopSearch();
  },
  

Mysteriously in my trials this problem disappeared so I removed the code. 

A real fix in the context of firefox would probably get rid of this whole mEnterEvent nonsense entirely and just fire the textentered event from the keypress handler. 

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I was under the impression that bug 339697 fixed this, but I guess not. Should bug 281859 be reopened?

Comment 10

[Stefan Zwanenburg]

To me, it looks like it only happens when no suggestions list shows up. If I indeed search for something uncommon, no suggestions will come up, and I won't be able to press enter to actually search. Though it works just fine when the suggestionslist has shown up at least once for the current search.

Comment 11

[Mike Connor [:mconnor]]

Attachment: ben's hack, better-integrated

been running with this patch on my mac build for a while, haven't been able to reproduce with it.  There's some degree of suckage here, but the hack seems lower-risk than mucking with autocomplete's behaviour on branch.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 228103 [details] [diff] [review]
ben's hack, better-integrated

r=[e-mail]

Comment 13

[:Gavin Sharp [email: [e-mail]]]

I landed this on the trunk for mconnor, since he's away.
mozilla/browser/components/search/content/search.xml 	1.76

Comment 14

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/content/search.xml 	1.37.2.43

Comment 15

[Haudy Kazemi]

It think this is still happening on Mozilla/5.0 (Windows;;; en-US; rv:1.8.1a3) Gecko/20060707 BonEcho/2.0a3 ID:2006070704
I'll report again if I can confirm it in the 20060708 nightly.

There is more discussion here:
http://forums.mozillazine.org/viewtopic.php?t=433818

Comment 16

[Jesse Ruderman]

I'm still experiencing this problem (pressing Enter in the search bar sometimes does nothing) using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060707 Minefield/3.0a1.

Comment 17

[Asaf Romano (gone)]

Testing this, the event handler isn't even reached.

Comment 18

[Asaf Romano (gone)]

Attachment: Switch to VK_RETURN

Gecko passes both the return key and the enter key are passed as VK_RETURN.

Comment 19

[Asaf Romano (gone)]

s/are passes/

Comment 20

[(not reading, please use [e-mail] instead)]

I'm hitting this bug still, too.  I'll test out asaf's patch and confirm that on win32 it fixes the problem.

just some questions:

> Gecko passes both the return key and the enter key are passed as VK_RETURN

is this true for both the enter and return key on the mac, too?

if so, what does that mean that lines of code like:

http://lxr.mozilla.org/mozilla1.8/source/mail/extensions/newsblog/content/feed-subscriptions.js#478
and
http://lxr.mozilla.org/mozilla1.8/source/extensions/xforms/resources/content/select1.xml#186

Should search.xml handle both, or should select1.xml and feed-subscriptions.js be fixed?

Comment 21

[Asaf Romano (gone)]

Yes, we're always passing DOM_VK_RETURN, this is a long-standing issue, see bug 105990.

As for the cases you've mentioned, we just never hit them, this is a very common mistake (probably due to the constant names).

Comment 22

[Asaf Romano (gone)]

trunk: mozilla/browser/components/search/content/search.xml 1.77

Comment 23

[(not reading, please use [e-mail] instead)]

asaf, thanks for the explanation and background on VK_ENTER / VK_RETURN.

> I'll test out asaf's patch and confirm that on win32 it fixes the problem.

update:  I haven't seen this bug since applying asaf's patch to my bonecho tree on windows.

for my own benefit, I've confirmed that on win32 the VK_RETURN handler gets hit (for both enter and return) whereas the VK_ENTER doesn't.

in case mtschrep is do approvals today, I think this is one we'd definitely want for b1.  hopefully, it is not too late.

Comment 24

[gene c]

Its NOT fixed in this version best I can tell:

  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060709 BonEcho/2.0b1 ID:2006070915

Comment 25

[Mike Schroepfer]

Comment on attachment 228594 [details] [diff] [review]
Switch to VK_RETURN

Figuring this is a low-risk high impact change for the b1 respin.

Comment 26

[Asaf Romano (gone)]

1.8:
Checking in browser/components/search/content/search.xml;
/cvsroot/mozilla/browser/components/search/content/search.xml,v  <--  search.xml
new revision: 1.37.2.44; previous revision: 1.37.2.43
done

Comment 27

[Tracy Walker [:tracy]]

verified with recent builds on Windows and Mac

Comment 28

[:Gavin Sharp [email: [e-mail]]]

*** Bug 338717 has been marked as a duplicate of this bug. ***

Comment 29

[Dave Townsend [:mossop]]

*** Bug 345191 has been marked as a duplicate of this bug. ***

Comment 30

[:Gavin Sharp [email: [e-mail]]]

I've backed out the patch for this bug in bug 344189, to try another solution. If people see this bug again (enter not working in the search bar) in tomorrow's builds (or any build with that change in it) please comment in bug 344189.

Comment 31

[Ger Teunis]

Tip for the users: when the search doesn't start with the enter key, press the down arrow on you keyboard, then the search will start.