Bug 1559264 - Quantumbar: Don't call setValueFromResult when opening the history popup. r=dao a=ritu
authorDrew Willcoxon <adw@mozilla.com>
Fri, 21 Jun 2019 18:37:27 +0000
changeset 537066 60e8af0747b4ae12b440b3e406ffedd4ccc71d5f
parent 537065 3d562db5d1ed358a18da5bbe160c1da4864ef6c0
child 537067 8fb74fcbc12c83a230f700196e83193e6749d740
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)
reviewersdao, ritu
bugs1559264, 1529931
milestone68.0
Bug 1559264 - Quantumbar: Don't call setValueFromResult when opening the history popup. r=dao a=ritu The problem is that on switching back to the first tab (see the bug), userTypedValue is non-null when URLBarSetURI is called. Therefore the proxy state can't be valid. Something about bug 1529931 caused userTypedValue to go from null to non-null in this case. Details below, but the summary is that we shouldn't be calling UrlbarInput.setValueFromResult when opening the history popup, because setValueFromResult sets userTypedValue. Before bug 1529931, result.autofill would always be undefined for the first result in the history popup, because we didn't allow UnifiedComplete to return an autofill result for the search triggered by the history popup. After that bug, UnifiedComplete could return an autofill result in that case -- and it likely would since the first result in the history popup has a very high frecency, which also makes it eligible for autofill. The problem with having an autofill result in the history popup is that it triggers the input.setValueFromResult() call in UrlbarController.receiveResults [1], and setValueFromResult sets userTypedValue. So when the user opens the history popup, userTypedValue gets set to a non-null value (input._lastSearchString). The fix is to not allow autofill for the history popup. After making that fix on revision https://hg.mozilla.org/mozilla-central/rev/5e2a3b886e64, the bug went away. However, after I made that fix on a fresh tree, the bug still happened. It turns out that input.setValueFromResult still ends up getting called, by UrlbarView._selectItem [2], which is called when results are received [3]. The fix for this afaict is just to pass `updateInput: false` to _selectItem. The autofill-related fix doesn't seem to be necessary at all anymore (likely due to the substantial changes to autofill since that bug landed), but I left it in anyway since it seems right to not allow autofill results for the history popup. One other useful bit of info is that userTypedValue is set to null by tabbrowser on page load [4], so that's how userTypedValue has a null value when the bug manifests and it goes from null to non-null. [1] https://hg.mozilla.org/mozilla-central/file/5e2a3b886e647af1968b9e52a6672bdeee2a0d6f/browser/components/urlbar/UrlbarController.jsm#l150 [2] https://searchfox.org/mozilla-central/rev/da14c413ef663eb1ba246799e94a240f81c42488/browser/components/urlbar/UrlbarView.jsm#685 [3] https://searchfox.org/mozilla-central/rev/da14c413ef663eb1ba246799e94a240f81c42488/browser/components/urlbar/UrlbarView.jsm#220 [4] https://searchfox.org/mozilla-central/rev/da14c413ef663eb1ba246799e94a240f81c42488/browser/base/content/tabbrowser.js#5118 Differential Revision: https://phabricator.services.mozilla.com/D35505
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarView.jsm
browser/components/urlbar/tests/browser/browser_dropmarker.js
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -1302,17 +1302,17 @@ class UrlbarInput {
       return;
     }
 
     if (event.originalTarget.classList.contains("urlbar-history-dropmarker") &&
         event.button == 0) {
       if (this.view.isOpen) {
         this.view.close();
       } else {
-        this.startQuery();
+        this.startQuery({ allowAutofill: false });
       }
     }
   }
 
   _on_input(event) {
     let value = this.textValue;
     this.valueIsTyped = true;
     this._untrimmedValue = value;
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@@ -212,17 +212,19 @@ class UrlbarView {
       if (queryContext.preselected) {
         isFirstPreselectedResult = true;
         this._selectItem(this._rows.firstElementChild, {
           updateInput: false,
           setAccessibleFocus: this.controller._userSelectionBehavior == "arrow",
         });
       } else {
         // Clear the selection when we get a new set of results.
-        this._selectItem(null);
+        this._selectItem(null, {
+          updateInput: false,
+        });
       }
       // Hide the one-off search buttons if the search string is empty, or
       // starts with a potential @ search alias or the search restriction
       // character.
       let trimmedValue = queryContext.searchString.trim();
       this._enableOrDisableOneOffSearches(
         trimmedValue &&
         trimmedValue[0] != "@" &&
--- a/browser/components/urlbar/tests/browser/browser_dropmarker.js
+++ b/browser/components/urlbar/tests/browser/browser_dropmarker.js
@@ -1,20 +1,52 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/
  */
 
 "use strict";
 
-add_task(async function() {
-  await BrowserTestUtils.withNewTab({ gBrowser, url: "http://example.com/" }, async () => {
-    await UrlbarTestUtils.promisePopupOpen(window, () => {
-      let historyDropMarker =
-        window.document.getAnonymousElementByAttribute(gURLBar.textbox, "anonid", "historydropmarker");
-      EventUtils.synthesizeMouseAtCenter(historyDropMarker, {}, window);
-    });
-    let queryContext = await gURLBar.lastQueryContextPromise;
+add_task(async function basic() {
+  await BrowserTestUtils.withNewTab("http://example.com/", async () => {
+    let queryContext = await clickDropmarker();
     is(queryContext.searchString, "",
        "Clicking the history dropmarker should initiate an empty search instead of searching for the loaded URL");
-    is(gURLBar.value, "example.com",
+    is(gURLBar.value, "http://example.com/",
        "Clicking the history dropmarker should not change the input value");
+    await UrlbarTestUtils.promisePopupClose(window,
+      () => EventUtils.synthesizeKey("KEY_Escape"));
   });
 });
+
+add_task(async function proxyState() {
+  // Open two tabs.
+  await BrowserTestUtils.withNewTab("about:blank", async browser1 => {
+    await BrowserTestUtils.withNewTab("http://example.com/", async browser2 => {
+      // Click the dropmarker on the second tab and switch back to the first
+      // tab.
+      await clickDropmarker();
+      await UrlbarTestUtils.promisePopupClose(window,
+        () => EventUtils.synthesizeKey("KEY_Escape"));
+      await BrowserTestUtils.switchTab(
+        gBrowser,
+        gBrowser.getTabForBrowser(browser1)
+      );
+      // Switch back to the second tab.
+      await BrowserTestUtils.switchTab(
+        gBrowser,
+        gBrowser.getTabForBrowser(browser2)
+      );
+      // The proxy state should be valid.
+      Assert.equal(gURLBar.getAttribute("pageproxystate"), "valid");
+    });
+  });
+});
+
+async function clickDropmarker() {
+  await UrlbarTestUtils.promisePopupOpen(window, () => {
+    let historyDropMarker =
+      window.document.getAnonymousElementByAttribute(gURLBar.textbox, "anonid",
+                                                     "historydropmarker");
+    EventUtils.synthesizeMouseAtCenter(historyDropMarker, {}, window);
+  });
+  let queryContext = await gURLBar.lastQueryContextPromise;
+  return queryContext;
+}