Bug 1302472 - keyboard selected value in autocomplete popup cannot be confirmed with <enter> if completedefaultindex is true but completeselectedindex is false. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Sat, 17 Sep 2016 14:03:37 +0200
changeset 416156 478155aa8ec4f41f6c81592f0b2872fa2882f6e7
parent 416020 560b2c805bf7bebeb3ceebc495a81b2aa4c0c755
child 416157 e024f2321cbae1433d5a1f8d524a58291f654a9f
push id30045
push usermak77@bonardo.net
push dateWed, 21 Sep 2016 15:59:00 +0000
reviewersadw
bugs1302472
milestone52.0a1
Bug 1302472 - keyboard selected value in autocomplete popup cannot be confirmed with <enter> if completedefaultindex is true but completeselectedindex is false. r=adw MozReview-Commit-ID: FR4pKGZLAl2
toolkit/components/autocomplete/nsAutoCompleteController.cpp
toolkit/components/autocomplete/tests/unit/test_finalCompleteValue_defaultIndex.js
--- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -1391,25 +1391,33 @@ nsAutoCompleteController::EnterMatch(boo
     bool completeSelection;
     input->GetCompleteSelectedIndex(&completeSelection);
 
     int32_t selectedIndex;
     popup->GetSelectedIndex(&selectedIndex);
     if (selectedIndex >= 0) {
       nsAutoString inputValue;
       input->GetTextValue(inputValue);
-      bool defaultCompleted = mDefaultIndexCompleted &&
-                              inputValue.Equals(mPlaceholderCompletionString,
-                                                nsCaseInsensitiveStringComparator());
-      if (aIsPopupSelection || (!completeSelection && !defaultCompleted)) {
+      if (aIsPopupSelection || !completeSelection) {
         // We need to fill-in the value if:
-        //  * completeselectedindex is false and we didn't defaultComplete
+        //  * completeselectedindex is false
         //  * A row in the popup was confirmed
+        //
+        // TODO: This is not totally correct, cause it will also confirm
+        // a result selected with a simple mouseover, that could also have
+        // happened accidentally, maybe touching a touchpad.
+        // The reason is that autocomplete.xml sets selectedIndex on mousemove
+        // making impossible, in the !completeSelection case, to distinguish if
+        // the user wanted to confirm autoFill or the popup entry.
+        // The solution may be to change autocomplete.xml to set selectedIndex
+        // only on popupClick, but that requires changing the selection behavior.
         GetResultValueAt(selectedIndex, true, value);
-      } else if (defaultCompleted) {
+      } else if (mDefaultIndexCompleted &&
+                 inputValue.Equals(mPlaceholderCompletionString,
+                                   nsCaseInsensitiveStringComparator())) {
         // We also need to fill-in the value if the default index completion was
         // confirmed, though we cannot use the selectedIndex cause the selection
         // may have been changed by the mouse in the meanwhile.
         GetFinalDefaultCompleteValue(value);
       } 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
--- a/toolkit/components/autocomplete/tests/unit/test_finalCompleteValue_defaultIndex.js
+++ b/toolkit/components/autocomplete/tests/unit/test_finalCompleteValue_defaultIndex.js
@@ -13,17 +13,17 @@ function AutoCompleteInput(aSearches) {
 }
 AutoCompleteInput.prototype = Object.create(AutoCompleteInputBase.prototype);
 
 add_test(function test_handleEnter() {
   let results = [
     ["mozilla.com", "https://www.mozilla.com"],
     ["gomozilla.org", "http://www.gomozilla.org"],
   ];
-  doSearch("moz", results, 0, controller => {
+  doSearch("moz", results, { selectedIndex: 0 }, controller => {
     let input = controller.input;
     Assert.equal(input.textValue, "mozilla.com");
     Assert.equal(controller.getFinalCompleteValueAt(0), results[0][1]);
     Assert.equal(controller.getFinalCompleteValueAt(1), results[1][1]);
     Assert.equal(input.popup.selectedIndex, 0);
 
     controller.handleEnter(false);
     // Verify that the keyboard-selected thing got inserted,
@@ -36,40 +36,65 @@ add_test(function test_handleEnter_other
   // The popup selection may not coincide with what is filled into the input
   // field, for example if the user changed it with the mouse and then pressed
   // Enter. In such a case we should still use the inputField value and not the
   // popup selected value.
   let results = [
     ["mozilla.com", "https://www.mozilla.com"],
     ["gomozilla.org", "http://www.gomozilla.org"],
   ];
-  doSearch("moz", results, 1, controller => {
+  doSearch("moz", results, { selectedIndex: 1 }, controller => {
     let input = controller.input;
     Assert.equal(input.textValue, "mozilla.com");
     Assert.equal(controller.getFinalCompleteValueAt(0), results[0][1]);
     Assert.equal(controller.getFinalCompleteValueAt(1), results[1][1]);
     Assert.equal(input.popup.selectedIndex, 1);
 
     controller.handleEnter(false);
     // Verify that the keyboard-selected thing got inserted,
     // and not the mouse selection:
     Assert.equal(controller.input.textValue, "https://www.mozilla.com");
   });
 });
 
-function doSearch(aSearchString, aResults, aSelectedIndex, aOnCompleteCallback) {
+add_test(function test_handleEnter_otherSelected_nocompleteselectedindex() {
+  let results = [
+    ["mozilla.com", "https://www.mozilla.com"],
+    ["gomozilla.org", "http://www.gomozilla.org"],
+  ];
+  doSearch("moz", results, { selectedIndex: 1,
+                             completeSelectedIndex: false }, controller => {
+    let input = controller.input;
+    Assert.equal(input.textValue, "mozilla.com");
+    Assert.equal(controller.getFinalCompleteValueAt(0), results[0][1]);
+    Assert.equal(controller.getFinalCompleteValueAt(1), results[1][1]);
+    Assert.equal(input.popup.selectedIndex, 1);
+
+    controller.handleEnter(false);
+    // Verify that the keyboard-selected result is inserted, not the
+    // defaultComplete.
+    Assert.equal(controller.input.textValue, "http://www.gomozilla.org");
+  });
+});
+
+function doSearch(aSearchString, aResults, aOptions, aOnCompleteCallback) {
   let search = new AutoCompleteSearchBase(
     "search",
     new AutoCompleteResult(aResults)
   );
   registerAutoCompleteSearch(search);
 
   let input = new AutoCompleteInput([ search.name ]);
   input.textValue = aSearchString;
-  input.popup.selectedIndex = aSelectedIndex;
+  if ("selectedIndex" in aOptions) {
+    input.popup.selectedIndex = aOptions.selectedIndex;
+  }
+  if ("completeSelectedIndex" in aOptions) {
+    input.completeSelectedIndex = aOptions.completeSelectedIndex;
+  }
   // Needed for defaultIndex completion.
   input.selectTextRange(aSearchString.length, aSearchString.length);
 
   let controller = Cc["@mozilla.org/autocomplete/controller;1"].
                    getService(Ci.nsIAutoCompleteController);
   controller.input = input;
   controller.startSearch(aSearchString);