Bug 1472080 - Make Enter always autocomplete the selected result; r=bgrins. draft
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Tue, 17 Jul 2018 09:32:30 +0200
changeset 819308 5f62365ffaa46e12997d81bb8fe71c79f8178eb7
parent 819215 547144f5596c1a146b208d68d93950a6313080ca
push id116504
push userbmo:nchevobbe@mozilla.com
push dateTue, 17 Jul 2018 15:42:38 +0000
reviewersbgrins
bugs1472080
milestone63.0a1
Bug 1472080 - Make Enter always autocomplete the selected result; r=bgrins. This patch ensure that we have a consistent behavior when the autocomplete popup is open and the user hit Enter. We remove the autocompletePopupNavigated property which is now useless. We also make sure that we always show the popup when we have at least one result (previously we needed to have 2 results to make it visible). This is again for consistency sake and making it more obvious for the user what will happen when hitting Enter or Tab. Tests are modified to reflect these new behaviors. MozReview-Commit-ID: 8QgoeU6DNk6
devtools/client/webconsole/components/JSTerm.js
devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_return_key_no_selection.js
devtools/client/webconsole/test/mochitest/browser_jsterm_completion.js
--- a/devtools/client/webconsole/components/JSTerm.js
+++ b/devtools/client/webconsole/components/JSTerm.js
@@ -124,24 +124,16 @@ class JSTerm extends Component {
     this._lastFrameActorId = null;
 
     /**
      * Last input value.
      * @type string
      */
     this.lastInputValue = "";
 
-    /**
-     * Tells if the autocomplete popup was navigated since the last open.
-     *
-     * @private
-     * @type boolean
-     */
-    this._autocompletePopupNavigated = false;
-
     this.autocompletePopup = null;
     this.inputNode = null;
     this.completeNode = null;
 
     this.COMPLETE_FORWARD = 0;
     this.COMPLETE_BACKWARD = 1;
     this.COMPLETE_HINT_ONLY = 2;
     this.COMPLETE_PAGEUP = 3;
@@ -187,19 +179,19 @@ class JSTerm extends Component {
               if (
                 !this.autocompletePopup.isOpen &&
                 !Debugger.isCompilableUnit(this.getInputValue())
               ) {
                 // incomplete statement
                 return "CodeMirror.Pass";
               }
 
-              if (this._autocompletePopupNavigated &&
-                this.autocompletePopup.isOpen &&
-                this.autocompletePopup.selectedIndex > -1
+              if (
+                this.autocompletePopup.isOpen
+                && this.autocompletePopup.selectedIndex > -1
               ) {
                 return this.acceptProposedCompletion();
               }
 
               this.execute();
               return null;
             },
 
@@ -226,36 +218,30 @@ class JSTerm extends Component {
               // Input is not empty and some text is selected, let the editor handle this.
               return true;
             },
 
             "Up": () => {
               let inputUpdated;
               if (this.autocompletePopup.isOpen) {
                 inputUpdated = this.complete(this.COMPLETE_BACKWARD);
-                if (inputUpdated) {
-                  this._autocompletePopupNavigated = true;
-                }
               } else if (this.canCaretGoPrevious()) {
                 inputUpdated = this.historyPeruse(HISTORY_BACK);
               }
 
               if (!inputUpdated) {
                 return "CodeMirror.Pass";
               }
               return null;
             },
 
             "Down": () => {
               let inputUpdated;
               if (this.autocompletePopup.isOpen) {
                 inputUpdated = this.complete(this.COMPLETE_FORWARD);
-                if (inputUpdated) {
-                  this._autocompletePopupNavigated = true;
-                }
               } else if (this.canCaretGoNext()) {
                 inputUpdated = this.historyPeruse(HISTORY_FORWARD);
               }
 
               if (!inputUpdated) {
                 return "CodeMirror.Pass";
               }
               return null;
@@ -266,22 +252,19 @@ class JSTerm extends Component {
                 this.clearCompletion();
               }
               return "CodeMirror.Pass";
             },
 
             "Right": () => {
               const haveSuggestion =
                 this.autocompletePopup.isOpen || this.lastCompletion.value;
-              const useCompletion =
-                this.canCaretGoNext() || this._autocompletePopupNavigated;
 
               if (
                 haveSuggestion &&
-                useCompletion &&
                 this.complete(this.COMPLETE_HINT_ONLY) &&
                 this.lastCompletion.value &&
                 this.acceptProposedCompletion()
               ) {
                 return null;
               }
 
               if (this.autocompletePopup.isOpen) {
@@ -329,30 +312,26 @@ class JSTerm extends Component {
                 return null;
               }
 
               return "CodeMirror.Pass";
             },
 
             "PageUp": () => {
               if (this.autocompletePopup.isOpen) {
-                if (this.complete(this.COMPLETE_PAGEUP)) {
-                  this._autocompletePopupNavigated = true;
-                }
+                this.complete(this.COMPLETE_PAGEUP);
                 return null;
               }
 
               return "CodeMirror.Pass";
             },
 
             "PageDown": () => {
               if (this.autocompletePopup.isOpen) {
-                if (this.complete(this.COMPLETE_PAGEDOWN)) {
-                  this._autocompletePopupNavigated = true;
-                }
+                this.complete(this.COMPLETE_PAGEDOWN);
                 return null;
               }
 
               return "CodeMirror.Pass";
             },
 
             "Home": () => {
               if (this.autocompletePopup.isOpen) {
@@ -840,76 +819,64 @@ class JSTerm extends Component {
         if (this.autocompletePopup.isOpen) {
           this.clearCompletion();
           event.preventDefault();
           event.stopPropagation();
         }
         break;
 
       case KeyCodes.DOM_VK_RETURN:
-        if (this._autocompletePopupNavigated &&
+        if (
             this.autocompletePopup.isOpen &&
             this.autocompletePopup.selectedIndex > -1) {
           this.acceptProposedCompletion();
         } else {
           this.execute();
         }
         event.preventDefault();
         break;
 
       case KeyCodes.DOM_VK_UP:
         if (this.autocompletePopup.isOpen) {
           inputUpdated = this.complete(this.COMPLETE_BACKWARD);
-          if (inputUpdated) {
-            this._autocompletePopupNavigated = true;
-          }
         } else if (this.canCaretGoPrevious()) {
           inputUpdated = this.historyPeruse(HISTORY_BACK);
         }
         if (inputUpdated) {
           event.preventDefault();
         }
         break;
 
       case KeyCodes.DOM_VK_DOWN:
         if (this.autocompletePopup.isOpen) {
           inputUpdated = this.complete(this.COMPLETE_FORWARD);
-          if (inputUpdated) {
-            this._autocompletePopupNavigated = true;
-          }
         } else if (this.canCaretGoNext()) {
           inputUpdated = this.historyPeruse(HISTORY_FORWARD);
         }
         if (inputUpdated) {
           event.preventDefault();
         }
         break;
 
       case KeyCodes.DOM_VK_PAGE_UP:
         if (this.autocompletePopup.isOpen) {
           inputUpdated = this.complete(this.COMPLETE_PAGEUP);
-          if (inputUpdated) {
-            this._autocompletePopupNavigated = true;
-          }
         } else {
           this.hud.outputScroller.scrollTop =
             Math.max(0,
               this.hud.outputScroller.scrollTop -
               this.hud.outputScroller.clientHeight
             );
         }
         event.preventDefault();
         break;
 
       case KeyCodes.DOM_VK_PAGE_DOWN:
         if (this.autocompletePopup.isOpen) {
           inputUpdated = this.complete(this.COMPLETE_PAGEDOWN);
-          if (inputUpdated) {
-            this._autocompletePopupNavigated = true;
-          }
         } else {
           this.hud.outputScroller.scrollTop =
             Math.min(this.hud.outputScroller.scrollHeight,
               this.hud.outputScroller.scrollTop +
               this.hud.outputScroller.clientHeight
             );
         }
         event.preventDefault();
@@ -939,27 +906,23 @@ class JSTerm extends Component {
 
       case KeyCodes.DOM_VK_LEFT:
         if (this.autocompletePopup.isOpen || this.lastCompletion.value) {
           this.clearCompletion();
         }
         break;
 
       case KeyCodes.DOM_VK_RIGHT:
-        const cursorAtTheEnd = this.inputNode.selectionStart ==
-                             this.inputNode.selectionEnd &&
-                             this.inputNode.selectionStart ==
-                             inputValue.length;
-        const haveSuggestion = this.autocompletePopup.isOpen ||
-                             this.lastCompletion.value;
-        const useCompletion = cursorAtTheEnd || this._autocompletePopupNavigated;
-        if (haveSuggestion && useCompletion &&
-            this.complete(this.COMPLETE_HINT_ONLY) &&
-            this.lastCompletion.value &&
-            this.acceptProposedCompletion()) {
+        const haveSuggestion = this.autocompletePopup.isOpen || this.lastCompletion.value;
+        if (
+          haveSuggestion &&
+          this.complete(this.COMPLETE_HINT_ONLY) &&
+          this.lastCompletion.value &&
+          this.acceptProposedCompletion()
+        ) {
           event.preventDefault();
         }
         if (this.autocompletePopup.isOpen) {
           this.clearCompletion();
         }
         break;
 
       case KeyCodes.DOM_VK_TAB:
@@ -1296,17 +1259,17 @@ class JSTerm extends Component {
     const popup = this.autocompletePopup;
     popup.setItems(items);
 
     const completionType = this.lastCompletion.completionType;
     this.lastCompletion = {
       value: inputValue,
       matchProp: lastPart,
     };
-    if (items.length > 1 && !popup.isOpen) {
+    if (items.length > 0 && !popup.isOpen) {
       let popupAlignElement;
       let xOffset;
       let yOffset;
 
       if (this.editor) {
         popupAlignElement = this.node.querySelector(".CodeMirror-cursor");
         // We need to show the popup at the ".".
         xOffset = -1 * lastPart.length * this._inputCharWidth;
@@ -1316,22 +1279,21 @@ class JSTerm extends Component {
           (inputUntilCursor.lastIndexOf("\n") + 1) -
           lastPart.length;
         xOffset = (offset * this._inputCharWidth) + this._chevronWidth;
         popupAlignElement = this.inputNode;
       }
 
       if (popupAlignElement) {
         popup.openPopup(popupAlignElement, xOffset, yOffset);
-        this._autocompletePopupNavigated = false;
       }
-    } else if (items.length < 2 && popup.isOpen) {
+    } else if (items.length === 0 && popup.isOpen) {
       popup.hidePopup();
-      this._autocompletePopupNavigated = false;
     }
+
     if (items.length == 1) {
       popup.selectedIndex = 0;
     }
 
     this.onAutocompleteSelect();
 
     if (completionType != this.COMPLETE_HINT_ONLY && popup.itemCount == 1) {
       this.acceptProposedCompletion();
@@ -1376,17 +1338,16 @@ class JSTerm extends Component {
         // value again.
         if (this.inputNode) {
           this.inputNode.blur();
         }
         this.autocompletePopup.once("popup-closed", () => {
           this.focus();
         });
         this.autocompletePopup.hidePopup();
-        this._autocompletePopupNavigated = false;
       }
     }
   }
 
   /**
    * Accept the proposed input completion.
    *
    * @return boolean
--- a/devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_return_key_no_selection.js
+++ b/devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_return_key_no_selection.js
@@ -45,20 +45,23 @@ async function performTests() {
   await onPopUpOpen;
 
   ok(popup.isOpen, "popup is open");
   is(popup.itemCount, 2, "popup.itemCount is correct");
   isnot(popup.selectedIndex, -1, "popup.selectedIndex is correct");
 
   info("press Return and wait for popup to hide");
   const onPopUpClose = popup.once("popup-closed");
-  executeSoon(() => EventUtils.synthesizeKey("KEY_Enter"));
+  EventUtils.synthesizeKey("KEY_Enter");
   await onPopUpClose;
 
   ok(!popup.isOpen, "popup is not open after KEY_Enter");
-  is(jsterm.getInputValue(), "", "inputNode is empty after KEY_Enter");
+  is(jsterm.getInputValue(), "window.testBugA",
+    "input was completed with the first item of the popup");
   ok(!getJsTermCompletionValue(jsterm), "completeNode is empty");
 
+  EventUtils.synthesizeKey("KEY_Enter");
+  is(jsterm.getInputValue(), "", "input is empty after KEY_Enter");
+
   const state = ui.consoleOutput.getStore().getState();
   const entries = getHistoryEntries(state);
-  is(entries[entries.length - 1], "window.testBug",
-     "jsterm history is correct");
+  is(entries[entries.length - 1], "window.testBugA", "jsterm history is correct");
 }
--- a/devtools/client/webconsole/test/mochitest/browser_jsterm_completion.js
+++ b/devtools/client/webconsole/test/mochitest/browser_jsterm_completion.js
@@ -14,21 +14,24 @@ add_task(async function() {
   await performTests();
   // And then run it with the CodeMirror-powered one.
   await pushPref("devtools.webconsole.jsterm.codeMirror", true);
   await performTests();
 });
 
 async function performTests() {
   const {jsterm, ui} = await openNewTabAndConsole(TEST_URI);
+  const {autocompletePopup} = jsterm;
 
   // Test typing 'docu'.
   await jstermSetValueAndComplete(jsterm, "docu");
   is(jsterm.getInputValue(), "docu", "'docu' completion (input.value)");
   checkJsTermCompletionValue(jsterm, "    ment", "'docu' completion (completeNode)");
+  is(autocompletePopup.items.length, 1, "autocomplete popup has 1 item");
+  ok(autocompletePopup.isOpen, "autocomplete popup is open with 1 item");
 
   // Test typing 'docu' and press tab.
   await jstermSetValueAndComplete(jsterm, "docu", undefined, jsterm.COMPLETE_FORWARD);
   is(jsterm.getInputValue(), "document", "'docu' tab completion");
 
   checkJsTermCursor(jsterm, "document".length, "cursor is at the end of 'document'");
   is(getJsTermCompletionValue(jsterm).replace(/ /g, ""), "", "'docu' completed");