Bug 1043584 - fix mouseover vs. enter issue in the urlbar, r=mak

--HG--
extra : rebase_source : a44d3e897a6ae930119fff45b6babc7a0ad8bcbb

browser/base/content/test/general/browser.ini

@@ -442,6 +442,8 @@ skip-if = e10s # Bug 1093941 - Waits indefinitely for onSearchComplete
 [browser_urlbarCopying.js]
 [browser_urlbarEnter.js]
 skip-if = e10s # Bug 1093941 - used to cause obscure non-windows child process crashes on try
+[browser_urlbarEnterAfterMouseOver.js]
+skip-if = os == "linux" || e10s # Bug 1073339 - Investigate autocomplete test unreliability on Linux/e10s
 [browser_urlbarRevert.js]
 skip-if = e10s # Bug 1093941 - ESC reverted the location bar value - Got foobar, expected example.com
 [browser_urlbarSearchSingleWordNotification.js]

browser/base/content/test/general/browser_urlbarEnterAfterMouseOver.js

@@ -0,0 +1,71 @@
+function repeat(limit, func) {
+  for (let i = 0; i < limit; i++) {
+    func(i);
+  }
+}
+
+function* promiseAutoComplete(inputText) {
+  gURLBar.focus();
+  gURLBar.value = inputText.slice(0, -1);
+  EventUtils.synthesizeKey(inputText.slice(-1), {});
+  yield promiseSearchComplete();
+}
+
+function is_selected(index) {
+  is(gURLBar.popup.richlistbox.selectedIndex, index, `Item ${index + 1} should be selected`);
+}
+
+add_task(function*() {
+  registerCleanupFunction(promiseClearHistory);
+
+  yield promiseClearHistory();
+  let tabCount = gBrowser.tabs.length;
+
+  let visits = [];
+  repeat(10, i => {
+    visits.push({
+      uri: makeURI("http://example.com/autocomplete/?" + i),
+    });
+  });
+  yield PlacesTestUtils.addVisits(visits);
+
+  Services.prefs.setBoolPref("browser.urlbar.unifiedcomplete", true);
+  yield* do_test();
+  Services.prefs.setBoolPref("browser.urlbar.unifiedcomplete", false);
+  yield* do_test();
+  Services.prefs.clearUserPref("browser.urlbar.unifiedcomplete");
+});
+
+function* do_test() {
+  gBrowser.selectedTab = gBrowser.addTab("about:blank");
+  yield promiseAutoComplete("http://example.com/autocomplete/");
+
+  let popup = gURLBar.popup;
+  let results = popup.richlistbox.children;
+  is(results.length, 11, "Should get 11 results");
+
+  let initiallySelected = gURLBar.popup.richlistbox.selectedIndex;
+
+  info("Key Down to select the next item");
+  EventUtils.synthesizeKey("VK_DOWN", {});
+  is_selected(initiallySelected + 1);
+  let expectedURL = gURLBar.controller.getFinalCompleteValueAt(initiallySelected + 1);
+
+  is(gURLBar.value, gURLBar.controller.getValueAt(initiallySelected + 1),
+     "Value in the URL bar should be updated by keyboard selection");
+
+  // Verify that what we're about to do changes the selectedIndex:
+  isnot(initiallySelected + 1, 3, "Shouldn't be changing the selectedIndex to the same index we keyboard-selected.");
+
+  // Would love to use a synthetic mousemove event here, but that doesn't seem to do anything.
+  // EventUtils.synthesizeMouseAtCenter(results[3], {type: "mousemove"});
+  gURLBar.popup.richlistbox.selectedIndex = 3;
+  is_selected(3);
+
+  let autocompletePopupHidden = promisePopupHidden(gURLBar.popup);
+  let openedExpectedPage = waitForDocLoadAndStopIt(expectedURL);
+  EventUtils.synthesizeKey("VK_RETURN", {});
+  yield Promise.all([autocompletePopupHidden, openedExpectedPage]);
+
+  gBrowser.removeCurrentTab();
+}

toolkit/components/autocomplete/nsAutoCompleteController.cpp

@@ -52,7 +52,8 @@ nsAutoCompleteController::nsAutoCompleteController() :
   mSearchesOngoing(0),
   mSearchesFailed(0),
   mFirstSearchResult(false),
