Bug 1301093 - Part 3: fix delayed handleEnter regression. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 13 Sep 2016 09:35:00 +0200
changeset 413740 7b544bfe695cd6c3a19326c71cf603b265213963
parent 413739 b1175292d466c9d00f04ff57bfbe0522c2f7e062
child 414527 b957bf6eca2b2387a6c57bc4993a0766e660e06a
push id29482
push usermak77@bonardo.net
push dateWed, 14 Sep 2016 18:58:13 +0000
reviewersadw
bugs1301093
milestone51.0a1
Bug 1301093 - Part 3: fix delayed handleEnter regression. r=adw MozReview-Commit-ID: 1SZeMUtLjNg
browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
browser/base/content/test/urlbar/browser_canonizeURL.js
browser/base/content/test/urlbar/head.js
browser/base/content/urlbarBindings.xml
toolkit/components/autocomplete/nsAutoCompleteController.cpp
toolkit/content/widgets/autocomplete.xml
--- a/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
+++ b/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
@@ -36,8 +36,55 @@ add_task(function* test_sametext() {
   event.initEvent("input", true, true);
   gURLBar.dispatchEvent(event);
   EventUtils.synthesizeKey("VK_RETURN", {});
 
   info("wait for the page to load");
   yield BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
                                        false, "http://example.com/");
 });
