Bug 1595068 - Fix issue with debounced autocompletion in JsTerm. r=Honza,julienw.
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Wed, 13 Nov 2019 16:19:18 +0000
changeset 501891 5dcaf36d218a33e8d20563f6210a296e0d47f2fa
parent 501890 828246fa2bdf81639ca22092cb330e2473c1d4e8
child 501892 34daa01d30f74e292f82b22ffc4fcb6c0b50aa0b
push id36801
push userdvarga@mozilla.com
push dateThu, 14 Nov 2019 17:12:31 +0000
treeherdermozilla-central@a19a226a8c6a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersHonza, julienw
bugs1595068
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1595068 - Fix issue with debounced autocompletion in JsTerm. r=Honza,julienw. The issue was that if a user typed a legitimate letter, then, quickly after (i.e. before the autocompletion results are here) hit Enter or Tab, the resulting input value would be erroneous. This is because we retrieve the value to insert from the autocomplete popup, which at that time, is out-of-date, for this brief moment. The fix consists in updating the preLabel property of the autocomplete popup items in order to get the proper completion if the user ever hit enter, tab or arrow right. We also take this as an opportunity to have a mechanism to be able to cancel a registered update. Differential Revision: https://phabricator.services.mozilla.com/D52376
devtools/client/webconsole/components/Input/JSTerm.js
devtools/client/webconsole/test/browser/browser.ini
devtools/client/webconsole/test/browser/browser_jsterm_autocomplete_race_on_enter.js
devtools/client/webconsole/test/browser/browser_jsterm_completion_bracket_cached_results.js
devtools/shared/debounce.js
--- a/devtools/client/webconsole/components/Input/JSTerm.js
+++ b/devtools/client/webconsole/components/Input/JSTerm.js
@@ -112,24 +112,32 @@ class JSTerm extends Component {
     super(props);
 
     const { webConsoleUI } = props;
 
     this.webConsoleUI = webConsoleUI;
     this.hudId = this.webConsoleUI.hudId;
 
     this._onEditorChanges = this._onEditorChanges.bind(this);
+    this._onEditorBeforeChange = this._onEditorBeforeChange.bind(this);
     this.onContextMenu = this.onContextMenu.bind(this);
     this.imperativeUpdate = this.imperativeUpdate.bind(this);
 
     // We debounce the autocompleteUpdate so we don't send too many requests to the server
     // as the user is typing.
     // The delay should be small enough to be unnoticed by the user.
     this.autocompleteUpdate = debounce(this.props.autocompleteUpdate, 75, this);
 
+    // Because the autocomplete has a slight delay (75ms), there can be time where the
+    // codeMirror completion text is out-of-date, which might lead to issue when the user
+    // accept the autocompletion while the update of the completion text is still pending.
+    // In order to account for that, we put any future value of the completion text in
+    // this property.
+    this.pendingCompletionText = null;
+
     /**
      * Last input value.
      * @type string
      */
     this.lastInputValue = "";
 
     this.autocompletePopup = null;
 
@@ -695,27 +703,53 @@ class JSTerm extends Component {
    * Even handler for the "beforeChange" event fired by codeMirror. This event is fired
    * when codeMirror is about to make a change to its DOM representation.
    */
   _onEditorBeforeChange(cm, change) {
     // If the user did not type a character that matches the completion text, then we
     // clear it before the change is done to prevent a visual glitch.
     // See Bugs 1491776 & 1558248.
     const { from, to, origin, text } = change;
+    const isAddedText =
+      from.line === to.line && from.ch === to.ch && origin === "+input";
+    const addedText = text.join("");
     const completionText = this.getAutoCompletionText();
 
     const addedCharacterMatchCompletion =
-      from.line === to.line &&
-      from.ch === to.ch &&
-      origin === "+input" &&
-      completionText.startsWith(text.join(""));
+      isAddedText && completionText.startsWith(addedText);
+
+    const addedCharacterMatchPopupItem =
+      isAddedText &&
+      this.autocompletePopup.items.some(({ preLabel, label }) =>
+        label.startsWith(preLabel + addedText)
+      );
 
     if (!completionText || change.canceled || !addedCharacterMatchCompletion) {
       this.setAutoCompletionText("");
     }
+
+    if (!addedCharacterMatchCompletion && !addedCharacterMatchPopupItem) {
+      this.autocompletePopup.hidePopup();
+    } else if (
+      completionText &&
+      !change.canceled &&
+      (addedCharacterMatchCompletion || addedCharacterMatchPopupItem)
+    ) {
+      // The completion text will be updated when the debounced autocomplete update action
+      // is done, so in the meantime we set the pending value to pendingCompletionText.
+      // See Bug 1595068 for more information.
+      this.pendingCompletionText = completionText.substring(text.length);
+      // And we update the preLabel of the matching autocomplete items that may be used
+      // in the acceptProposedAutocompletion function.
+      this.autocompletePopup.items.forEach(item => {
+        if (item.label.startsWith(item.preLabel + addedText)) {
+          item.preLabel += addedText;
+        }
+      });
+    }
   }
 
   /**
    * The editor "changes" event handler.
    */
   _onEditorChanges(cm, changes) {
     const value = this._getValue();
 
@@ -934,20 +968,22 @@ class JSTerm extends Component {
       }
       this.setAutoCompletionText(suffix);
     } else {
       this.setAutoCompletionText("");
     }
   }
 
   /**
-   * Clear the current completion information and close the autocomplete popup,
-   * if needed.
+   * Clear the current completion information, cancel any pending autocompletion update
+   * and close the autocomplete popup, if needed.
    */
   clearCompletion() {
+    this.autocompleteUpdate.cancel();
+
     this.setAutoCompletionText("");
     if (this.autocompletePopup) {
       this.autocompletePopup.clearItems();
 
       if (this.autocompletePopup.isOpen) {
         this.autocompletePopup.once("popup-closed", () => {
           this.focus();
         });
@@ -990,16 +1026,17 @@ class JSTerm extends Component {
         );
         // If there's not a bracket after the cursor, add it.
         if (!inputAfterCursor.trimLeft().startsWith("]")) {
           completionText = completionText + "]";
         }
       }
     }
 
+    this.autocompleteUpdate.cancel();
     this.props.autocompleteClear();
 
     if (completionText) {
       this.insertStringAtCursor(
         completionText,
         numberOfCharsToReplaceCharsBeforeCursor
       );
     }
@@ -1042,25 +1079,31 @@ class JSTerm extends Component {
    * @param string suffix
    *        The proposed suffix for the input value.
    */
   setAutoCompletionText(suffix) {
     if (!this.editor) {
       return;
     }
 
+    this.pendingCompletionText = null;
+
     if (suffix && !this.canDisplayAutoCompletionText()) {
       suffix = "";
     }
 
     this.editor.setAutoCompletionText(suffix);
   }
 
   getAutoCompletionText() {
-    return this.editor ? this.editor.getAutoCompletionText() : null;
+    const renderedCompletionText =
+      this.editor && this.editor.getAutoCompletionText();
+    return typeof this.pendingCompletionText === "string"
+      ? this.pendingCompletionText
+      : renderedCompletionText;
   }
 
   /**
    * Indicate if the input has an autocompletion suggestion, i.e. that there is either
    * something in the autocompletion text or that there's a selected item in the
    * autocomplete popup.
    */
   hasAutocompletionSuggestion() {
@@ -1099,16 +1142,18 @@ class JSTerm extends Component {
     return this.editor ? this.editor.defaultCharWidth() : null;
   }
 
   onContextMenu(e) {
     this.props.serviceContainer.openEditContextMenu(e);
   }
 
   destroy() {
+    this.autocompleteUpdate.cancel();
+
     if (this.autocompletePopup) {
       this.autocompletePopup.destroy();
       this.autocompletePopup = null;
     }
 
     if (this.editor) {
       this.editor.destroy();
       this.editor = null;
--- a/devtools/client/webconsole/test/browser/browser.ini
+++ b/devtools/client/webconsole/test/browser/browser.ini
@@ -214,16 +214,17 @@ skip-if = verify
 [browser_jsterm_autocomplete_getters_learn_more_link.js]
 [browser_jsterm_autocomplete_helpers.js]
 [browser_jsterm_autocomplete_in_chrome_tab.js]
 [browser_jsterm_autocomplete_in_debugger_stackframe.js]
 [browser_jsterm_autocomplete_inside_text.js]
 [browser_jsterm_autocomplete_native_getters.js]
 [browser_jsterm_autocomplete_nav_and_tab_key.js]
 [browser_jsterm_autocomplete_paste_undo.js]
+[browser_jsterm_autocomplete_race_on_enter.js]
 [browser_jsterm_autocomplete_return_key_no_selection.js]
 [browser_jsterm_autocomplete_return_key.js]
 [browser_jsterm_autocomplete_width.js]
 [browser_jsterm_autocomplete_will_navigate.js]
 [browser_jsterm_autocomplete-properties-with-non-alphanumeric-names.js]
 [browser_jsterm_await_assignments.js]
 [browser_jsterm_await_concurrent_same_result.js]
 [browser_jsterm_await_concurrent.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/webconsole/test/browser/browser_jsterm_autocomplete_race_on_enter.js
@@ -0,0 +1,138 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that pressing Enter quickly after a letter that makes the input exactly match the
+// item in the autocomplete popup does not insert unwanted character. See Bug 1595068.
+
+const TEST_URI = `data:text/html;charset=utf-8,<script>
+  var uvwxyz = "zyxwvu";
+</script>Autocomplete race on Enter`;
+
+add_task(async function() {
+  const hud = await openNewTabAndConsole(TEST_URI);
+  const { jsterm } = hud;
+  const { autocompletePopup } = jsterm;
+
+  info(`Enter "scre" and wait for the autocomplete popup to be displayed`);
+  let onPopupOpened = autocompletePopup.once("popup-opened");
+  await setInputValueForAutocompletion(hud, "scre");
+  await onPopupOpened;
+  checkInputCompletionValue(hud, "en", "completeNode has expected value");
+
+  info(`Type "n" and quickly after, "Enter"`);
+  let onPopupClosed = autocompletePopup.once("popup-closed");
+  EventUtils.synthesizeKey("e");
+  await waitForTime(50);
+  EventUtils.synthesizeKey("KEY_Enter");
+  await onPopupClosed;
+
+  is(getInputValue(hud), "screen", "the input has the expected value");
+
+  setInputValue(hud, "");
+
+  info(
+    "Check that it works when typed word match exactly the item in the popup"
+  );
+  onPopupOpened = autocompletePopup.once("popup-opened");
+  await setInputValueForAutocompletion(hud, "wind");
+  await onPopupOpened;
+  checkInputCompletionValue(hud, "ow", "completeNode has expected value");
+
+  info(`Quickly type "o", "w" and "Enter"`);
+  onPopupClosed = autocompletePopup.once("popup-closed");
+  EventUtils.synthesizeKey("o");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("w");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("KEY_Enter");
+  await onPopupClosed;
+
+  is(getInputValue(hud), "window", "the input has the expected value");
+
+  setInputValue(hud, "");
+
+  info("Check that it works when there's no autocomplete popup");
+  let onAutocompleteUpdated = jsterm.once("autocomplete-updated");
+  await setInputValueForAutocompletion(hud, "uvw");
+  await onAutocompleteUpdated;
+  checkInputCompletionValue(hud, "xyz", "completeNode has expected value");
+
+  info(`Quickly type "x" and "Enter"`);
+  EventUtils.synthesizeKey("x");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("KEY_Enter");
+  await waitFor(
+    () => getInputValue(hud) === "uvwxyz",
+    "input has expected 'uvwxyz' value"
+  );
+  ok(true, "input has the expected value");
+
+  setInputValue(hud, "");
+
+  info(
+    "Check that it works when there's no autocomplete popup and the whole word is typed"
+  );
+  onAutocompleteUpdated = jsterm.once("autocomplete-updated");
+  await setInputValueForAutocompletion(hud, "uvw");
+  await onAutocompleteUpdated;
+  checkInputCompletionValue(hud, "xyz", "completeNode has expected value");
+
+  info(`Quickly type "x", "y", "z" and "Enter"`);
+  const onResultMessage = waitForMessage(hud, "zyxwvu", ".result");
+  EventUtils.synthesizeKey("x");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("y");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("z");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("KEY_Enter");
+  info("wait for result message");
+  await onResultMessage;
+  is(getInputValue(hud), "", "Expression was evaluated and input was cleared");
+
+  setInputValue(hud, "");
+
+  info("Check that it works when typed letter match another item in the popup");
+  onPopupOpened = autocompletePopup.once("popup-opened");
+  await setInputValueForAutocompletion(hud, "[].so");
+  await onPopupOpened;
+  checkInputCompletionValue(hud, "me", "completeNode has expected value");
+  is(
+    autocompletePopup.items.map(({ label }) => label).join("|"),
+    "some|sort",
+    "autocomplete has expected items"
+  );
+
+  info(`Quickly type "m" and "Enter"`);
+  onPopupClosed = autocompletePopup.once("popup-closed");
+  EventUtils.synthesizeKey("m");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("KEY_Enter");
+  await onPopupClosed;
+  is(getInputValue(hud), "[].some", "the input has the expected value");
+
+  setInputValue(hud, "");
+
+  info(
+    "Hitting Enter quicly after a letter that should close the popup evaluates the expression"
+  );
+  onPopupOpened = autocompletePopup.once("popup-opened");
+  await setInputValueForAutocompletion(hud, "var docx = 1; doc");
+  await onPopupOpened;
+  checkInputCompletionValue(hud, "ument", "completeNode has expected value");
+
+  info(`Quickly type "x" and "Enter"`);
+  onPopupClosed = autocompletePopup.once("popup-closed");
+  const onMessage = waitForMessage(hud, "1", ".result");
+  EventUtils.synthesizeKey("x");
+  await waitForTime(5);
+  EventUtils.synthesizeKey("KEY_Enter");
+  await Promise.all[(onPopupClosed, onMessage)];
+  is(
+    getInputValue(hud),
+    "",
+    "the input is empty and the expression was evaluated"
+  );
+});
--- a/devtools/client/webconsole/test/browser/browser_jsterm_completion_bracket_cached_results.js
+++ b/devtools/client/webconsole/test/browser/browser_jsterm_completion_bracket_cached_results.js
@@ -117,18 +117,18 @@ add_task(async function() {
       );
       checkInputCompletionValue(
         hud,
         expectedCompletionText,
         `completeNode has expected value`
       );
     }
 
-    setInputValue(hud, "");
     const onPopupClose = autocompletePopup.once("popup-closed");
     EventUtils.synthesizeKey("KEY_Escape");
     await onPopupClose;
+    setInputValue(hud, "");
   }
 });
 
 function getAutocompletePopupLabels(autocompletePopup) {
   return autocompletePopup.items.map(i => i.label);
 }
--- a/devtools/shared/debounce.js
+++ b/devtools/shared/debounce.js
@@ -9,25 +9,37 @@
  * amount of time has passed without it being called.
  *
  * @param {Function} func
  *         The function to debounce
  * @param {number} wait
  *         The wait period
  * @param {Object} scope
  *         The scope to use for func
- * @return {Function} The debounced function
+ * @return {Function} The debounced function, which has a `cancel` method that the
+ *                    consumer can call to cancel any pending setTimeout callback.
  */
 exports.debounce = function(func, wait, scope) {
   let timer = null;
 
-  return function() {
+  function clearTimer(resetTimer = false) {
     if (timer) {
       clearTimeout(timer);
     }
+    if (resetTimer) {
+      timer = null;
+    }
+  }
+
+  const debouncedFunction = function() {
+    clearTimer();
 
     const args = arguments;
     timer = setTimeout(function() {
       timer = null;
       func.apply(scope, args);
     }, wait);
   };
+
+  debouncedFunction.cancel = clearTimer.bind(null, true);
+
+  return debouncedFunction;
 };