Bug 1297374 - When setting default engine from the url bar, stop the new engine replacing the old icon as the url bar always shows all engines. r=florian
authorMark Banner <standard8@mozilla.com>
Thu, 20 Oct 2016 10:17:20 +0100
changeset 362174 b9ca193b71b30b2b20294389838ee4e166c6fa5a
parent 362173 f57f5762e4f74cc8369f78f3942f465f6baefff3
child 362175 77880cde0de11bf9c4e01f03cae985f3b9f04ae3
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-beta@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersflorian
bugs1297374
milestone52.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 1297374 - When setting default engine from the url bar, stop the new engine replacing the old icon as the url bar always shows all engines. r=florian Also ensure the telemetry origin is set correctly when the searchbar popup first opens. MozReview-Commit-ID: Fd7hc6BTVVQ
browser/base/content/urlbarBindings.xml
browser/components/search/content/search.xml
browser/components/search/test/browser.ini
browser/components/search/test/browser_oneOffContextMenu_setDefault.js
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -1367,25 +1367,25 @@ file, You can obtain one at http://mozil
                 onget="return this._overrideValue;"
                 onset="this._overrideValue = val; return val;"/>
 
       <method name="enableOneOffSearches">
         <parameter name="enable"/>
         <body><![CDATA[
           this._oneOffSearchesEnabled = enable;
           if (enable) {
+            this.oneOffSearchButtons.telemetryOrigin = "urlbar";
             this.oneOffSearchButtons.style.display = "-moz-box";
             this.oneOffSearchButtons.popup = this;
             this.oneOffSearchButtons.textbox = this.input;
-            this.oneOffSearchButtons.telemetryOrigin = "urlbar";
           } else {
+            this.oneOffSearchButtons.telemetryOrigin = null;
             this.oneOffSearchButtons.style.display = "none";
             this.oneOffSearchButtons.popup = null;
             this.oneOffSearchButtons.textbox = null;
-            this.oneOffSearchButtons.telemetryOrigin = null;
           }
         ]]></body>
       </method>
 
       <method name="openSearchSuggestionsNotificationLearnMoreURL">
         <body><![CDATA[
         let url = Services.urlFormatter.formatURL(
           Services.prefs.getCharPref("app.support.baseURL") + "suggestions"
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -971,20 +971,22 @@
                                ]) * -1;
           // Ensure the panel is wide enough to fit at least 3 engines.
           let minWidth = Math.max(
             parseInt(this.width) + magnifierWidth,
             this.oneOffButtons.buttonWidth * 3
           );
           this.style.minWidth = minWidth + "px";
 
+          // Set the origin before assigning the popup, as the assignment does
+          // a rebuild and would miss the origin.
+          this.oneOffButtons.telemetryOrigin = "searchbar";
           // Set popup after setting the minWidth since it builds the buttons.
           this.oneOffButtons.popup = this;
           this.oneOffButtons.textbox = this.input;
-          this.oneOffButtons.telemetryOrigin = "searchbar";
         }
 
         // First handle deciding if we are showing the reduced version of the
         // popup containing only the preferences button. We do this if the
         // glass icon has been clicked if the text field is empty.
         let searchbar = document.getElementById("searchbar");
         let tree = document.getAnonymousElementByAttribute(this, "anonid",
                                                            "tree")
