Bug 1394207 - Only fire the SidebarFocused event for non-startup entry points;r=Gijs
authorBrian Grinstead <bgrinstead@mozilla.com>
Wed, 06 Sep 2017 08:49:07 -0700
changeset 428701 9435660523cb463d22b032c2f0d88b7565a7ab89
parent 428700 16290ce48f84f5c2224894155291c894ad759d0c
child 428702 5edfd440c97f26c484add1826c985d05c4cdb5cc
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1394207
milestone57.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 1394207 - Only fire the SidebarFocused event for non-startup entry points;r=Gijs If the sidebar is being opened during startup (either at delayedLoad or when being adopted from another window), we don't want sidebar panels to steal focus from the page MozReview-Commit-ID: 4mBP2dQdyKY
browser/base/content/browser-sidebar.js
browser/base/content/test/sidebar/browser.ini
browser/base/content/test/sidebar/browser_sidebar_adopt.js
--- a/browser/base/content/browser-sidebar.js
+++ b/browser/base/content/browser-sidebar.js
@@ -324,16 +324,18 @@ var SidebarUI = {
    * @param {DOMNode} [triggerNode] Node, usually a button, that triggered the
    *                                showing of the sidebar.
    */
   show(commandID, triggerNode) {
     return this._show(commandID).then(() => {
       if (triggerNode) {
         updateToggleControlLabel(triggerNode);
       }
+
+      this._fireFocusedEvent();
       BrowserUITelemetry.countSidebarEvent(commandID, "show");
     });
   },
 
   /**
    * Implementation for show. Also used internally for sidebars that are shown
    * when a window is opened and we don't want to ping telemetry.
    *
@@ -379,30 +381,26 @@ var SidebarUI = {
         this.title = title;
       }
 
       let url = sidebarBroadcaster.getAttribute("sidebarurl");
       this.browser.setAttribute("src", url); // kick off async load
 
       if (this.browser.contentDocument.location.href != url) {
         this.browser.addEventListener("load", event => {
+          // We're handling the 'load' event before it bubbles up to the usual
+          // (non-capturing) event handlers. Let it bubble up before resolving.
+          setTimeout(() => {
+            resolve();
 
-          // We're handling the 'load' event before it bubbles up to the usual
-          // (non-capturing) event handlers. Let it bubble up before firing the
-          // SidebarFocused event.
-          setTimeout(() => this._fireFocusedEvent(), 0);
-
-          resolve();
-
-          // Now that the currentId is updated, fire a show event.
-          this._fireShowEvent();
+            // Now that the currentId is updated, fire a show event.
+            this._fireShowEvent();
+          }, 0);
         }, {capture: true, once: true});
       } else {
-        // Older code handled this case, so we do it too.
-        this._fireFocusedEvent();
         resolve();
 
         // Now that the currentId is updated, fire a show event.
         this._fireShowEvent();
       }
 
       let selBrowser = gBrowser.selectedBrowser;
       selBrowser.messageManager.sendAsyncMessage("Sidebar:VisibilityChange",
--- a/browser/base/content/test/sidebar/browser.ini
+++ b/browser/base/content/test/sidebar/browser.ini
@@ -1,5 +1,6 @@
 [DEFAULT]
 
 [browser_bug409481.js]
+[browser_sidebar_adopt.js]
 [browser_sidebar_move.js]
 [browser_sidebar_switcher.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/sidebar/browser_sidebar_adopt.js
@@ -0,0 +1,46 @@
+
+/* This test checks that the SidebarFocused event doesn't fire in adopted
+ * windows when the sidebar gets opened during window opening, to make sure
+ * that sidebars don't steal focus from the page in this case (Bug 1394207).
+ * There's another case not covered here that has the same expected behavior -
+ * during the initial browser startup - but it would be hard to do with a mochitest. */
+
+registerCleanupFunction(() => {
+  SidebarUI.hide();
+});
+
+function failIfSidebarFocusedFires() {
+  ok(false, "This event shouldn't have fired");
+}
+
+add_task(async function() {
+  info("Opening the sidebar and expecting both SidebarShown and SidebarFocused events");
+
+  let initialShown = BrowserTestUtils.waitForEvent(window, "SidebarShown");
+  let initialFocus = BrowserTestUtils.waitForEvent(window, "SidebarFocused");
+
+  await SidebarUI.show("viewBookmarksSidebar");
+  await initialShown;
+  await initialFocus;
+
+  ok(true, "SidebarShown and SidebarFocused events fired on a new window");
+});
+
+add_task(async function() {
+  info("Opening a new window and expecting the SidebarFocused event to not fire");
+
+  let promiseNewWindow = BrowserTestUtils.waitForNewWindow(false);
+  BrowserTestUtils.openNewBrowserWindow({opener: window});
+  let win = await promiseNewWindow;
+
+  let adoptedShown = BrowserTestUtils.waitForEvent(win, "SidebarShown");
+  win.addEventListener("SidebarFocused", failIfSidebarFocusedFires);
+
+  registerCleanupFunction(async function() {
+    win.removeEventListener("SidebarFocused", failIfSidebarFocusedFires);
+    await BrowserTestUtils.closeWindow(win);
+  });
+
+  await adoptedShown;
+  ok(true, "SidebarShown event fired on an adopted window");
+});