Bug 1444301 - Move Options button into meatball menu; r=jryans
authorBrian Birtles <birtles@gmail.com>
Thu, 05 Apr 2018 10:13:21 +0900
changeset 412180 a6afc78b217ee54d3a9de7f58d2710d104e2ad54
parent 412179 0f07755ee54f665c18a0f16449481edefa3e200e
child 412181 220140d1c28ea1f09f758cb9f5da23f2cb845dc7
push id62302
push userbbirtles@mozilla.com
push dateFri, 06 Apr 2018 21:15:54 +0000
treeherderautoland@c5e2ad71fce5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjryans
bugs1444301
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 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 {