Bug 1557555 - Quantumbar: Autofill only when text is entered, never when it's removed (e.g., on backspace). r=mak a=jcristau DEVEDITION_68_0b10_BUILD1 DEVEDITION_68_0b10_RELEASE FENNEC_68_0b10_BUILD1 FENNEC_68_0b10_RELEASE FIREFOX_68_0b10_BUILD1 FIREFOX_68_0b10_RELEASE
authorDrew Willcoxon <adw@mozilla.com>
Wed, 12 Jun 2019 17:43:18 +0000
changeset 536935 4c434d19d03d5461e54fa22dfb82eaed4cd6631b
parent 536934 44d7135f115d5ca4d150424b2b425841d2db4139
child 536936 a0ea982feff9d2b2ade29bc2bd0f698eca1ff3e1
child 536938 e2af396f76076920acb9807e199f695ff28e99c8
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, jcristau
bugs1557555, 1545731, 719888
milestone68.0
Bug 1557555 - Quantumbar: Autofill only when text is entered, never when it's removed (e.g., on backspace). r=mak a=jcristau Autofill only when the user enters text -- i.e., when event.data in _on_input is non-empty. That allows us to simplify autofill quite a bit. We can get rid of the edit action listener that we previously used to detect selection deletion, and we also don't need the prefix check (lastSearchStartsWithNewSearch). Therefore this fixes both this bug (bug 1557555) and bug 1545731/719888. This makes one other change: We can check event.inputType in _on_input to tell whether the user is pasting, rather than keeping a _valueIsPasted property that we set in _on_paste. Differential Revision: https://phabricator.services.mozilla.com/D34645
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/UrlbarTestUtils.jsm
browser/components/urlbar/tests/browser/browser_UrlbarInput_unit.js
browser/components/urlbar/tests/browser/browser_autoFill_backspaced.js
browser/components/urlbar/tests/browser/browser_autoFill_undo.js
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -88,17 +88,16 @@ class UrlbarInput {
     this.controller.setInput(this);
     this.view = new UrlbarView(this);
     this.valueIsTyped = false;
     this.userInitiatedFocus = false;
     this.isPrivate = PrivateBrowsingUtils.isWindowPrivate(this.window);
     this.lastQueryContextPromise = Promise.resolve();
     this._actionOverrideKeyCount = 0;
     this._autofillPlaceholder = "";
-    this._deletedEndOfAutofillPlaceholder = false;
     this._lastSearchString = "";
     this._resultForCurrentValue = null;
     this._suppressStartQuery = false;
     this._untrimmedValue = "";
     this._openViewOnFocus = false;
 
     // This exists only for tests.
     this._enableAutofillPlaceholder = true;
@@ -186,18 +185,16 @@ class UrlbarInput {
    * Uninitializes this input object, detaching it from the inputField.
    */
   uninit() {
     for (let name of this._inputFieldEvents) {
       this.inputField.removeEventListener(name, this);
     }
     this.removeEventListener("mousedown", this);
 
-    this.editor.removeEditActionListener(this);
-
     this.view.panel.remove();
 
     // When uninit is called due to exiting the browser's customize mode,
     // this.inputField.controllers is not the original list of controllers, and
     // it doesn't contain CopyCutController.  That's why removeCopyCutController
     // must be called when entering customize mode.  If uninit ends up getting
     // called by something else though, try to remove the controller now.
     try {
@@ -694,41 +691,16 @@ class UrlbarInput {
   /**
    * Remove the hidden focus styles.
    * This is used by Activity Stream and about:privatebrowsing for search hand-off.
    */
   removeHiddenFocus() {
     this.textbox.classList.remove("hidden-focus");
   }
 
-  /**
-   * nsIEditActionListener method implementation.  We use this to detect when
-   * the user deletes autofilled substrings.
-   *
-   * There is also a DidDeleteSelection method, but it's called before the input
-   * event is fired.  So the order is: WillDeleteSelection, DidDeleteSelection,
-   * input event.  Further, in DidDeleteSelection, the passed-in selection
-   * object is the same as the object passed to WillDeleteSelection, but by that
-   * point its properties have been adjusted to account for the deletion.  For
-   * example, the endOffset property of its range will be smaller than it was in
-   * WillDeleteSelection.  Therefore we compute whether the user deleted the
-   * autofilled substring here in WillDeleteSelection instead of deferring it to
-   * when we handle the input event.
-   *
-   * @param {Selection} selection
-   *   The Selection object.
-   */
-  WillDeleteSelection(selection) {
-    this._deletedEndOfAutofillPlaceholder =
-      selection &&
-      selection.getRangeAt(0).endOffset ==
-        this._autofillPlaceholder.length &&
-      this._autofillPlaceholder.endsWith(String(selection));
-  }
-
   // Getters and Setters below.
 
   get focused() {
     return this.textbox.getAttribute("focused") == "true";
   }
 
   get goButton() {
     return this.document.getAnonymousElementByAttribute(this.textbox, "anonid",
@@ -814,31 +786,21 @@ class UrlbarInput {
 
   /**
    * Autofills the autofill placeholder string if appropriate, and determines
    * whether autofill should be allowed for the new search started by an input
    * event.
    *
    * @param {string} value
    *   The new search string.
-   * @param {boolean} deletedAutofilledSubstring
-   *   Whether the user deleted the previously autofilled substring.
    * @returns {boolean}
    *   Whether autofill should be allowed in the new search.
    */
-  _maybeAutofillOnInput(value, deletedAutofilledSubstring) {
-    // Determine whether autofill should be allowed for the new search triggered
-    // by the input event.
-    let lastSearchStartsWithNewSearch =
-      value.length < this._lastSearchString.length &&
-      this._lastSearchString.startsWith(value);
-    let allowAutofill =
-      !lastSearchStartsWithNewSearch &&
-      !deletedAutofilledSubstring &&
-      this.selectionEnd == value.length;
+  _maybeAutofillOnInput(value) {
+    let allowAutofill = this.selectionEnd == value.length;
 
     // Determine whether we can autofill the placeholder.  The placeholder is a
     // value that we autofill now, when the search starts and before we wait on
     // its first result, in order to prevent a flicker in the input caused by
     // the previous autofilled substring disappearing and reappearing when the
     // first result arrives.  Of course we can only autofill the placeholder if
     // it starts with the new search string, and we shouldn't autofill anything
     // if the caret isn't at the end of the input.
@@ -1340,27 +1302,22 @@ class UrlbarInput {
       if (this.view.isOpen) {
         this.view.close();
       } else {
         this.startQuery();
       }
     }
   }
 
-  _on_input() {
+  _on_input(event) {
     let value = this.textValue;
     this.valueIsTyped = true;
-    let valueIsPasted = this._valueIsPasted;
-    this._valueIsPasted = false;
     this._untrimmedValue = value;
     this.window.gBrowser.userTypedValue = value;
 
-    let deletedEndOfAutofillPlaceholder = this._deletedEndOfAutofillPlaceholder;
-    this._deletedEndOfAutofillPlaceholder = false;
-
     let compositionState = this._compositionState;
     let compositionClosedPopup = this._compositionClosedPopup;
 
     // Clear composition values if we're no more composing.
     if (this._compositionState != UrlbarUtils.COMPOSITION.COMPOSING) {
       this._compositionState = UrlbarUtils.COMPOSITION.NONE;
       this._compositionClosedPopup = false;
     }
@@ -1388,42 +1345,35 @@ class UrlbarInput {
     // We should do nothing during composition or if composition was canceled
     // and we didn't close the popup on composition start.
     if (compositionState == UrlbarUtils.COMPOSITION.COMPOSING ||
         (compositionState == UrlbarUtils.COMPOSITION.CANCELED &&
          !compositionClosedPopup)) {
       return;
     }
 
-    let deletedAutofilledSubstring =
-      deletedEndOfAutofillPlaceholder && value == this._lastSearchString;
-    let allowAutofill = !valueIsPasted &&
-      this._maybeAutofillOnInput(value, deletedAutofilledSubstring);
+    // Autofill only when text is inserted (i.e., event.data is not empty) and
+    // it's not due to pasting.
+    let allowAutofill =
+      !!event.data &&
+      !event.inputType.startsWith("insertFromPaste") &&
+      event.inputType != "insertFromYank" &&
+      this._maybeAutofillOnInput(value);
 
     this.startQuery({
       searchString: value,
       allowAutofill,
       allowEmptyInput: true,
       resetSearchState: false,
     });
   }
 
   _on_select(event) {
-    if (!this.window.windowUtils.isHandlingUserInput) {
-      // Register the editor listener we use to detect when the user deletes
-      // autofilled substrings.  The editor is destroyed and removes all its
-      // listeners at various surprising times, and autofill causes a non-user
-      // select, which is why we do this here instead of, for example, in the
-      // constructor.  addEditActionListener is idempotent, so it's OK to call
-      // it even when we're already registered.
-      this.editor.addEditActionListener(this);
-      return;
-    }
-
-    if (!Services.clipboard.supportsSelectionClipboard()) {
+    if (!this.window.windowUtils.isHandlingUserInput ||
+        !Services.clipboard.supportsSelectionClipboard()) {
       return;
     }
 
     let val = this._getSelectedValueForClipboard();
     if (!val) {
       return;
     }
 
@@ -1457,17 +1407,17 @@ class UrlbarInput {
     this._updateUrlTooltip();
   }
 
   _on_paste(event) {
     let originalPasteData = event.clipboardData.getData("text/plain");
     if (!originalPasteData) {
       return;
     }
-    this._valueIsPasted = true;
+
     let oldValue = this.inputField.value;
     let oldStart = oldValue.substring(0, this.selectionStart);
     // If there is already non-whitespace content in the URL bar
     // preceding the pasted content, it's not necessary to check
     // protocols used by the pasted content:
     if (oldStart.trim()) {
       return;
     }
--- a/browser/components/urlbar/tests/UrlbarTestUtils.jsm
+++ b/browser/components/urlbar/tests/UrlbarTestUtils.jsm
@@ -314,18 +314,21 @@ class UrlbarAbstraction {
   focus() {
     this.urlbar.inputField.focus();
   }
 
   /**
    * Dispatches an input event to the input field.
    */
   fireInputEvent() {
-    let event = this.window.document.createEvent("Events");
-    event.initEvent("input", true, true);
+    // Set event.data to the last character in the input, for a couple of
+    // reasons: It simulates the user typing, and it's necessary for autofill.
+    let event = new InputEvent("input", {
+      data: this.urlbar.value[this.urlbar.value.length - 1] || null,
+    });
     this.urlbar.inputField.dispatchEvent(event);
   }
 
   set value(val) {
     this.urlbar.value = val;
   }
   get value() {
     return this.urlbar.value;
--- a/browser/components/urlbar/tests/browser/browser_UrlbarInput_unit.js
+++ b/browser/components/urlbar/tests/browser/browser_UrlbarInput_unit.js
@@ -155,48 +155,8 @@ add_task(async function test_input_with_
     checkStartQueryCall(fakeController.startQuery, {
       searchString: "search",
       isPrivate: true,
     });
 
     sandbox.resetHistory();
   });
 });
-
-add_task(async function test_autofill_disabled_on_prefix_search() {
-  await withNewWindow(input => {
-    // search for "autofill" -- autofill should be enabled
-    input.inputField.value = "autofill";
-    input.handleEvent({
-      target: input.inputField,
-      type: "input",
-    });
-    checkStartQueryCall(fakeController.startQuery, {
-      searchString: "autofill",
-      allowAutofill: true,
-    });
-
-    // search for "auto" -- autofill should be disabled since the previous
-    // search string starts with the new search string
-    input.inputField.value = "auto";
-    input.handleEvent({
-      target: input.inputField,
-      type: "input",
-    });
-    checkStartQueryCall(fakeController.startQuery, {
-      searchString: "auto",
-      allowAutofill: false,
-    }, 1);
-
-    // search for "autofill" again -- autofill should be enabled
-    input.inputField.value = "autofill";
-    input.handleEvent({
-      target: input.inputField,
-      type: "input",
-    });
-    checkStartQueryCall(fakeController.startQuery, {
-      searchString: "autofill",
-      allowAutofill: true,
-    }, 2);
-
-    sandbox.resetHistory();
-  });
-});
--- a/browser/components/urlbar/tests/browser/browser_autoFill_backspaced.js
+++ b/browser/components/urlbar/tests/browser/browser_autoFill_backspaced.js
@@ -12,22 +12,25 @@ async function test_autocomplete(data) {
   info(desc);
 
   await promiseAutocompleteResultPopup(typed);
   Assert.equal(gURLBar.textValue, autofilled, "autofilled value is as expected");
   if (onAutoFill)
     onAutoFill();
 
   info("Synthesizing keys");
-  keys.forEach(key => EventUtils.synthesizeKey(key));
+  for (let key of keys) {
+    let args = Array.isArray(key) ? key : [key];
+    EventUtils.synthesizeKey(...args);
+  }
+
+  await promiseSearchComplete();
 
   Assert.equal(gURLBar.textValue, modified, "backspaced value is as expected");
 
-  await promiseSearchComplete();
-
   Assert.greater(UrlbarTestUtils.getResultCount(window), 0,
     "Should get at least 1 result");
 
   let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
 
   Assert.equal(result.type, type,
     "Should have the correct result type");
 
@@ -38,21 +41,20 @@ async function test_autocomplete(data) {
 add_task(async function() {
   registerCleanupFunction(async function() {
     Services.prefs.clearUserPref("browser.urlbar.autoFill");
     gURLBar.handleRevert();
     await PlacesUtils.history.clear();
   });
   Services.prefs.setBoolPref("browser.urlbar.autoFill", true);
 
-  // Add a typed visit, so it will be autofilled.
-  await PlacesTestUtils.addVisits({
-    uri: NetUtil.newURI("http://example.com/"),
-    transition: Ci.nsINavHistoryService.TRANSITION_TYPED,
-  });
+  await PlacesTestUtils.addVisits([
+    "http://example.com/",
+    "http://example.com/foo",
+  ]);
 
   await test_autocomplete({ desc: "DELETE the autofilled part should search",
                             typed: "exam",
                             autofilled: "example.com/",
                             modified: "exam",
                             keys: ["KEY_Delete"],
                             type: UrlbarUtils.RESULT_TYPE.SEARCH,
                           });
@@ -144,10 +146,77 @@ add_task(async function() {
                             onAutoFill: () => {
                               gURLBar.blur();
                               gURLBar.focus();
                               gURLBar.selectionStart = 2;
                               gURLBar.selectionEnd = 12;
                             },
                          });
 
+  await test_autocomplete({
+    desc: "Right arrow key and then backspace should delete the backslash and not re-trigger autofill",
+    typed: "ex",
+    autofilled: "example.com/",
+    modified: "example.com",
+    keys: ["KEY_ArrowRight", "KEY_Backspace"],
+    type: UrlbarUtils.RESULT_TYPE.URL,
+  });
+
+  await test_autocomplete({
+    desc: "Right arrow key, selecting the last few characters using the keyboard, and then backspace should delete the characters and not re-trigger autofill",
+    typed: "ex",
+    autofilled: "example.com/",
+    modified: "example.c",
+    keys: [
+      "KEY_ArrowRight",
+      ["KEY_ArrowLeft", { shiftKey: true }],
+      ["KEY_ArrowLeft", { shiftKey: true }],
+      ["KEY_ArrowLeft", { shiftKey: true }],
+      "KEY_Backspace",
+    ],
+    type: UrlbarUtils.RESULT_TYPE.URL,
+  });
+
+  // The remaining tests fail on awesomebar because it doesn't properly handle
+  // them.
+  if (!UrlbarPrefs.get("quantumbar")) {
+    return;
+  }
+
+  await test_autocomplete({
+    desc: "End and then backspace should delete the backslash and not re-trigger autofill",
+    typed: "ex",
+    autofilled: "example.com/",
+    modified: "example.com",
+    keys: [
+      AppConstants.platform == "macosx" ?
+        ["KEY_ArrowRight", { metaKey: true }] :
+        "KEY_End",
+      "KEY_Backspace",
+    ],
+    type: UrlbarUtils.RESULT_TYPE.URL,
+  });
+
+  await test_autocomplete({
+    desc: "Clicking in the input after the text and then backspace should delete the backslash and not re-trigger autofill",
+    typed: "ex",
+    autofilled: "example.com/",
+    modified: "example.com",
+    keys: ["KEY_Backspace"],
+    type: UrlbarUtils.RESULT_TYPE.URL,
+    onAutoFill: () => {
+      // This assumes that the center of the input is to the right of the end
+      // of the text, so the caret is placed at the end of the text on click.
+      EventUtils.synthesizeMouseAtCenter(gURLBar.inputField, {});
+    },
+  });
+
+  await test_autocomplete({
+    desc: "Selecting the next result and then backspace should delete the last character and not re-trigger autofill",
+    typed: "ex",
+    autofilled: "example.com/",
+    modified: "example.com/fo",
+    keys: ["KEY_ArrowDown", "KEY_Backspace"],
+    type: UrlbarUtils.RESULT_TYPE.URL,
+  });
+
   await PlacesUtils.history.clear();
 });
--- a/browser/components/urlbar/tests/browser/browser_autoFill_undo.js
+++ b/browser/components/urlbar/tests/browser/browser_autoFill_undo.js
@@ -39,15 +39,15 @@ add_task(async function test() {
 
   // Undo the typed x.
   goDoCommand("cmd_undo");
 
   // The text should be restored to ex[ample.com/] (with the part in brackets
   // autofilled and selected).
   await UrlbarTestUtils.promiseSearchComplete(window);
   details = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
-  Assert.ok(details.autofill);
+  Assert.equal(details.autofill, !UrlbarPrefs.get("quantumbar"));
   Assert.equal(gURLBar.value, "example.com/");
   Assert.equal(gURLBar.selectionStart, "ex".length);
   Assert.equal(gURLBar.selectionEnd, "example.com/".length);
 
   await PlacesUtils.history.clear();
 });