-  mImmediateSearchesCount(0)
+  mImmediateSearchesCount(0),
+  mCompletedSelectionIndex(-1)
 {
 }
 
@@ -122,6 +123,7 @@ nsAutoCompleteController::SetInput(nsIAutoCompleteInput *aInput)
   mSearchStatus = nsIAutoCompleteController::STATUS_NONE;
   mRowCount = 0;
   mSearchesOngoing = 0;
+  mCompletedSelectionIndex = -1;
 
   // Initialize our list of search objects
   uint32_t searchCount;
@@ -421,10 +423,12 @@ nsAutoCompleteController::HandleKeyNavigation(uint32_t aKey, bool *_retval)
             input->SetTextValue(value);
             input->SelectTextRange(value.Length(), value.Length());
           }
+          mCompletedSelectionIndex = selectedIndex;
         } else {
           // Nothing is selected, so fill in the last typed value
           input->SetTextValue(mSearchString);
           input->SelectTextRange(mSearchString.Length(), mSearchString.Length());
+          mCompletedSelectionIndex = -1;
         }
       }
     } else {
@@ -1244,17 +1248,29 @@ nsAutoCompleteController::EnterMatch(bool aIsPopupSelection)
     int32_t selectedIndex;
     popup->GetSelectedIndex(&selectedIndex);
     if (selectedIndex >= 0) {
+      nsAutoString finalValue;
       // If completeselectedindex is false or a row was selected from the popup,
-      // enter it into the textbox. If completeselectedindex is true, or
-      // EnterMatch was called via other means, for instance pressing Enter,
-      // don't fill in the value as it will have already been filled in as
-      // needed, unless the final complete value differs.
-      nsAutoString finalValue, inputValue;
-      GetResultValueAt(selectedIndex, true, finalValue);
-      input->GetTextValue(inputValue);
-      if (!completeSelection || aIsPopupSelection ||
-          !finalValue.Equals(inputValue)) {
+      // enter it into the textbox.
+      if (!completeSelection || aIsPopupSelection) {
+        GetResultValueAt(selectedIndex, true, finalValue);
         value = finalValue;
+      } else if (mCompletedSelectionIndex != -1) {
+        // If completeselectedindex is true, and EnterMatch was not invoked by
+        // mouse-clicking a match (for example the user pressed Enter),
+        // don't fill in the value as it will have already been filled in as
+        // needed, unless the selected match has a final complete value that
+        // differs from the user-facing value.
+        GetResultValueAt(mCompletedSelectionIndex, true, finalValue);
+        nsAutoString inputValue;
+        input->GetTextValue(inputValue);
+        if (!finalValue.Equals(inputValue)) {
+          value = finalValue;
+        }
+        // Note that if the user opens the popup, mouses over entries without
+        // ever selecting one with the keyboard, and then hits enter, none of
+        // the above cases will be hitt, since mouseover doesn't activate
+        // completeselectedindex and thus mCompletedSelectionIndex would be
+        // -1.
       }
     }
     else if (shouldComplete) {

toolkit/components/autocomplete/nsAutoCompleteController.h

@@ -151,6 +151,14 @@ protected:
   uint32_t mSearchesFailed;
   bool mFirstSearchResult;
   uint32_t mImmediateSearchesCount;
+  // The index of the match on the popup that was selected using the keyboard,
+  // if the completeselectedindex attribute is set.
+  // This is used to distinguish that selection (which would have been put in
+  // the input on being selected) from a moused-over selectedIndex value. This
+  // distinction is used to prevent mouse moves from inadvertently changing
+  // what happens once the user hits Enter on the keyboard.
+  // See bug 1043584 for more details.
+  int32_t  mCompletedSelectionIndex;
 };
 
 #endif /* __nsAutoCompleteController__ */

toolkit/components/autocomplete/tests/unit/test_finalCompleteValue.js

@@ -1,7 +1,3 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this file,
- * You can obtain one at http://mozilla.org/MPL/2.0/. */
-
 function AutoCompleteResult(aValues, aFinalCompleteValues) {
   this._values = aValues;
   this._finalCompleteValues = aFinalCompleteValues;

toolkit/components/autocomplete/tests/unit/test_finalCompleteValueSelectedIndex.js

@@ -0,0 +1,97 @@
+function AutoCompleteResult(aResultValues) {
+  this._values = aResultValues.map(x => x[0]);
+  this._finalCompleteValues = aResultValues.map(x => x[1]);
+}
+AutoCompleteResult.prototype = Object.create(AutoCompleteResultBase.prototype);
+
+let selectByWasCalled = false;
+function AutoCompleteInput(aSearches) {
+  this.searches = aSearches;
+  this.popup.selectedIndex = 0;
+  this.popup.selectBy = function(reverse, page) {
+    Assert.equal(selectByWasCalled, false);
+    selectByWasCalled = true;
+    Assert.equal(reverse, false);
+    Assert.equal(page, false);
+    this.selectedIndex += (reverse ? -1 : 1) * (page ? 100 : 1);
+  };
+  this.completeSelectedIndex = true;
+}
+AutoCompleteInput.prototype = Object.create(AutoCompleteInputBase.prototype);
+
+function run_test() {
+  run_next_test();
+}
+
+add_test(function test_handleEnter() {
+  let results = [
+    ["mozilla.com", "http://www.mozilla.com"],
+    ["mozilla.org", "http://www.mozilla.org"],
+  ];
+  // First check the case where we do select a value with the keyboard:
+  doSearch("moz", results, function(aController) {
+    Assert.equal(aController.input.textValue, "moz");
+    Assert.equal(aController.getFinalCompleteValueAt(0), "http://www.mozilla.com");
+    Assert.equal(aController.getFinalCompleteValueAt(1), "http://www.mozilla.org");
+
+    Assert.equal(aController.input.popup.selectedIndex, 0);
+    aController.handleKeyNavigation(Ci.nsIDOMKeyEvent.DOM_VK_DOWN);
+    Assert.equal(aController.input.popup.selectedIndex, 1);
+    // Simulate mouse interaction changing selectedIndex
+    // ie NOT keyboard interaction:
+    aController.input.popup.selectedIndex = 0;
+
+    aController.handleEnter(false);
+    // Verify that the keyboard-selected thing got inserted,
+    // and not the mouse selection:
+    Assert.equal(aController.input.textValue, "http://www.mozilla.org");
+  });
+
+  // Then the case where we do not:
+  doSearch("moz", results, function(aController) {
+    Assert.equal(aController.input.textValue, "moz");
+    Assert.equal(aController.getFinalCompleteValueAt(0), "http://www.mozilla.com");
+    Assert.equal(aController.getFinalCompleteValueAt(1), "http://www.mozilla.org");
+
+    Assert.equal(aController.input.popup.selectedIndex, 0);
+    aController.input.popupOpen = true;
+    // Simulate mouse interaction changing selectedIndex
+    // ie NOT keyboard interaction:
+    aController.input.popup.selectedIndex = 1;
+    Assert.equal(selectByWasCalled, false);
+    Assert.equal(aController.input.popup.selectedIndex, 1);
+
+    aController.handleEnter(false);
+    // Verify that the input stayed the same, because no selection was made
+    // with the keyboard:
+    Assert.equal(aController.input.textValue, "moz");
+  });
+});
+
+function doSearch(aSearchString, aResults, aOnCompleteCallback) {
+  selectByWasCalled = false;
+  let search = new AutoCompleteSearchBase(
+    "search",
+    new AutoCompleteResult(aResults)
+  );
+  registerAutoCompleteSearch(search);
+
+  let controller = Cc["@mozilla.org/autocomplete/controller;1"].
+                   getService(Ci.nsIAutoCompleteController);
+
+  // Make an AutoCompleteInput that uses our searches and confirms results.
+  let input = new AutoCompleteInput([ search.name ]);
+  input.textValue = aSearchString;
+
+  controller.input = input;
+  controller.startSearch(aSearchString);
+
+  input.onSearchComplete = function onSearchComplete() {
+    aOnCompleteCallback(controller);
+
+    // Clean up.
+    unregisterAutoCompleteSearch(search);
+    run_next_test();
+  };
+}
+

toolkit/components/autocomplete/tests/unit/xpcshell.ini

@@ -14,6 +14,7 @@ skip-if = toolkit == 'gonk'
 [test_completeDefaultIndex_casing.js]
 [test_finalCompleteValue.js]
 [test_finalCompleteValue_forceComplete.js]
+[test_finalCompleteValueSelectedIndex.js]
 [test_finalDefaultCompleteValue.js]
 [test_hiddenResult.js]
 [test_immediate_search.js]