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,a=gchang
authorMark Banner <standard8@mozilla.com>
Thu, 20 Oct 2016 10:17:20 +0100
changeset 436568 f29e3dc3f53e186ff2822be1d28f50b2e76853d2
parent 436567 1a8ed7ba82adecdd095ae3d7d99bcf672ef7e976
child 436569 3888f1a5702b34824b618b2110e09be7965477c5
push id35151
push userbmo:bob.silverberg@gmail.com
push dateWed, 09 Nov 2016 13:14:58 +0000
reviewersflorian, gchang
bugs1297374
milestone51.0a2
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,a=gchang 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
@@ -1385,25 +1385,25 @@ file, You can obtain one at http://mozil
         ]]></body>
       </method>
 
       <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,195 @@
+"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 ONEOFF_URLBAR_PREF = "browser.urlbar.oneOffSearches";
+
+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() {
+  Services.prefs.setBoolPref(ONEOFF_URLBAR_PREF, true);
+  registerCleanupFunction(function* () {
+    Services.prefs.clearUserPref(ONEOFF_URLBAR_PREF);
+  });
+
+  // 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);
+}