Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao, a=jcristau
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 04 Jul 2019 08:23:18 +0000
changeset 537203 6a1981c7a25745760e9f23b0469bcd6f9e208343
parent 537202 a2caf98462778a9308dd7a45ee76e46b440bbc81
child 537204 59f950e8418099e5c3c7bb7ed69a767ec52442f4
push id2090
push userjcristau@mozilla.com
push dateThu, 04 Jul 2019 14:39:11 +0000
treeherdermozilla-release@59f950e84180 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, dao, jcristau
bugs1562585, 1562823
milestone68.0
Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao, a=jcristau When the search string is supposed to be "empty", we should not store a url in inputhistory's input column, instead we should store an empty input. This happens for example when clicking the dropmarker and picking something, or when pressing DOWN on a valid pageproxystate (no user input). In the future we may evaluate not storing an empty string at all (Bug 1562823). Differential Revision: https://phabricator.services.mozilla.com/D36568
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/UrlbarTestUtils.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_inputHistory_emptystring.js
browser/components/urlbar/tests/legacy/browser.ini
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -89,16 +89,17 @@ class UrlbarInput {
     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._lastSearchString = "";
+    this._textValueOnLastSearch = "";
     this._resultForCurrentValue = null;
     this._suppressStartQuery = false;
     this._untrimmedValue = "";
     this._openViewOnFocus = false;
 
     // This exists only for tests.
     this._enableAutofillPlaceholder = true;
 
@@ -464,17 +465,17 @@ class UrlbarInput {
           // to the end of it. Because there's a trailing space in the value,
           // the user can directly start typing a query string at that point.
           this.selectionStart = this.selectionEnd = this.value.length;
 
           // Picking a keyword offer just fills it in the input and doesn't
           // visit anything.  The user can then type a search string.  Also
           // start a new search so that the offer appears in the view by itself
           // to make it even clearer to the user what's going on.
-          this.startQuery({ allowEmptyInput: true });
+          this.startQuery();
           return;
         }
         const actionDetails = {
           isSuggestion: !!result.payload.suggestion,
           alias: result.payload.keyword,
         };
         const engine = Services.search.getEngineByName(result.payload.engine);
         this._recordSearch(engine, event, actionDetails);
@@ -522,17 +523,20 @@ class UrlbarInput {
    * @param {Event} [event] The event that picked the result.
    * @returns {boolean}
    *   Whether the value has been canonized
    */
   setValueFromResult(result = null, event = null) {
     let canonizedUrl;
 
     if (!result) {
-      this.value = this._lastSearchString;
+      // This usually happens when there's no selected results (the user cycles
+      // through results and there was no heuristic), and we reset the input
+      // value to the previous text value.
+      this.value = this._textValueOnLastSearch;
     } else {
       // For autofilled results, the value that should be canonized is not the
       // autofilled value but the value that the user typed.
       canonizedUrl = this._maybeCanonizeURL(event, result.autofill ?
                        this._lastSearchString : this.textValue);
       if (canonizedUrl) {
         this.value = canonizedUrl;
       } else if (result.autofill) {
@@ -604,45 +608,39 @@ class UrlbarInput {
    *   Otherwise, the current input value must start with this value.
    * @param {boolean} [options.resetSearchState]
    *   If this is the first search of a user interaction with the input, set
    *   this to true (the default) so that search-related state from the previous
    *   interaction doesn't interfere with the new interaction.  Otherwise set it
    *   to false so that state is maintained during a single interaction.  The
    *   intended use for this parameter is that it should be set to false when
    *   this method is called due to input events.
-   * @param {boolean} [options.allowEmptyInput]
-   *   If true and the search string is empty, then the input will become empty
-   *   when no result is selected.  If false, the input will continue showing
-   *   the last non-empty search string when no result is selected.
    */
   startQuery({
     allowAutofill = true,
     searchString = null,
     resetSearchState = true,
-    allowEmptyInput = false,
   } = {}) {
     if (this._suppressStartQuery) {
       return;
     }
 
     if (resetSearchState) {
       this._resetSearchState();
     }
 
     if (!searchString) {
       searchString = (this.getAttribute("pageproxystate") == "valid") ?
                      "" : this.textValue;
     } else if (!this.textValue.startsWith(searchString)) {
       throw new Error("The current value doesn't start with the search string");
     }
 
-    if (searchString || allowEmptyInput) {
-      this._lastSearchString = searchString;
-    }
+    this._lastSearchString = searchString;
+    this._textValueOnLastSearch = this.textValue;
 
     // TODO (Bug 1522902): This promise is necessary for tests, because some
     // tests are not listening for completion when starting a query through
     // other methods than startQuery (input events for example).
     this.lastQueryContextPromise = this.controller.startQuery(new UrlbarQueryContext({
       allowAutofill,
       isPrivate: this.isPrivate,
       maxResults: UrlbarPrefs.get("maxRichResults"),
@@ -1303,17 +1301,19 @@ class UrlbarInput {
     }
 
     if (event.originalTarget.classList.contains("urlbar-history-dropmarker") &&
         event.button == 0) {
       if (this.view.isOpen) {
         this.view.close();
       } else {
         this.focus();
-        this.startQuery({ allowAutofill: false });
+        this.startQuery({
+          allowAutofill: false,
+        });
       }
     }
   }
 
   _on_input(event) {
     let value = this.textValue;
     this.valueIsTyped = true;
     this._untrimmedValue = value;
@@ -1362,17 +1362,16 @@ class UrlbarInput {
       !!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 ||
         !Services.clipboard.supportsSelectionClipboard()) {
       return;
--- a/browser/components/urlbar/tests/UrlbarTestUtils.jsm
+++ b/browser/components/urlbar/tests/UrlbarTestUtils.jsm
@@ -177,16 +177,21 @@ var UrlbarTestUtils = {
    * @param {object} win The window containing the urlbar
    * @returns {object} the results panel object.
    */
   getPanel(win) {
     let urlbar = getUrlbarAbstraction(win);
     return urlbar.panel;
   },
 
+  getDropMarker(win) {
+    let urlbar = getUrlbarAbstraction(win);
+    return urlbar.dropMarker;
+  },
+
   /**
    * Ensures at least one search suggestion is present.
    * @param {object} win The window containing the urlbar
    * @returns {boolean} whether at least one search suggestion is present.
    */
   promiseSuggestionsPresent(win) {
     let urlbar = getUrlbarAbstraction(win);
     return urlbar.promiseSearchSuggestions();
@@ -338,16 +343,21 @@ class UrlbarAbstraction {
     return this.quantumbar ? this.urlbar._lastSearchString :
                              this.urlbar.controller.searchString;
   }
 
   get panel() {
     return this.quantumbar ? this.urlbar.panel : this.urlbar.popup;
   }
 
+  get dropMarker() {
+    return this.window.document.getAnonymousElementByAttribute(
+      this.urlbar.textbox, "anonid", "historydropmarker");
+  }
+
   get oneOffSearchButtons() {
     return this.quantumbar ? this.urlbar.view.oneOffSearchButtons :
            this.urlbar.popup.oneOffSearchButtons;
   }
 
   get oneOffSearchButtonsVisible() {
     if (!this.quantumbar) {
       return this.window.getComputedStyle(this.oneOffSearchButtons.container).display != "none";
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -43,16 +43,17 @@ support-files =
 [browser_customizeMode.js]
 [browser_deleteAllText.js]
 [browser_doubleClickSelectsAll.js]
 [browser_downArrowKeySearch.js]
 [browser_dragdropURL.js]
 [browser_dropmarker.js]
 [browser_ime_composition.js]
 [browser_inputHistory.js]
+[browser_inputHistory_emptystring.js]
 [browser_keepStateAcrossTabSwitches.js]
 [browser_keywordBookmarklets.js]
 [browser_keyword_override.js]
 [browser_keywordSearch.js]
 [browser_keywordSearch_postData.js]
 uses-unsafe-cpows = true
 support-files =
   POSTSearchEngine.xml
new file mode 100644
--- /dev/null
+++ b/browser/components/urlbar/tests/browser/browser_inputHistory_emptystring.js
@@ -0,0 +1,111 @@
+/* eslint-disable mozilla/no-arbitrary-setTimeout */
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * This tests input history in cases where the search string is empty.
+ * In the future we may want to not account for these, but for now they are
+ * stored with an empty input field.
+ */
+
+"use strict";
+
+async function checkInputHistory(len = 0) {
+  await PlacesUtils.withConnectionWrapper("test::checkInputHistory", async db => {
+    let rows = await db.executeCached(`SELECT input FROM moz_inputhistory`);
+    Assert.equal(rows.length, len, "There should only be 1 entry");
+    if (len) {
+      Assert.equal(rows[0].getResultByIndex(0), "", "Input should be empty");
+    }
+  });
+}
+
+async function clearInputHistory() {
+  await PlacesUtils.withConnectionWrapper("test::clearInputHistory", db => {
+    return db.executeCached(`DELETE FROM moz_inputhistory`);
+  });
+ }
+
+const TEST_URL = "http://example.com/";
+
+async function do_test(openFn, pickMethod) {
+  await BrowserTestUtils.withNewTab({
+    gBrowser,
+    url: "about:blank",
+  }, async function(browser) {
+    await clearInputHistory();
+    if (!await openFn()) {
+      return;
+    }
+    await UrlbarTestUtils.promiseSearchComplete(window);
+    let promise = BrowserTestUtils.waitForDocLoadAndStopIt(TEST_URL, browser);
+    if (pickMethod == "keyboard") {
+      info(`Test pressing Enter`);
+      EventUtils.sendKey("down");
+      EventUtils.sendKey("return");
+    } else {
+      info("Test with click");
+      let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
+      EventUtils.synthesizeMouseAtCenter(result.element.row, {});
+    }
+    await promise;
+    await checkInputHistory(1);
+  });
+}
+
+add_task(async function setup() {
+  await PlacesUtils.history.clear();
+  await PlacesTestUtils.addVisits(TEST_URL);
+  registerCleanupFunction(async () => {
+    await PlacesUtils.history.clear();
+  });
+});
+
+add_task(async function test_history_no_search_terms() {
+  for (let pickMethod of ["keyboard", "mouse"]) {
+    // If a testFn returns false, it will be skipped.
+    for (let openFn of [
+      () => {
+        info("Test opening panel with down key");
+        gURLBar.focus();
+        EventUtils.sendKey("down");
+        return true;
+      },
+      () => {
+        info("Test opening panel with history dropmarker");
+        UrlbarTestUtils.getDropMarker(window).click();
+        return true;
+      },
+      async () => {
+        info("Test opening panel with history dropmarker on a page");
+        let selectedBrowser = gBrowser.selectedBrowser;
+        await BrowserTestUtils.loadURI(selectedBrowser, TEST_URL);
+        await BrowserTestUtils.browserLoaded(selectedBrowser);
+        UrlbarTestUtils.getDropMarker(window).click();
+        return true;
+      },
+      // The following tests must be the last to run because of the pref change.
+      async () => {
+        info("Test opening panel on focus");
+        Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
+        gURLBar.blur();
+        EventUtils.synthesizeMouseAtCenter(gURLBar.textbox, {});
+        registerCleanupFunction(() => {
+          Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
+        });
+        return UrlbarPrefs.get("quantumbar");
+      },
+      async () => {
+        info("Test opening panel on focus on a page");
+        let selectedBrowser = gBrowser.selectedBrowser;
+        await BrowserTestUtils.loadURI(selectedBrowser, TEST_URL);
+        await BrowserTestUtils.browserLoaded(selectedBrowser);
+        gURLBar.blur();
+        EventUtils.synthesizeMouseAtCenter(gURLBar.textbox, {});
+        return UrlbarPrefs.get("quantumbar");
+      },
+    ]) {
+      await do_test(openFn, pickMethod);
+    }
+  }
+});
--- a/browser/components/urlbar/tests/legacy/browser.ini
+++ b/browser/components/urlbar/tests/legacy/browser.ini
@@ -53,16 +53,17 @@ skip-if = os != "mac" # Mac only feature
 [../browser/browser_autoFill_trimURLs.js]
 [../browser/browser_autoFill_typed.js]
 [../browser/browser_autoFill_undo.js]
 [../browser/browser_autocomplete_tag_star_visibility.js]
 [../browser/browser_canonizeURL.js]
 [../browser/browser_dragdropURL.js]
 [../browser/browser_ime_composition.js]
 [../browser/browser_inputHistory.js]
+[../browser/browser_inputHistory_emptystring.js]
 [../browser/browser_keywordBookmarklets.js]
 [../browser/browser_keepStateAcrossTabSwitches.js]
 [../browser/browser_keyword_override.js]
 [../browser/browser_keywordSearch.js]
 [../browser/browser_keywordSearch_postData.js]
 uses-unsafe-cpows = true
 support-files =
   ../browser/POSTSearchEngine.xml