Bug 1444301 - Move Options button into meatball menu; r=jryans
authorBrian Birtles <birtles@gmail.com>
Thu, 05 Apr 2018 10:13:21 +0900
changeset 778791 a6afc78b217ee54d3a9de7f58d2710d104e2ad54
parent 778790 0f07755ee54f665c18a0f16449481edefa3e200e
child 778792 220140d1c28ea1f09f758cb9f5da23f2cb845dc7
push id105582
push userkgupta@mozilla.com
push dateFri, 06 Apr 2018 21:31:13 +0000
reviewersjryans
bugs1444301
milestone61.0a1
Bug 1444301 - Move Options button into meatball menu; r=jryans MozReview-Commit-ID: HnTbtdI5gS6
devtools/client/debugger/test/mochitest/browser_dbg_worker-window.js
devtools/client/framework/components/toolbox-controller.js
devtools/client/framework/components/toolbox-toolbar.js
devtools/client/framework/test/browser_toolbox_tabsswitch_shortcuts.js
devtools/client/framework/toolbox.js
devtools/client/locales/en-US/toolbox.properties
devtools/client/themes/toolbox.css
--- a/devtools/client/debugger/test/mochitest/browser_dbg_worker-window.js
+++ b/devtools/client/debugger/test/mochitest/browser_dbg_worker-window.js
@@ -46,17 +46,17 @@ add_task(function* () {
     });
   });
   ok(toolbox.win.parent.document.title.includes(WORKER_URL),
      "worker URL in host title");
 
   let toolTabs = toolbox.doc.querySelectorAll(".devtools-tab");
   let activeTools = [...toolTabs].map(tab=>tab.getAttribute("data-id"));
 
-  is(activeTools.join(","), "webconsole,jsdebugger,scratchpad,options",
+  is(activeTools.join(","), "webconsole,jsdebugger,scratchpad",
     "Correct set of tools supported by worker");
 
   terminateWorkerInTab(tab, WORKER_URL);
   yield waitForWorkerClose(workerClient);
   yield close(client);
 
   yield toolbox.destroy();
 });
--- a/devtools/client/framework/components/toolbox-controller.js
+++ b/devtools/client/framework/components/toolbox-controller.js
@@ -35,17 +35,16 @@ class ToolboxController extends Componen
       }
     };
 
     this.setFocusedButton = this.setFocusedButton.bind(this);
     this.setToolboxButtons = this.setToolboxButtons.bind(this);
     this.setCurrentToolId = this.setCurrentToolId.bind(this);
     this.highlightTool = this.highlightTool.bind(this);
     this.unhighlightTool = this.unhighlightTool.bind(this);
-    this.setOptionsPanel = this.setOptionsPanel.bind(this);
     this.setHostTypes = this.setHostTypes.bind(this);
     this.setDockOptionsEnabled = this.setDockOptionsEnabled.bind(this);
     this.setCanCloseToolbox = this.setCanCloseToolbox.bind(this);
     this.setCanRender = this.setCanRender.bind(this);
     this.setPanelDefinitions = this.setPanelDefinitions.bind(this);
     this.updateButtonIds = this.updateButtonIds.bind(this);
     this.updateFocusedButton = this.updateFocusedButton.bind(this);
   }
@@ -63,27 +62,25 @@ class ToolboxController extends Componen
   /**
    * The button and tab ids must be known in order to be able to focus left and right
    * using the arrow keys.
    */
   updateButtonIds() {
     const {
       toolboxButtons,
       panelDefinitions,
-      optionsPanel,
       canCloseToolbox,
     } = this.state;
 
     // This is a little gnarly, but go through all of the state and extract the IDs.
     this.setState({
       buttonIds: [
         ...toolboxButtons.filter(btn => btn.isInStartContainer).map(({id}) => id),
         ...panelDefinitions.map(({id}) => id),
         ...toolboxButtons.filter(btn => !btn.isInStartContainer).map(({id}) => id),
-        optionsPanel ? optionsPanel.id : null,
         canCloseToolbox ? "toolbox-close" : null
       ].filter(id => id)
     });
 
     this.updateFocusedButton();
   }
 
   updateFocusedButton() {
@@ -109,20 +106,16 @@ class ToolboxController extends Componen
       this.setFocusedButton(currentToolId);
     });
   }
 
   setCanRender() {
     this.setState({ canRender: true }, this.updateButtonIds);
   }
 