+
+add_task(function* test_after_empty_search() {
+  yield promiseAutocompleteResultPopup("");
+  gURLBar.focus();
+  gURLBar.value = "e";
+  EventUtils.synthesizeKey("x", {});
+  EventUtils.synthesizeKey("VK_RETURN", {});
+
+  info("wait for the page to load");
+  yield BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
+                                       false, "http://example.com/");
+});
+
+add_task(function* test_disabled_ac() {
+  // Disable autocomplete.
+  let suggestHistory = Preferences.get("browser.urlbar.suggest.history");
+  Preferences.set("browser.urlbar.suggest.history", false);
+  let suggestBookmarks = Preferences.get("browser.urlbar.suggest.bookmark");
+  Preferences.set("browser.urlbar.suggest.bookmark", false);
+  let suggestOpenPages = Preferences.get("browser.urlbar.suggest.openpage");
+  Preferences.set("browser.urlbar.suggest.openpages", false);
+
+  Services.search.addEngineWithDetails("MozSearch", "", "", "", "GET",
+                                       "http://example.com/?q={searchTerms}");
+  let engine = Services.search.getEngineByName("MozSearch");
+  let originalEngine = Services.search.currentEngine;
+  Services.search.currentEngine = engine;
+
+  registerCleanupFunction(function* () {
+    Preferences.set("browser.urlbar.suggest.history", suggestHistory);
+    Preferences.set("browser.urlbar.suggest.bookmark", suggestBookmarks);
+    Preferences.set("browser.urlbar.suggest.openpage", suggestOpenPages);
+
+    Services.search.currentEngine = originalEngine;
+    let engine = Services.search.getEngineByName("MozSearch");
+    Services.search.removeEngine(engine);
+  });
+
+  gURLBar.focus();
+  gURLBar.value = "e";
+  EventUtils.synthesizeKey("x", {});
+  EventUtils.synthesizeKey("VK_RETURN", {});
+
+  info("wait for the page to load");
+  yield BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
+                                       false, "http://example.com/?q=ex");
+});
--- a/browser/base/content/test/urlbar/browser_canonizeURL.js
+++ b/browser/base/content/test/urlbar/browser_canonizeURL.js
@@ -11,16 +11,23 @@ var pairs = [
   ["example.foo/bar ", "http://example.foo/bar"],
   ["1.1.1.1", "http://1.1.1.1/"],
   ["ftp://example", "ftp://example/"],
   ["ftp.example.bar", "http://ftp.example.bar/"],
   ["ex ample", Services.search.defaultEngine.getSubmission("ex ample", null, "keyword").uri.spec],
 ];
 
 add_task(function*() {
+  // Disable autoFill for this test, since it could mess up the results.
+  let autoFill = Preferences.get("browser.urlbar.autoFill");
+  Preferences.set("browser.urlbar.autoFill", false);
+  registerCleanupFunction(() => {
+    Preferences.set("browser.urlbar.autoFill", autoFill);
+  });
+
   for (let [inputValue, expectedURL] of pairs) {
     let focusEventPromise = BrowserTestUtils.waitForEvent(gURLBar, "focus");
     let messagePromise = BrowserTestUtils.waitForMessage(gBrowser.selectedBrowser.messageManager,
                                                          "browser_canonizeURL:start");
 
     let stoppedLoadPromise = ContentTask.spawn(gBrowser.selectedBrowser, [inputValue, expectedURL],
       function([inputValue, expectedURL]) {
         return new Promise(resolve => {
--- a/browser/base/content/test/urlbar/head.js
+++ b/browser/base/content/test/urlbar/head.js
@@ -5,16 +5,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
   "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
   "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
   "resource://testing-common/PlacesTestUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TabCrashHandler",
   "resource:///modules/ContentCrashHandlers.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
+  "resource://gre/modules/Preferences.jsm");
 
 function waitForCondition(condition, nextTest, errorMsg, retryTimes) {
   retryTimes = typeof retryTimes !== 'undefined' ?  retryTimes : 30;
   var tries = 0;
   var interval = setInterval(function() {
     if (tries >= retryTimes) {
       ok(false, errorMsg);
       moveOn();
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -127,17 +127,17 @@ file, You can obtain one at http://mozil
       <field name="_value">""</field>
       <field name="gotResultForCurrentQuery">false</field>
 
       <!--
         This is set around HandleHenter so it can be used in handleCommand.
         It is also used to track whether we must handle a delayed handleEnter,
         by checking if it has been cleared.
       -->
-      <field name="searchStringOnHandleEnter">""</field>
+      <field name="handleEnterInstance">null</field>
 
       <!--
         For performance reasons we want to limit the size of the text runs we
         build and show to the user.
       -->
       <field name="textRunsMaxLen">255</field>
 
       <!--
@@ -388,16 +388,20 @@ file, You can obtain one at http://mozil
                              event &&
                              event.altKey &&
                              !isTabEmpty(gBrowser.selectedTab);
               where = altEnter ? "tab" : "current";
             }
           }
 
           let url = this.value;
+          if (!url) {
+            return;
+          }
+
           let mayInheritPrincipal = false;
           let postData = null;
           let lastLocationChange = gBrowser.selectedBrowser.lastLocationChange;
           let matchLastLocationChange = true;
 
           let action = this._parseActionUrl(url);
           if (action) {
             switch (action.type) {
@@ -453,35 +457,34 @@ file, You can obtain one at http://mozil
                 this._recordSearchEngineLoad(selectedOneOff.engine, value,
                                              event, where, openUILinkParams);
               this._loadURL(url, postData, where, openUILinkParams,
                             matchLastLocationChange, mayInheritPrincipal);
               return;
             }
           }
 
-          this._canonizeURL(event, response => {
-            [url, postData, mayInheritPrincipal] = response;
+          url = this._canonizeURL(event, url);
+          getShortcutOrURIAndPostData(url).then(({url, postData, mayInheritPrincipal}) => {
             if (url) {
               matchLastLocationChange =
-                lastLocationChange ==
-                gBrowser.selectedBrowser.lastLocationChange;
+                lastLocationChange == gBrowser.selectedBrowser.lastLocationChange;
               this._loadURL(url, postData, where, openUILinkParams,
                             matchLastLocationChange, mayInheritPrincipal);
             }
           });
         ]]></body>
       </method>
 
       <property name="oneOffSearchQuery">
         <getter><![CDATA[
           // this.textValue may be an autofilled string.  Search only with the
           // portion that the user typed, if any, by preferring the autocomplete
-          // controller's searchString (including searchStringOnHandleEnter).
-          return this.searchStringOnHandleEnter ||
+          // controller's searchString (including handleEnterInstance.searchString).
+          return (this.handleEnterInstance && this.handleEnterInstance.searchString) ||
                  this.mController.searchString ||
                  this.textValue;
         ]]></getter>
       </property>
 
       <method name="_loadURL">
         <parameter name="url"/>
         <parameter name="postData"/>
@@ -568,23 +571,19 @@ file, You can obtain one at http://mozil
               .maybeRecordTelemetry(event, openUILinkWhere, openUILinkParams);
           let submission = engine.getSubmission(query, null, "keyword");
           return [submission.uri.spec, submission.postData];
         ]]></body>
       </method>
 
       <method name="_canonizeURL">
         <parameter name="aTriggeringEvent"/>
-        <parameter name="aCallback"/>
+        <parameter name="aUrl"/>
         <body><![CDATA[
-          var url = this.value;
-          if (!url) {
-            aCallback(["", null, false]);
-            return;
-          }
+          let url = aUrl;
 
           // Only add the suffix when the URL bar value isn't already "URL-like",
           // and only if we get a keyboard event, to match user expectations.
           if (/^\s*[^.:\/\s]+(?:\/.*|\s*)$/i.test(url) &&
               (aTriggeringEvent instanceof KeyEvent)) {
             let accel = this.AppConstants.platform == "macosx" ?
                         aTriggeringEvent.metaKey :
                         aTriggeringEvent.ctrlKey;
@@ -625,19 +624,17 @@ file, You can obtain one at http://mozil
               } else {
                 url = url + suffix;
               }
 
               url = "http://www." + url;
             }
           }
 
-          getShortcutOrURIAndPostData(url).then(data => {
-            aCallback([data.url, data.postData, data.mayInheritPrincipal]);
-          });
+          return url;
         ]]></body>
       </method>
 
       <field name="_contentIsCropped">false</field>
 
       <method name="_initURLTooltip">
         <body><![CDATA[
           if (this.focused || !this._contentIsCropped)
@@ -1039,21 +1036,24 @@ file, You can obtain one at http://mozil
           // However, if the default result is automatically selected, we
           // ensure that it corresponds to the current input.
 
           // Store the current search string so it can be used in
           // handleCommand, which will be called as a result of
           // mController.handleEnter().
           // Note this is also used to detect if we should perform a delayed
           // handleEnter, in such a case it won't have been cleared.
-          this.searchStringOnHandleEnter = this.mController.searchString;
+          this.handleEnterInstance = {
+            searchString: this.mController.searchString,
+            event: event
+          };
 
           if (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery) {
             let rv = this.mController.handleEnter(false, event);
-            this.searchStringOnHandleEnter = "";
+            this.handleEnterInstance = null;
             return rv;
           }
 
           return true;
         ]]></body>
       </method>
 
       <method name="handleDelete">
@@ -1758,45 +1758,58 @@ file, You can obtain one at http://mozil
       </method>
 
       <method name="onResultsAdded">
         <body>
           <![CDATA[
             // If nothing is selected yet, select the first result if it is a
             // pre-selected "heuristic" result.  (See UnifiedComplete.js.)
             if (this.selectedIndex == -1 && this._isFirstResultHeuristic) {
-              // Don't handle this as a user-initiated action.
-              this._ignoreNextSelect = true;
-
               // Don't fire DOMMenuItemActive so that screen readers still see
               // the input as being focused.
               this.richlistbox.suppressMenuItemEvent = true;
-
               this.selectedIndex = 0;
               this.richlistbox.suppressMenuItemEvent = false;
-              this._ignoreNextSelect = false;
             }
 
             this.input.gotResultForCurrentQuery = true;
 
             // Check if we should perform a delayed handleEnter.
-            if (this.input.searchStringOnHandleEnter) {
+            if (this.input.handleEnterInstance) {
               try {
                 // Safety check: handle only if the search string didn't change.
-                if (this.input.mController.searchString == this.input.searchStringOnHandleEnter) {
-                  this.input.mController.handleEnter(false);
+                let instance = this.input.handleEnterInstance;
+                if (this.input.mController.searchString == instance.searchString) {
+                  this.input.mController.handleEnter(false, instance.event);
                 }
               } finally {
-                this.input.searchStringOnHandleEnter = "";
+                this.input.handleEnterInstance = null;
               }
             }
           ]]>
         </body>
       </method>
 
+      <method name="_onSearchBegin">
+        <body><![CDATA[
+          // Set the selected index to 0 (heuristic) until a result comes back
+          // and we can evaluate it better.
+          //
+          // This is required to properly manage delayed handleEnter:
+          // 1. if a search starts we set selectedIndex to 0 here, and it will
+          //    be updated by onResultsAdded. Since selectedIndex is 0,
+          //    handleEnter will delay the action if a result didn't arrive yet.
+          // 2. if a search doesn't start (for example if autocomplete is
+          //    disabled), this won't be called, and the selectedIndex will be
+          //    the default -1 value. Then handleEnter will know it should not
+          //    delay the action, cause a result wont't ever arrive.
+          this.selectedIndex = 0;
+        ]]></body>
+      </method>
+
     </implementation>
     <handlers>
 
       <handler event="SelectedOneOffButtonChanged"><![CDATA[
         this._selectedOneOffChanged();
       ]]></handler>
 
       <handler event="mousedown"><![CDATA[
--- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -1626,16 +1626,20 @@ nsAutoCompleteController::ProcessResult(
       uint32_t delta = totalMatchCount - oldRowCount;
 
       mRowCount += delta;
       if (mTree) {
         mTree->RowCountChanged(oldRowCount, delta);
       }
     }
 
+    // Try to autocomplete the default index for this search.
+    // Do this before invalidating so the binding knows about it.
+    CompleteDefaultIndex(aSearchIndex);
+
     // Refresh the popup view to display the new search results
     nsCOMPtr<nsIAutoCompletePopup> popup;
     input->GetPopup(getter_AddRefs(popup));
     NS_ENSURE_TRUE(popup != nullptr, NS_ERROR_FAILURE);
     popup->Invalidate(nsIAutoCompletePopup::INVALIDATE_REASON_NEW_RESULT);
 
     uint32_t minResults;
     input->GetMinResultsForPopup(&minResults);
@@ -1643,20 +1647,18 @@ nsAutoCompleteController::ProcessResult(
     // Make sure the popup is open, if necessary, since we now have at least one
     // search result ready to display. Don't force the popup closed if we might
     // get results in the future to avoid unnecessarily canceling searches.
     if (mRowCount || !minResults) {
       OpenPopup();
     } else if (mSearchesOngoing == 0) {
       ClosePopup();
     }
-  }
-
-  if (searchResult == nsIAutoCompleteResult::RESULT_SUCCESS ||
-      searchResult == nsIAutoCompleteResult::RESULT_SUCCESS_ONGOING) {
+  } else if (searchResult == nsIAutoCompleteResult::RESULT_SUCCESS ||
+             searchResult == nsIAutoCompleteResult::RESULT_SUCCESS_ONGOING) {
     // Try to autocomplete the default index for this search.
     CompleteDefaultIndex(aSearchIndex);
   }
 
   return NS_OK;
 }
 
 nsresult
@@ -1735,20 +1737,21 @@ nsAutoCompleteController::CompleteDefaul
     return NS_OK;
 
   bool shouldComplete;
   input->GetCompleteDefaultIndex(&shouldComplete);
   if (!shouldComplete)
     return NS_OK;
 
   nsAutoString resultValue;
-  if (NS_SUCCEEDED(GetDefaultCompleteValue(aResultIndex, true, resultValue)))
+  if (NS_SUCCEEDED(GetDefaultCompleteValue(aResultIndex, true, resultValue))) {
     CompleteValue(resultValue);
 
-  mDefaultIndexCompleted = true;
+    mDefaultIndexCompleted = true;
+  }
 
   return NS_OK;
 }
 
 nsresult
 nsAutoCompleteController::GetDefaultCompleteResult(int32_t aResultIndex,
                                                    nsIAutoCompleteResult** _result,
                                                    int32_t* _defaultIndex)
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -1045,16 +1045,20 @@ extends="chrome://global/content/binding
           return val;
         ]]>
         </setter>
       </property>
 
       <method name="onSearchBegin">
         <body><![CDATA[
           this.richlistbox.mouseSelectedIndex = -1;
+
+          if (typeof this._onSearchBegin == "function") {
+            this._onSearchBegin();
+          }
         ]]></body>
       </method>
 
       <method name="openAutocompletePopup">
         <parameter name="aInput"/>
         <parameter name="aElement"/>
         <body>
           <![CDATA[