Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao
☠☠ backed out by 76d6cbd3793b ☠ ☠
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 03 Jul 2019 20:42:30 +0000
changeset 540827 a2ed47577d891e1c4daf92d461d8af8915c33d7a
parent 540826 8ad5fbc5b9358fc84aa43d9a1b19c851056b1f39
child 540828 cd9291b975b66d5442064ae91cf13f2ea920e845
push id11529
push userarchaeopteryx@coole-files.de
push dateThu, 04 Jul 2019 15:22:33 +0000
treeherdermozilla-beta@ebb510a784b8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, dao
bugs1562585, 1562823
milestone69.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 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao 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 exists only for tests.
     this._enableAutofillPlaceholder = true;
 
     // Forward textbox methods and properties.
@@ -489,17 +490,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);
@@ -547,17 +548,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) {
@@ -629,45 +633,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._textValueOnLastSearche = 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"),
@@ -1332,27 +1330,31 @@ class UrlbarInput {
     }
 
     if (event.currentTarget == this.inputField) {
       if (event.detail == 2 &&
           UrlbarPrefs.get("doubleClickSelectsAll")) {
         this.editor.selectAll();
         event.preventDefault();
       } else if (this.openViewOnFocus && !this.view.isOpen) {
-        this.startQuery({ allowAutofill: false });
+        this.startQuery({
+          allowAutofill: false,
+        });
       }
       return;
     }
 
     if (event.originalTarget.classList.contains("urlbar-history-dropmarker")) {
       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;
@@ -1401,17 +1403,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