-  setOptionsPanel(optionsPanel) {
-    this.setState({ optionsPanel }, this.updateButtonIds);
-  }
-
   highlightTool(highlightedTool) {
     let { highlightedTools } = this.state;
     highlightedTools.add(highlightedTool);
     this.setState({ highlightedTools });
   }
 
   unhighlightTool(id) {
     let { highlightedTools } = this.state;
--- a/devtools/client/framework/components/toolbox-toolbar.js
+++ b/devtools/client/framework/components/toolbox-toolbar.js
@@ -24,22 +24,21 @@ class ToolboxToolbar extends Component {
     return {
       // The currently focused item (for arrow keyboard navigation)
       // This ID determines the tabindex being 0 or -1.
       focusedButton: PropTypes.string,
       // List of command button definitions.
       toolboxButtons: PropTypes.array,
       // The id of the currently selected tool, e.g. "inspector"
       currentToolId: PropTypes.string,
-      // An optionally highlighted tools, e.g. "inspector".
+      // An optionally highlighted tools, e.g. "inspector" (used by ToolboxTabs
+      // component).
       highlightedTools: PropTypes.instanceOf(Set),
       // List of tool panel definitions (used by ToolboxTabs component).
       panelDefinitions: PropTypes.array,
-      // The options panel definition.
-      optionsPanel: PropTypes.object,
       // List of possible docking options.
       hostTypes: PropTypes.arrayOf(PropTypes.shape({
         position: PropTypes.string.isRequired,
         switchHost: PropTypes.func.isRequired,
       })),
       // Should the docking options be enabled? They are disabled in some
       // contexts such as WebIDE.
       areDockButtonsEnabled: PropTypes.bool,
@@ -70,17 +69,16 @@ class ToolboxToolbar extends Component {
     const containerProps = {className: "devtools-tabbar"};
     return this.props.canRender
       ? (
         div(
           containerProps,
           renderToolboxButtonsStart(this.props),
           ToolboxTabs(this.props),
           renderToolboxButtonsEnd(this.props),
-          renderOptions(this.props),
           renderSeparator(),
           renderToolboxControls(this.props)
         )
       )
       : div(containerProps);
   }
 }
 
@@ -153,46 +151,16 @@ function renderToolboxButtons({focusedBu
         }
       });
     }),
     isStart ? div({className: "devtools-separator"}) : null
   );
 }
 
 /**
- * The options button is a ToolboxTab just like in the ToolboxTabs component.
- * However it is separate from the normal tabs, so deal with it separately here.
- * The following props are expected.
- *
- * @param {string} focusedButton
- *        The id of the focused button.
- * @param {string} currentToolId
- *        The currently selected tool's id; e.g.  "inspector".
- * @param {Object} highlightedTools
- *        Optionally highlighted tools, e.g. "inspector".
- * @param {Object} optionsPanel
- *        A single panel definition for the options panel.
- * @param {Function} selectTool
- *        Function to select a tool in the toolbox.
- * @param {Function} focusButton
- *        Keep a record of the currently focused button.
- */
-function renderOptions({focusedButton, currentToolId, highlightedTools,
-                        optionsPanel, selectTool, focusButton}) {
-  return div({id: "toolbox-option-container"}, ToolboxTab({
-    panelDefinition: optionsPanel,
-    currentToolId,
-    selectTool,
-    highlightedTools,
-    focusedButton,
-    focusButton,
-  }));
-}
-
-/**
  * Render a separator.
  */
 function renderSeparator() {
   return div({className: "devtools-separator"});
 }
 
 /**
  * Render the toolbox control buttons. The following props are expected:
@@ -206,16 +174,18 @@ function renderSeparator() {
  * @param {Function} hostTypes[].switchHost
  *        Function to switch the host.
  * @param {boolean} areDockOptionsEnabled
  *        They are not enabled in certain situations like when they are in the
  *        WebIDE.
  * @param {boolean} canCloseToolbox
  *        Do we need to add UI for closing the toolbox? We don't when the
  *        toolbox is undocked, for example.
+ * @param {Function} selectTool
+ *        Function to select a tool based on its id.
  * @param {Function} closeToolbox
  *        Completely close the toolbox.
  * @param {Function} focusButton
  *        Keep a record of the currently focused button.
  * @param {Object} L10N
  *        Localization interface.
  */
 function renderToolboxControls(props) {
@@ -277,37 +247,49 @@ function renderToolboxControls(props) {
  * @param {Object[]} props.hostTypes
  *        Array of host type objects.
  * @param {string} props.hostTypes[].position
  *        Position name.
  * @param {Function} props.hostTypes[].switchHost
  *        Function to switch the host.
  *        This array will be empty if we shouldn't shouldn't show any dock
  *        options.
+ * @param {Function} props.selectTool
+ *        Function to select a tool based on its id.
  * @param {Object} props.L10N
  *        Localization interface.
  * @param {Object} props.toolbox
  *        The devtools toolbox. Used by the Menu component to determine which
  *        document to use.
  */
-function showMeatballMenu(menuButton, {hostTypes, L10N, toolbox}) {
+function showMeatballMenu(menuButton, {hostTypes, selectTool, L10N, toolbox}) {
   const menu = new Menu({ id: "toolbox-meatball-menu" });
 
+  // Dock options
   for (const hostType of hostTypes) {
     menu.append(new MenuItem({
       id: `toolbox-meatball-menu-dock-${hostType.position}`,
       label: L10N.getStr(
         `toolbox.meatballMenu.dock.${hostType.position}.label`
       ),
       click: () => hostType.switchHost(),
     }));
   }
 
-  // (Yes, it's true we might end up with an empty menu here. Don't worry,
-  // by the end of this patch series that won't be the case.)
+  if (menu.items.length) {
+    menu.append(new MenuItem({ type: "separator" }));
+  }
+
+  // Settings
+  menu.append(new MenuItem({
+    id: "toolbox-meatball-menu-settings",
+    label: L10N.getStr("toolbox.meatballMenu.settings.label"),
+    // TODO: Show "F1" acceltext once MenuItem supports it.
+    click: () => selectTool("options"),
+  }));
 
   const rect = menuButton.getBoundingClientRect();
   const screenX = menuButton.ownerDocument.defaultView.mozInnerScreenX;
   const screenY = menuButton.ownerDocument.defaultView.mozInnerScreenY;
 
   // Display the popup below the button.
   menu.popup(rect.left + screenX, rect.bottom + screenY, toolbox);
 }
--- a/devtools/client/framework/test/browser_toolbox_tabsswitch_shortcuts.js
+++ b/devtools/client/framework/test/browser_toolbox_tabsswitch_shortcuts.js
@@ -13,17 +13,21 @@ const {LocalizationHelper} = require("de
 const L10N = new LocalizationHelper("devtools/client/locales/toolbox.properties");
 
 add_task(async function() {
   let tab = await addTab("about:blank");
   let target = TargetFactory.forTab(tab);
   await target.makeRemote();
 
   let toolIDs = gDevTools.getToolDefinitionArray()
-                         .filter(def => def.isTargetSupported(target))
+                         .filter(
+                           def =>
+                             def.isTargetSupported(target) &&
+                             def.id !== "options"
+                         )
                          .map(def => def.id);
 
   let toolbox = await gDevTools.showToolbox(target, toolIDs[0], Toolbox.HostType.BOTTOM);
   let nextShortcut = L10N.getStr("toolbox.nextTool.key");
   let prevShortcut = L10N.getStr("toolbox.previousTool.key");
 
   // Iterate over all tools, starting from options to netmonitor, in normal
   // order.
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -1099,20 +1099,16 @@ Toolbox.prototype = {
     // Get the initial list of tab definitions. This list can be amended at a later time
     // by tools registering themselves.
     const definitions = gDevTools.getToolDefinitionArray();
     definitions.forEach(definition => this._buildPanelForTool(definition));
 
     // Get the definitions that will only affect the main tab area.
     this.panelDefinitions = definitions.filter(definition =>
       definition.isTargetSupported(this._target) && definition.id !== "options");
-
-    this.optionsDefinition = definitions.find(({id}) => id === "options");
-    // The options tool is treated slightly differently, and is in a different area.
-    this.component.setOptionsPanel(definitions.find(({id}) => id === "options"));
   },
 
   _mountReactComponent: function() {
     // Ensure the toolbar doesn't try to render until the tool is ready.
     const element = this.React.createElement(this.ToolboxController, {
       L10N,
       currentToolId: this.currentToolId,
       selectTool: this.selectTool,
@@ -1960,35 +1956,31 @@ Toolbox.prototype = {
   },
 
   /**
    * Loads the tool next to the currently selected tool.
    */
   selectNextTool: function() {
     let definitions = this.component.panelDefinitions;
     const index = definitions.findIndex(({id}) => id === this.currentToolId);
-    let definition = definitions[index + 1];
-    if (!definition) {
-      definition = index === -1 ? definitions[0] : this.optionsDefinition;
-    }
+    let definition = index === -1 || index >= definitions.length - 1
+                     ? definitions[0]
+                     : definitions[index + 1];
     return this.selectTool(definition.id);
   },
 
   /**
    * Loads the tool just left to the currently selected tool.
    */
   selectPreviousTool: function() {
     let definitions = this.component.panelDefinitions;
     const index = definitions.findIndex(({id}) => id === this.currentToolId);
-    let definition = definitions[index - 1];
-    if (!definition) {
-      definition = index === -1
-        ? definitions[definitions.length - 1]
-        : this.optionsDefinition;
-    }
+    let definition = index === -1 || index < 1
+                     ? definitions[definitions.length - 1]
+                     : definitions[index - 1];
     return this.selectTool(definition.id);
   },
 
   /**
    * Highlights the tool's tab if it is not the currently selected tool.
    *
    * @param {string} id
    *        The id of the tool to highlight
--- a/devtools/client/locales/en-US/toolbox.properties
+++ b/devtools/client/locales/en-US/toolbox.properties
@@ -154,16 +154,22 @@ toolbox.meatballMenu.button.tooltip=Cust
 
 # LOCALIZATION NOTE (toolbox.meatballMenu.dock.*.label): These labels are shown
 # in the "..." menu in the toolbox and represent the different arrangements for
 # docking (or undocking) the developer tools toolbox.
 toolbox.meatballMenu.dock.bottom.label=Dock to bottom
 toolbox.meatballMenu.dock.side.label=Dock to side
 toolbox.meatballMenu.dock.window.label=Undock
 
+# LOCALIZATION NOTE (toolbox.meatballMenu.settings.label): This is the label for
+# the item in the "..." menu in the toolbox that brings up the Settings
+# (Options) panel.
+# The keyboard shortcut will be shown to the side of the label.
+toolbox.meatballMenu.settings.label=Settings
+
 # LOCALIZATION NOTE (toolbox.closebutton.tooltip): This is the tooltip for
 # the close button the developer tools toolbox.
 toolbox.closebutton.tooltip=Close Developer Tools
 
 # LOCALIZATION NOTE (toolbox.allToolsButton.tooltip): This is the tooltip for the
 # "all tools" button displayed when some tools are hidden by overflow of the toolbar.
 toolbox.allToolsButton.tooltip=Select another tool
 
--- a/devtools/client/themes/toolbox.css
+++ b/devtools/client/themes/toolbox.css
@@ -54,17 +54,16 @@
   flex: 1;
   overflow: hidden;
 }
 
 /* Set flex attribute to Toolbox buttons and Picker container so,
    they don't overlap with the tab bar */
 #toolbox-buttons-start,
 #toolbox-buttons-end,
-#toolbox-option-container,
 #toolbox-controls {
   display: flex;
   align-items: stretch;
 }
 
 /* Toolbox tabs */
 
 .devtools-tab {
@@ -155,27 +154,16 @@
 .devtools-tab.selected > img {
   fill: var(--theme-toolbar-selected-color);
 }
 
 .devtools-tab.highlighted > img {
   fill: var(--theme-toolbar-highlighted-color);
 }
 
-/* The options tab is special - it doesn't have the same parent
-   as the other tabs (toolbox-option-container vs toolbox-tabs) */
-#toolbox-option-container .devtools-tab {
-  border-color: transparent;
-  border-width: 0;
-  padding-inline-start: 1px;
-}
-#toolbox-option-container img {
-  margin: 0 3px;
-}
-
 /* Toolbox controls */
 
 
 #toolbox-close::before {
   background-image: var(--close-button-image);
 }
 
 #toolbox-meatball-menu-button::before {