Bug 1451592 - Hide the iframe button when there are no child iframes; r?jryans draft
authorBrian Birtles <birtles@gmail.com>
Mon, 16 Apr 2018 19:49:38 +0200
changeset 783180 244294e7aae5b3022eabda9bee76380cfac34769
parent 782432 7ff499dfcd51cf4a95ebf0db506b415bf7bb27c3
child 783181 84491a9db65d0afc151b299839e88e562b8c8c86
push id106636
push userbbirtles@mozilla.com
push dateMon, 16 Apr 2018 19:27:19 +0000
reviewersjryans
bugs1451592
milestone61.0a1
Bug 1451592 - Hide the iframe button when there are no child iframes; r?jryans MozReview-Commit-ID: 7Q3PojIzmy4
devtools/client/framework/components/toolbox-toolbar.js
devtools/client/framework/test/browser_toolbox_options_disable_buttons.js
devtools/client/framework/toolbox-options.js
devtools/client/framework/toolbox.js
--- a/devtools/client/framework/components/toolbox-toolbar.js
+++ b/devtools/client/framework/components/toolbox-toolbar.js
@@ -172,26 +172,26 @@ function renderToolboxButtons({focusedBu
           onKeyDown(event);
         }
       });
     });
 
   // Add the appropriate separator, if needed.
   let children = renderedButtons;
   if (renderedButtons.length) {
+    if (isStart) {
+      children.push(renderSeparator());
     // For the end group we add a separator *before* the RDM button if it
-    // exists.
-    if (rdmIndex !== -1) {
+    // exists, but only if it is not the only button.
+    } else if (rdmIndex !== -1 && visibleButtons.length > 1) {
       children.splice(
         children.length - 1,
         0,
         renderSeparator()
       );
-    } else {
-      children.push(renderSeparator());
     }
   }
 
   return div({id: `toolbox-buttons-${isStart ? "start" : "end"}`}, ...children);
 }
 
 /**
  * Render a separator.
--- a/devtools/client/framework/test/browser_toolbox_options_disable_buttons.js
+++ b/devtools/client/framework/test/browser_toolbox_options_disable_buttons.js
@@ -1,17 +1,22 @@
 /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
-const TEST_URL = "data:text/html;charset=utf8,test for dynamically " +
-                 "registering and unregistering tools";
+let TEST_URL = "data:text/html;charset=utf8,test for dynamically " +
+               "registering and unregistering tools";
+
+// The frames button is only shown if the page has at least one iframe so we
+// need to add one to the test page.
+TEST_URL += "<iframe src=\"data:text/plain,iframe\"></iframe>";
+
 var doc = null, toolbox = null, panelWin = null, modifiedPrefs = [];
 
 function test() {
   addTab(TEST_URL).then(tab => {
     let target = TargetFactory.forTab(tab);
     gDevTools.showToolbox(target)
       .then(testSelectTool)
       .then(testToggleToolboxButtons)
--- a/devtools/client/framework/toolbox-options.js
+++ b/devtools/client/framework/toolbox-options.js
@@ -179,17 +179,17 @@ OptionsPanel.prototype = {
 
     let createCommandCheckbox = button => {
       let checkboxLabel = this.panelDoc.createElement("label");
       let checkboxSpanLabel = this.panelDoc.createElement("span");
       checkboxSpanLabel.textContent = button.description;
       let checkboxInput = this.panelDoc.createElement("input");
       checkboxInput.setAttribute("type", "checkbox");
       checkboxInput.setAttribute("id", button.id);
-      if (button.isVisible) {
+      if (Services.prefs.getBoolPref(button.visibilityswitch, true)) {
         checkboxInput.setAttribute("checked", true);
       }
       checkboxInput.addEventListener("change",
         onCheckboxClick.bind(this, checkboxInput));
 
       checkboxLabel.appendChild(checkboxInput);
       checkboxLabel.appendChild(checkboxSpanLabel);
       return checkboxLabel;
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -761,32 +761,35 @@ Toolbox.prototype = {
    *                       argument, to be called whenever the checked state changes.
    * @property {Function} teardown - Function run on toolbox close to let a chance to
    *                      unregister listeners set when `setup` was called and avoid
    *                      memory leaks. The same arguments than `setup` function are
    *                      passed to `teardown`.
    * @property {Function} isTargetSupported - Function to automatically enable/disable
    *                      the button based on the target. If the target don't support
    *                      the button feature, this method should return false.
+   * @property {Function} isCurrentlyVisible - Function to automatically
+   *                      hide/show the button based on current state.
    * @property {Function} isChecked - Optional function called to known if the button
    *                      is toggled or not. The function should return true when
    *                      the button should be displayed as toggled on.
    */
   _createButtonState: function(options) {
     let isCheckedValue = false;
     const {
       id,
       className,
       description,
       disabled,
       onClick,
       isInStartContainer,
       setup,
       teardown,
       isTargetSupported,
+      isCurrentlyVisible,
       isChecked,
       onKeyDown
     } = options;
     const toolbox = this;
     const button = {
       id,
       className,
       description,
@@ -797,16 +800,17 @@ Toolbox.prototype = {
         }
       },
       onKeyDown(event) {
         if (typeof onKeyDown == "function") {
           onKeyDown(event, toolbox);
         }
       },
       isTargetSupported,
+      isCurrentlyVisible,
       get isChecked() {
         if (typeof isChecked == "function") {
           return isChecked(toolbox);
         }
         return isCheckedValue;
       },
       set isChecked(value) {
         // Note that if options.isChecked is given, this is ignored
@@ -1235,16 +1239,19 @@ Toolbox.prototype = {
   _buildFrameButton() {
     this.frameButton = this._createButtonState({
       id: "command-button-frames",
       description: L10N.getStr("toolbox.frames.tooltip"),
       onClick: this.showFramesMenu,
       isTargetSupported: target => {
         return target.activeTab && target.activeTab.traits.frames;
       },
+      isCurrentlyVisible: () => {
+        return this.frameMap.size > 1;
+      },
       onKeyDown: this.handleKeyDownOnFramesButton
     });
 
     return this.frameButton;
   },
 
   /**
    * Toggle the picker, but also decide whether or not the highlighter should
@@ -1376,26 +1383,33 @@ Toolbox.prototype = {
   },
 
   /**
    * Ensure the visibility of each toolbox button matches the preference value.
    */
   _commandIsVisible: function(button) {
     const {
       isTargetSupported,
+      isCurrentlyVisible,
       visibilityswitch
     } = button;
 
-    let visible = Services.prefs.getBoolPref(visibilityswitch, true);
-
-    if (isTargetSupported) {
-      return visible && isTargetSupported(this.target);
+    if (!Services.prefs.getBoolPref(visibilityswitch, true)) {
+      return false;
+    }
+
+    if (isTargetSupported && !isTargetSupported(this.target)) {
+      return false;
     }
 
-    return visible;
+    if (isCurrentlyVisible && !isCurrentlyVisible()) {
+      return false;
+    }
+
+    return true;
   },
 
   /**
    * Build a panel for a tool definition.
    *
    * @param {string} toolDefinition
    *        Tool definition of the tool to build a tab for.
    */
@@ -2247,20 +2261,16 @@ Toolbox.prototype = {
    *                 id {Number}: frame ID
    *                 url {String}: frame URL
    *                 title {String}: frame title
    *                 destroy {Boolean}: Set to true if destroyed
    *                 parentID {Number}: ID of the parent frame (not set
    *                                    for top level window)
    */
   _updateFrames: function(data) {
-    if (!Services.prefs.getBoolPref("devtools.command-button-frames.enabled")) {
-      return;
-    }
-
     // We may receive this event before the toolbox is ready.
     if (!this.isReady) {
       return;
     }
 
     // Store (synchronize) data about all existing frames on the backend
     if (data.destroyAll) {
       this.frameMap.clear();
@@ -2297,16 +2307,20 @@ Toolbox.prototype = {
     let topFrameSelected = frame ? !frame.parentID : false;
     this._framesButtonChecked = false;
 
     // If non-top level frame is selected the toolbar button is
     // marked as 'checked' indicating that a child frame is active.
     if (!topFrameSelected && this.selectedFrameId) {
       this._framesButtonChecked = false;
     }
+
+    // We may need to hide/show the frames button now.
+    this.frameButton.isVisible = this._commandIsVisible(this.frameButton);
+    this.component.setToolboxButtons(this.toolbarButtons);
   },
 
   /**
    * Returns a 0-based selected frame depth.
    *
    * For example, if the root frame is selected, the returned value is 0.  For a sub-frame
    * of the root document, the returned value is 1, and so on.
    */