Bug 1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar. r=Gijs
authorDrew Willcoxon <adw@mozilla.com>
Fri, 30 Mar 2018 09:34:47 -0700
changeset 410798 a86e795540dccdc02752f7233c91c66e20735490
parent 410797 206b41cdbafdfee947488a61f3ee40bd3bd9880b
child 410799 70c80f12ce8e9bacfcf2c14c555014de6f8ec17b
push id61967
push userdwillcoxon@mozilla.com
push dateFri, 30 Mar 2018 16:37:09 +0000
treeherderautoland@a86e795540dc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1449947
milestone61.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 1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar. r=Gijs MozReview-Commit-ID: 5H9XfRXq8eO
browser/base/content/browser-pageActions.js
browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js
browser/base/content/test/urlbar/head.js
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -1201,39 +1201,42 @@ BrowserPageActions.addSearchEngine = {
     }
     for (let engine of this.engines) {
       let button = document.createElement("toolbarbutton");
       button.classList.add("subviewbutton", "subviewbutton-iconic");
       button.setAttribute("label", engine.title);
       button.setAttribute("image", engine.icon);
       button.setAttribute("uri", engine.uri);
       button.addEventListener("command", event => {
-        PanelMultiView.hidePopup(BrowserPageActions.panelNode);
-        this._handleClickOnEngineButton(button);
+        let panelNode = panelViewNode.closest("panel");
+        PanelMultiView.hidePopup(panelNode);
+        this._installEngine(button.getAttribute("uri"),
+                            button.getAttribute("image"));
       });
       body.appendChild(button);
     }
   },
 
   onCommand(event, buttonNode) {
     if (!buttonNode.closest("panel")) {
       // The urlbar button was clicked.  It should have a subview if there are
       // many engines.
       let manyEngines = this.engines.length > 1;
       this.action.setWantsSubview(manyEngines, window);
       if (manyEngines) {
         return;
       }
     }
-    this._handleClickOnEngineButton(buttonNode);
-  },
-
-  _handleClickOnEngineButton(button) {
-    this._installEngine(button.getAttribute("uri"),
-                        button.getAttribute("image"));
+    // Either the panel button or urlbar button was clicked -- not a button in
+    // the subview -- but in either case, there's only one search engine.
+    // (Because this method isn't called when the panel button is clicked and it
+    // shows a subview, and the many-engines case for the urlbar returned early
+    // above.)
+    let engine = this.engines[0];
+    this._installEngine(engine.uri, engine.icon);
   },
 
   _installEngine(uri, image) {
     Services.search.addEngine(uri, null, image, false, {
       onSuccess: engine => {
         BrowserPageActionFeedback.show(this.action, {
           text: this.strings.GetStringFromName("searchAddedFoundEngine"),
         });
--- a/browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js
+++ b/browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js
@@ -1,11 +1,11 @@
 "use strict";
 
-// Checks a page that doesn't offer any engines.
+// Checks the panel button with a page that doesn't offer any engines.
 add_task(async function none() {
   let url = "http://mochi.test:8888/";
   await BrowserTestUtils.withNewTab(url, async () => {
     // Open the panel.
     await promisePageActionPanelOpen();
     EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
     await promisePageActionPanelHidden();
 
@@ -15,17 +15,17 @@ add_task(async function none() {
               "Action should not be present in panel");
     let button =
       BrowserPageActions.panelButtonNodeForActionID("addSearchEngine");
     Assert.ok(!button, "Action button should not be in panel");
   });
 });
 
 
-// Checks a page that offers one engine.
+// Checks the panel button with a page that offers one engine.
 add_task(async function one() {
   let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_one.html";
   await BrowserTestUtils.withNewTab(url, async () => {
     // Open the panel.
     await promisePageActionPanelOpen();
 
     // The action should be present.
     let actions = PageActions.actionsInPanel(window);
@@ -40,17 +40,17 @@ add_task(async function one() {
     Assert.equal(button.label, expectedTitle, "Button label");
     Assert.equal(button.classList.contains("subviewbutton-nav"), false,
                  "Button should not expand into a subview");
 
     // Click the action's button.
     let enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
     let hiddenPromise = promisePageActionPanelHidden();
-    let feedbackPromise = promiseFeedbackPanelHidden();
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(button, {});
     await hiddenPromise;
     let engine = await enginePromise;
     let feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown");
 
     // Open the panel again.
     await promisePageActionPanelOpen();
@@ -84,17 +84,17 @@ add_task(async function one() {
     Assert.ok(button, "Action button should be in panel");
     Assert.equal(button.label, expectedTitle, "Button label");
     Assert.equal(button.classList.contains("subviewbutton-nav"), false,
                  "Button should not expand into a subview");
   });
 });
 
 
-// Checks a page that offers many engines.
+// Checks the panel button with a page that offers many engines.
 add_task(async function many() {
   let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_many.html";
   await BrowserTestUtils.withNewTab(url, async () => {
     // Open the panel.
     await promisePageActionPanelOpen();
 
     // The action should be present.
     let actions = PageActions.actionsInPanel(window);
@@ -127,17 +127,17 @@ add_task(async function many() {
       ],
       "Subview children"
     );
 
     // Click the first engine to install it.
     let enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
     let hiddenPromise = promisePageActionPanelHidden();
-    let feedbackPromise = promiseFeedbackPanelHidden();
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
     await hiddenPromise;
     let engines = [];
     let engine = await enginePromise;
     engines.push(engine);
     let feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
 
@@ -155,17 +155,17 @@ add_task(async function many() {
       ],
       "Subview children"
     );
 
     // Click the next engine to install it.
     enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_1");
     hiddenPromise = promisePageActionPanelHidden();
-    feedbackPromise = promiseFeedbackPanelHidden();
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
     await hiddenPromise;
     engine = await enginePromise;
     engines.push(engine);
     feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
 
     // Open the panel again.  This time the action button should show the one
@@ -182,17 +182,17 @@ add_task(async function many() {
     Assert.equal(button.label, expectedTitle, "Button label");
     Assert.equal(button.classList.contains("subviewbutton-nav"), false,
                  "Button should not expand into a subview");
 
     // Click the button.
     enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_2");
     hiddenPromise = promisePageActionPanelHidden();
-    feedbackPromise = promiseFeedbackPanelHidden();
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(button, {});
     await hiddenPromise;
     engine = await enginePromise;
     engines.push(engine);
     feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
 
     // All engines are installed at this point.  Open the panel and make sure
@@ -231,17 +231,17 @@ add_task(async function many() {
 
     // Remove the second engine.
     enginePromise =
       promiseEngine("engine-removed", "page_action_menu_add_search_engine_1");
     Services.search.removeEngine(engines.shift());
     await enginePromise;
 
     // Open the panel again and check the subview.  The subview should be
-    // present now that there are two offerred engines again.
+    // present now that there are two offered engines again.
     await promisePageActionPanelOpen();
     actions = PageActions.actionsInPanel(window);
     action = actions.find(a => a.id == "addSearchEngine");
     Assert.ok(action, "Action should be present in panel");
     expectedTitle = "Add One-Click Search Engine";
     Assert.equal(action.getTitle(window), expectedTitle, "Action title");
     button = BrowserPageActions.panelButtonNodeForActionID("addSearchEngine");
     Assert.ok(button, "Button should be in panel");
@@ -284,22 +284,263 @@ add_task(async function many() {
       "Subview children"
     );
     EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
     await promisePageActionPanelHidden();
   });
 });
 
 
+// Checks the urlbar button with a page that offers one engine.
+add_task(async function urlbarOne() {
+  let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_one.html";
+  await BrowserTestUtils.withNewTab(url, async () => {
+    await promiseNodeVisible(BrowserPageActions.mainButtonNode);
+
+    // Pin the action to the urlbar.
+    let placedPromise = promisePlacedInUrlbar();
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = true;
+
+    // It should be placed.
+    let button = await placedPromise;
+    let actions = PageActions.actionsInUrlbar(window);
+    let action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Action button should be in urlbar");
+
+    // Click the action's button.
+    let enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    let engine = await enginePromise;
+    let feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown");
+
+    // The action should be gone.
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(!action, "Action should not be present in urlbar");
+    button = BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    Assert.ok(!button, "Action button should not be in urlbar");
+
+    // Remove the engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_0");
+    placedPromise = promisePlacedInUrlbar();
+    Services.search.removeEngine(engine);
+    await enginePromise;
+
+    // The action should be present again.
+    button = await placedPromise;
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Action button should be in urlbar");
+
+    // Clean up.
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = false;
+    await BrowserTestUtils.waitForCondition(() => {
+      return !BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    });
+  });
+});
+
+
+// Checks the urlbar button with a page that offers many engines.
+add_task(async function urlbarMany() {
+  let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_many.html";
+  await BrowserTestUtils.withNewTab(url, async () => {
+    await promiseNodeVisible(BrowserPageActions.mainButtonNode);
+
+    // Pin the action to the urlbar.
+    let placedPromise = promisePlacedInUrlbar();
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = true;
+
+    // It should be placed.
+    let button = await placedPromise;
+    let actions = PageActions.actionsInUrlbar(window);
+    let action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Action button should be in urlbar");
+
+    // Click the action's button.  The activated-action panel should open, and
+    // it should contain the addSearchEngine subview.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    let view = await waitForActivatedActionPanel();
+    let viewID =
+       BrowserPageActions._panelViewNodeIDForActionID("addSearchEngine", true);
+    Assert.equal(view.id, viewID, "View ID");
+    let body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_0",
+        "page_action_menu_add_search_engine_1",
+        "page_action_menu_add_search_engine_2",
+      ],
+      "Subview children"
+    );
+
+    // Click the first engine to install it.
+    let enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
+    let hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
+    await hiddenPromise;
+    let engines = [];
+    let engine = await enginePromise;
+    engines.push(engine);
+    let feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
+
+    // Open the panel again.  The installed engine should be gone.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    view = await waitForActivatedActionPanel();
+    body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_1",
+        "page_action_menu_add_search_engine_2",
+      ],
+      "Subview children"
+    );
+
+    // Click the next engine to install it.
+    enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_1");
+    hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
+    await hiddenPromise;
+    engine = await enginePromise;
+    engines.push(engine);
+    feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
+
+    // Now there's only one engine left, so clicking the button should simply
+    // install it instead of opening the activated-action panel.
+    enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_2");
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    engine = await enginePromise;
+    engines.push(engine);
+    feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
+
+    // All engines are installed at this point.  The action should be gone.
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(!action, "Action should be gone");
+    button = BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    Assert.ok(!button, "Button should not be in urlbar");
+
+    // Remove the first engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_0");
+    placedPromise = promisePlacedInUrlbar();
+    Services.search.removeEngine(engines.shift());
+    await enginePromise;
+
+    // The action should be placed again.
+    button = await placedPromise;
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Button should be in urlbar");
+
+    // Remove the second engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_1");
+    Services.search.removeEngine(engines.shift());
+    await enginePromise;
+
+    // Open the panel again and check the subview.  The subview should be
+    // present now that there are two offered engines again.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    view = await waitForActivatedActionPanel();
+    body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_0",
+        "page_action_menu_add_search_engine_1",
+      ],
+      "Subview children"
+    );
+
+    // Hide the panel.
+    hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    await hiddenPromise;
+
+    // Remove the third engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_2");
+    Services.search.removeEngine(engines.shift());
+    await enginePromise;
+
+    // Open the panel again and check the subview.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    view = await waitForActivatedActionPanel();
+    body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_0",
+        "page_action_menu_add_search_engine_1",
+        "page_action_menu_add_search_engine_2",
+      ],
+      "Subview children"
+    );
+
+    // Hide the panel.
+    hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    await hiddenPromise;
+
+    // Clean up.
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = false;
+    await BrowserTestUtils.waitForCondition(() => {
+      return !BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    });
+  });
+});
+
+
 function promiseEngine(expectedData, expectedEngineName) {
+  info(`Waiting for engine ${expectedData}`);
   return TestUtils.topicObserved("browser-search-engine-modified", (engine, data) => {
+    info(`Got engine ${engine.wrappedJSObject.name} ${data}`);
     return expectedData == data &&
            expectedEngineName == engine.wrappedJSObject.name;
   }).then(([engine, data]) => engine);
 }
 
-function promiseFeedbackPanelHidden() {
-  return new Promise(resolve => {
-    BrowserPageActionFeedback.panelNode.addEventListener("popuphidden", event => {
-      resolve(BrowserPageActionFeedback.feedbackLabel.textContent);
-    }, {once: true});
+function promiseFeedbackPanelShownAndHidden() {
+  info("Waiting for feedback panel popupshown");
+  return BrowserTestUtils.waitForEvent(BrowserPageActionFeedback.panelNode, "popupshown").then(() => {
+    info("Got feedback panel popupshown. Now waiting for popuphidden");
+    return BrowserTestUtils.waitForEvent(BrowserPageActionFeedback.panelNode, "popuphidden")
+          .then(() => BrowserPageActionFeedback.feedbackLabel.textContent);
   });
 }
+
+function promisePlacedInUrlbar() {
+  let action = PageActions.actionForID("addSearchEngine");
+  return new Promise(resolve => {
+    let onPlaced = action._onPlacedInUrlbar;
+    action._onPlacedInUrlbar = button => {
+      action._onPlacedInUrlbar = onPlaced;
+      if (action._onPlacedInUrlbar) {
+        action._onPlacedInUrlbar(button);
+      }
+      promiseNodeVisible(button).then(() => resolve(button));
+    };
+  });
+}
--- a/browser/base/content/test/urlbar/head.js
+++ b/browser/base/content/test/urlbar/head.js
@@ -242,16 +242,48 @@ function promisePageActionPanelOpen() {
     EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
     return shownPromise;
   }).then(() => {
     // Wait for items in the panel to become visible.
     return promisePageActionViewChildrenVisible(BrowserPageActions.mainViewNode);
   });
 }
 
+async function waitForActivatedActionPanel() {
+  if (!BrowserPageActions.activatedActionPanelNode) {
+    info("Waiting for activated-action panel to be added to mainPopupSet");
+    await new Promise(resolve => {
+      let observer = new MutationObserver(mutations => {
+        if (BrowserPageActions.activatedActionPanelNode) {
+          observer.disconnect();
+          resolve();
+        }
+      });
+      let popupSet = document.getElementById("mainPopupSet");
+      observer.observe(popupSet, { childList: true });
+    });
+    info("Activated-action panel added to mainPopupSet");
+  }
+  if (!BrowserPageActions.activatedActionPanelNode.state == "open") {
+    info("Waiting for activated-action panel popupshown");
+    await promisePanelShown(BrowserPageActions.activatedActionPanelNode);
+    info("Got activated-action panel popupshown");
+  }
+  let panelView =
+    BrowserPageActions.activatedActionPanelNode.querySelector("panelview");
+  if (panelView) {
+    await BrowserTestUtils.waitForEvent(
+      BrowserPageActions.activatedActionPanelNode,
+      "ViewShown"
+    );
+    await promisePageActionViewChildrenVisible(panelView);
+  }
+  return panelView;
+}
+
 function promisePageActionPanelShown() {
   return promisePanelShown(BrowserPageActions.panelNode);
 }
 
 function promisePageActionPanelHidden() {
   return promisePanelHidden(BrowserPageActions.panelNode);
 }
 
@@ -288,26 +320,28 @@ function promisePageActionViewShown() {
   return BrowserTestUtils.waitForEvent(BrowserPageActions.panelNode, "ViewShown").then(async event => {
     let panelViewNode = event.originalTarget;
     await promisePageActionViewChildrenVisible(panelViewNode);
     return panelViewNode;
   });
 }
 
 function promisePageActionViewChildrenVisible(panelViewNode) {
-  info("promisePageActionViewChildrenVisible waiting for a child node to be visible");
+  return promiseNodeVisible(panelViewNode.firstChild.firstChild);
+}
+
+function promiseNodeVisible(node) {
+  info(`promiseNodeVisible waiting, node.id=${node.id} node.localeName=${node.localName}\n`);
   let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                   .getInterface(Ci.nsIDOMWindowUtils);
   return BrowserTestUtils.waitForCondition(() => {
-    let bodyNode = panelViewNode.firstChild;
-    for (let childNode of bodyNode.childNodes) {
-      let bounds = dwu.getBoundsWithoutFlushing(childNode);
-      if (bounds.width > 0 && bounds.height > 0) {
-        return true;
-      }
+    let bounds = dwu.getBoundsWithoutFlushing(node);
+    if (bounds.width > 0 && bounds.height > 0) {
+      info(`promiseNodeVisible OK, node.id=${node.id} node.localeName=${node.localName}\n`);
+      return true;
     }
     return false;
   });
 }
 
 function promiseSpeculativeConnection(httpserver) {
   return BrowserTestUtils.waitForCondition(() => {
     if (httpserver) {