@@ -2021,27 +2023,29 @@
           // Select the context-clicked button so that consumers can easily
           // tell which button was acted on.
           this.selectedButton = this._buttonForEngine(this._contextEngine);
           this.handleSearchCommand(event, this._contextEngine, true);
         }
         if (anonid == "search-one-offs-context-set-default") {
           let currentEngine = Services.search.currentEngine;
 
-          // Make the target button of the context menu reflect the current
-          // search engine first. Doing this as opposed to rebuilding all the
-          // one-off buttons avoids flicker.
-          let button = this._buttonForEngine(this._contextEngine);
-          button.id = this._buttonIDForEngine(currentEngine);
-          let uri = "chrome://browser/skin/search-engine-placeholder.png";
-          if (currentEngine.iconURI)
-            uri = currentEngine.iconURI.spec;
-          button.setAttribute("image", uri);
-          button.setAttribute("tooltiptext", currentEngine.name);
-          button.engine = currentEngine;
+          if (!this.getAttribute("includecurrentengine")) {
+            // Make the target button of the context menu reflect the current
+            // search engine first. Doing this as opposed to rebuilding all the
+            // one-off buttons avoids flicker.
+            let button = this._buttonForEngine(this._contextEngine);
+            button.id = this._buttonIDForEngine(currentEngine);
+            let uri = "chrome://browser/skin/search-engine-placeholder.png";
+            if (currentEngine.iconURI)
+              uri = currentEngine.iconURI.spec;
+            button.setAttribute("image", uri);
+            button.setAttribute("tooltiptext", currentEngine.name);
+            button.engine = currentEngine;
+          }
 
           Services.search.currentEngine = this._contextEngine;
         }
       ]]></handler>
 
       <handler event="contextmenu"><![CDATA[
         let target = event.originalTarget;
         // Prevent the context menu from appearing except on the one off buttons.
--- a/browser/components/search/test/browser.ini
+++ b/browser/components/search/test/browser.ini
@@ -25,16 +25,17 @@ support-files =
 skip-if = os == "mac" # bug 967013
 [browser_google.js]
 [browser_google_codes.js]
 [browser_google_behavior.js]
 [browser_healthreport.js]
 [browser_hiddenOneOffs_cleanup.js]
 [browser_hiddenOneOffs_diacritics.js]
 [browser_oneOffContextMenu.js]
+[browser_oneOffContextMenu_setDefault.js]
 [browser_oneOffHeader.js]
 [browser_private_search_perwindowpb.js]
 [browser_yahoo.js]
 [browser_yahoo_behavior.js]
 [browser_abouthome_behavior.js]
 skip-if = true # Bug ??????, Bug 1100301 - leaks windows until shutdown when --run-by-dir
 [browser_aboutSearchReset.js]
 [browser_searchbar_openpopup.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/search/test/browser_oneOffContextMenu_setDefault.js
@@ -0,0 +1,189 @@
+"use strict";
+
+const TEST_ENGINE_NAME = "Foo";
+const TEST_ENGINE_BASENAME = "testEngine.xml";
+const SEARCHBAR_BASE_ID = "searchbar-engine-one-off-item-";
+const URLBAR_BASE_ID = "urlbar-engine-one-off-item-";
+
+const searchbar = document.getElementById("searchbar");
+const urlbar = document.getElementById("urlbar");
+const searchPopup = document.getElementById("PopupSearchAutoComplete");
+const urlbarPopup = document.getElementById("PopupAutoCompleteRichResult");
+const searchIcon = document.getAnonymousElementByAttribute(
+  searchbar, "anonid", "searchbar-search-button"
+);
+const searchOneOffBinding = document.getAnonymousElementByAttribute(
+  searchPopup, "anonid", "search-one-off-buttons"
+);
+const urlBarOneOffBinding = document.getAnonymousElementByAttribute(
+  urlbarPopup, "anonid", "one-off-search-buttons"
+);
+
+let originalEngine = Services.search.currentEngine;
+
+function resetEngine() {
+  Services.search.currentEngine = originalEngine;
+}
+
+registerCleanupFunction(resetEngine);
+
+add_task(function* init() {
+  yield promiseNewEngine(TEST_ENGINE_BASENAME, {
+    setAsCurrent: false,
+  });
+});
+
+add_task(function* test_searchBarChangeEngine() {
+  let oneOffButton = yield openPopupAndGetEngineButton(true, searchPopup,
+                                                       searchOneOffBinding,
+                                                       SEARCHBAR_BASE_ID);
+
+  const setDefaultEngineMenuItem = document.getAnonymousElementByAttribute(
+    searchOneOffBinding, "anonid", "search-one-offs-context-set-default"
+  );
+
+  // Click the set default engine menu item.
+  let promise = promiseCurrentEngineChanged();
+  EventUtils.synthesizeMouseAtCenter(setDefaultEngineMenuItem, {});
+
+  // This also checks the engine correctly changed.
+  yield promise;
+
+  Assert.equal(oneOffButton.id, SEARCHBAR_BASE_ID + originalEngine.name,
+               "Should now have the original engine's id for the button");
+  Assert.equal(oneOffButton.getAttribute("tooltiptext"), originalEngine.name,
+               "Should now have the original engine's name for the tooltip");
+  Assert.equal(oneOffButton.image, originalEngine.iconURI.spec,
+               "Should now have the original engine's uri for the image");
+
+  yield promiseClosePopup(searchPopup);
+});
+
+add_task(function* test_urlBarChangeEngine() {
+  // Ensure the engine is reset.
+  resetEngine();
+
+  let oneOffButton = yield openPopupAndGetEngineButton(false, urlbarPopup,
+                                                       urlBarOneOffBinding,
+                                                       URLBAR_BASE_ID);
+
+  const setDefaultEngineMenuItem = document.getAnonymousElementByAttribute(
+    urlBarOneOffBinding, "anonid", "search-one-offs-context-set-default"
+  );
+
+  // Click the set default engine menu item.
+  let promise = promiseCurrentEngineChanged();
+  EventUtils.synthesizeMouseAtCenter(setDefaultEngineMenuItem, {});
+
+  // This also checks the engine correctly changed.
+  yield promise;
+
+  let currentEngine = Services.search.currentEngine;
+
+  // For the urlbar, we should keep the new engine's icon.
+  Assert.equal(oneOffButton.id, URLBAR_BASE_ID + currentEngine.name,
+               "Should now have the original engine's id for the button");
+  Assert.equal(oneOffButton.getAttribute("tooltiptext"), currentEngine.name,
+               "Should now have the original engine's name for the tooltip");
+  Assert.equal(oneOffButton.image, currentEngine.iconURI.spec,
+               "Should now have the original engine's uri for the image");
+
+  yield promiseClosePopup(urlbarPopup);
+});
+
+/**
+ * Promises that an engine change has happened for the current engine, which
+ * has resulted in the test engine now being the current engine.
+ *
+ * @return {Promise} Resolved once the test engine is set as the current engine.
+ */
+function promiseCurrentEngineChanged() {
+  return new Promise(resolve => {
+    function observer(aSub, aTopic, aData) {
+      if (aData == "engine-current") {
+        Assert.ok(Services.search.currentEngine.name, TEST_ENGINE_NAME, "currentEngine set");
+        Services.obs.removeObserver(observer, "browser-search-engine-modified");
+        resolve();
+      }
+    }
+
+    Services.obs.addObserver(observer, "browser-search-engine-modified", false);
+  });
+}
+
+/**
+ * Opens the specified urlbar/search popup and gets the test engine from the
+ * one-off buttons.
+ *
+ * @param {Boolean} isSearch true if the search popup should be opened; false
+ *                           for the urlbar popup.
+ * @param {Object} popup The expected popup.
+ * @param {Object} oneOffBinding The expected one-off-binding for the popup.
+ * @param {String} baseId The expected string for the id of the current
+ *                        engine button, without the engine name.
+ * @return {Object} Returns an object that represents the one off button for the
+ *                          test engine.
+ */
+function* openPopupAndGetEngineButton(isSearch, popup, oneOffBinding, baseId) {
+  // Open the popup.
+  let promise = promiseEvent(popup, "popupshown");
+  info("Opening panel");
+
+  // We have to open the popups in differnt ways.
+  if (isSearch) {
+    // Use the search icon to avoid hitting the network.
+    EventUtils.synthesizeMouseAtCenter(searchIcon, {});
+  } else {
+    // There's no history at this stage, so we need to press a key.
+    urlbar.focus();
+    EventUtils.synthesizeKey("a", {});
+  }
+  yield promise;
+
+  const contextMenu = document.getAnonymousElementByAttribute(
+    oneOffBinding, "anonid", "search-one-offs-context-menu"
+  );
+  const oneOffButtons = document.getAnonymousElementByAttribute(
+    oneOffBinding, "anonid", "search-panel-one-offs"
+  );
+
+  // Get the one-off button for the test engine.
+  let oneOffButton;
+  for (let node of oneOffButtons.childNodes) {
+    if (node.engine && node.engine.name == TEST_ENGINE_NAME) {
+      oneOffButton = node;
+      break;
+    }
+  }
+  Assert.notEqual(oneOffButton, undefined,
+                  "One-off for test engine should exist");
+  Assert.equal(oneOffButton.getAttribute("tooltiptext"), TEST_ENGINE_NAME,
+               "One-off should have the tooltip set to the engine name");
+  Assert.equal(oneOffButton.id, baseId + TEST_ENGINE_NAME,
+               "Should have the correct id");
+
+  // Open the context menu on the one-off.
+  promise = BrowserTestUtils.waitForEvent(contextMenu, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(oneOffButton, {
+    type: "contextmenu",
+    button: 2,
+  });
+  yield promise;
+
+  return oneOffButton;
+}
+
+/**
+ * Closes the popup and moves the mouse away from it.
+ *
+ * @param {Button} popup The popup to close.
+ */
+function* promiseClosePopup(popup) {
+  // close the panel using the escape key.
+  let promise = promiseEvent(popup, "popuphidden");
+  EventUtils.synthesizeKey("VK_ESCAPE", {});
+  yield promise;
+
+  // Move the cursor out of the panel area to avoid messing with other tests.
+  yield EventUtils.synthesizeNativeMouseMove(popup);
+}