Bug 1266134 - Cleanup focus workarounds r=jryans draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Tue, 25 Oct 2016 09:30:01 -0700
changeset 429835 00e529775763b3d1c51f61c9ddb49ac850e93f72
parent 429834 53ab86e19c06384858fb3e6c99efb0447ecc3ce9
child 535067 4227cd9b7f1246474071945313b84f8d8e3c91ea
push id33674
push userbmo:poirot.alex@gmail.com
push dateWed, 26 Oct 2016 17:35:06 +0000
reviewersjryans
bugs1266134
milestone52.0a1
Bug 1266134 - Cleanup focus workarounds r=jryans MozReview-Commit-ID: 4Em2QXwanta
devtools/client/framework/toolbox-host-manager.js
devtools/client/framework/toolbox.js
--- a/devtools/client/framework/toolbox-host-manager.js
+++ b/devtools/client/framework/toolbox-host-manager.js
@@ -12,19 +12,18 @@ loader.lazyRequireGetter(this, "Hosts", 
  * Implement a wrapper on the chrome side to setup a Toolbox within Firefox UI.
  *
  * This component handles iframe creation within Firefox, in which we are loading
  * the toolbox document. Then both the chrome and the toolbox document communicate
  * via "message" events.
  *
  * Messages sent by the toolbox to the chrome:
  * - switch-host:
- *   Order to display the toolbox in another host (side, bottom or window)
- * - switch-to-previous-host:
- *   Order to display the toolbox in the previously used host
+ *   Order to display the toolbox in another host (side, bottom, window, or the
+ *   previously used one)
  * - toggle-minimize-mode:
  *   When using the bottom host, the toolbox can be miximized to only display
  *   the tool titles
  * - maximize-host:
  *   When using the bottom host in minimized mode, revert back to regular mode
  *   in order to see tool titles and the tools
  * - raise-host:
  *   Focus the tools
@@ -110,19 +109,16 @@ ToolboxHostManager.prototype = {
     // origin via event.source as it is null. So use a custom id.
     if (event.data.frameId != this.frameId) {
       return;
     }
     switch (event.data.name) {
       case "switch-host":
         this.switchHost(event.data.hostType);
         break;
-      case "switch-to-previous-host":
-        this.switchToPreviousHost();
-        break;
       case "maximize-host":
         this.host.maximize();
         break;
       case "raise-host":
         this.host.raise();
         break;
       case "toggle-minimize-mode":
         this.host.toggleMinimizeMode(event.data.toolbarHeight);
@@ -177,48 +173,38 @@ ToolboxHostManager.prototype = {
   },
 
   onHostMaximized() {
     this.postMessage({
       name: "host-maximized"
     });
   },
 
-  /**
-   * Switch to the last used host for the toolbox UI.
-   * This is determined by the devtools.toolbox.previousHost pref.
-   */
-  switchToPreviousHost() {
-    let hostType = Services.prefs.getCharPref(PREVIOUS_HOST);
+  switchHost: Task.async(function* (hostType) {
+    if (hostType == "previous") {
+      // Switch to the last used host for the toolbox UI.
+      // This is determined by the devtools.toolbox.previousHost pref.
+      hostType = Services.prefs.getCharPref(PREVIOUS_HOST);
 
-    // Handle the case where the previous host happens to match the current
-    // host. If so, switch to bottom if it's not already used, and side if not.
-    if (hostType === this.hostType) {
-      if (hostType === Toolbox.HostType.BOTTOM) {
-        hostType = Toolbox.HostType.SIDE;
-      } else {
-        hostType = Toolbox.HostType.BOTTOM;
+      // Handle the case where the previous host happens to match the current
+      // host. If so, switch to bottom if it's not already used, and side if not.
+      if (hostType === this.hostType) {
+        if (hostType === Toolbox.HostType.BOTTOM) {
+          hostType = Toolbox.HostType.SIDE;
+        } else {
+          hostType = Toolbox.HostType.BOTTOM;
+        }
       }
     }
-
-    return this.switchHost(hostType);
-  },
-
-  switchHost: Task.async(function* (hostType) {
     let iframe = this.host.frame;
     let newHost = this.createHost(hostType);
     let newIframe = yield newHost.create();
     // change toolbox document's parent to the new host
     newIframe.swapFrameLoaders(iframe);
 
-    // See bug 1022726, most probably because of swapFrameLoaders we need to
-    // first focus the window here, and then once again further from
-    // toolbox.js to make sure focus actually happens.
-    iframe.contentWindow.focus();
-
     this.destroyHost();
 
     if (this.hostType != Toolbox.HostType.CUSTOM) {
       Services.prefs.setCharPref(PREVIOUS_HOST, this.hostType);
     }
 
     this.host = newHost;
     this.hostType = hostType;
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -1847,40 +1847,36 @@ Toolbox.prototype = {
       button.setAttribute("checked", "true");
     }
   },
 
   /**
    * Switch to the last used host for the toolbox UI.
    */
   switchToPreviousHost: function () {
-    this.postMessage({
-      name: "switch-to-previous-host"
-    });
-    return this.once("host-changed");
+    return this.switchHost("previous");
   },
 
   /**
    * Switch to a new host for the toolbox UI. E.g. bottom, sidebar, window,
    * and focus the window when done.
    *
    * @param {string} hostType
    *        The host type of the new host object
    */
   switchHost: function (hostType) {
     if (hostType == this.hostType || !this._target.isLocalTab) {
       return null;
     }
 
     this.emit("host-will-change", hostType);
 
-    // If we call swapFrameLoaders() when a tool if focused it leaves the
-    // browser in a state where it thinks that the tool is focused but in
-    // reality the content area is focused. Blurring the tool before calling
-    // swapFrameLoaders() works around this issue.
+    // ToolboxHostManager is going to call swapFrameLoaders which mess up with
+    // focus. We have to blur before calling it in order to be able to restore
+    // the focus after, in _onSwitchedHost.
     this.focusTool(this.currentToolId, false);
 
     // Host code on the chrome side will send back a message once the host
     // switched
     this.postMessage({
       name: "switch-host",
       hostType
     });
@@ -1889,17 +1885,18 @@ Toolbox.prototype = {
   },
 
   _onSwitchedHost: function ({ hostType }) {
     this._hostType = hostType;
 
     this._buildDockButtons();
     this._addKeysToWindow();
 
-    // Focus the tool to make sure keyboard shortcuts work straight away.
+    // We blurred the tools at start of switchHost, but also when clicking on
+    // host switching button. We now have to restore the focus.
     this.focusTool(this.currentToolId, true);
 
     this.emit("host-changed");
     this._telemetry.log(HOST_HISTOGRAM, this._getTelemetryHostId());
   },
 
   /**
    * Return if the tool is available as a tab (i.e. if it's checked