Bug 1467572 - Part 12: Use native context menu instead of select element in the Device selector. r=Honza
authorGabriel Luong <gabriel.luong@gmail.com>
Wed, 15 Aug 2018 17:27:46 -0400
changeset 431859 06c72f105c508ef96f368ed2f180f94b192501c0
parent 431858 aed506e2100a0dd8e5325912edca9f74be9f3edf
child 431860 2eb675aaabacaed8fcb14a6defb40f59212e3dc6
push id34451
push userebalazs@mozilla.com
push dateThu, 16 Aug 2018 09:25:15 +0000
treeherdermozilla-central@161817e6d127 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersHonza
bugs1467572
milestone63.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 1467572 - Part 12: Use native context menu instead of select element in the Device selector. r=Honza - Removes all the unused styling for the <select> and <option> elements and the toolbar-dropdown class.
devtools/client/locales/en-US/responsive.properties
devtools/client/responsive.html/components/DeviceSelector.js
devtools/client/responsive.html/index.css
--- a/devtools/client/locales/en-US/responsive.properties
+++ b/devtools/client/locales/en-US/responsive.properties
@@ -6,68 +6,60 @@
 # available from the Web Developer sub-menu -> 'Responsive Design Mode'.
 #
 # The correct localization of this file might be to keep it in
 # English, or another language commonly spoken among web developers.
 # You want to make that choice consistent across the developer tools.
 # A good criteria is the language in which you'd find the best
 # documentation on web development on the web.
 
-# LOCALIZATION NOTE (responsive.editDeviceList): option displayed in the device
-# selector
-responsive.editDeviceList=Edit list…
+# LOCALIZATION NOTE (responsive.editDeviceList2): Context menu item displayed in the
+# device selector.
+responsive.editDeviceList2=Edit List…
 
-# LOCALIZATION NOTE (responsive.exit): tooltip text of the exit button.
+# LOCALIZATION NOTE (responsive.exit): Tooltip text of the exit button.
 responsive.exit=Close Responsive Design Mode
 
-# LOCALIZATION NOTE (responsive.rotate): tooltip text of the rotate button.
+# LOCALIZATION NOTE (responsive.rotate): Tooltip text of the rotate button.
 responsive.rotate=Rotate viewport
 
-# LOCALIZATION NOTE (responsive.deviceListLoading): placeholder text for
-# device selector when it's still fetching devices
-responsive.deviceListLoading=Loading…
-
-# LOCALIZATION NOTE (responsive.deviceListError): placeholder text for
-# device selector when an error occurred
-responsive.deviceListError=No list available
-
-# LOCALIZATION NOTE (responsive.done): button text in the device list modal
+# LOCALIZATION NOTE (responsive.done): Button text in the device list modal
 responsive.done=Done
 
-# LOCALIZATION NOTE (responsive.noDeviceSelected): placeholder text for the
-# device selector
-responsive.noDeviceSelected=no device selected
+# LOCALIZATION NOTE (responsive.responsiveMode): Placeholder text for the
+# device selector.
+responsive.responsiveMode=Responsive
 
-# LOCALIZATION NOTE (responsive.enableTouch): tooltip text for the touch
-# simulation button when it's disabled
+# LOCALIZATION NOTE (responsive.enableTouch): Tooltip text for the touch
+# simulation button when it's disabled.
 responsive.enableTouch=Enable touch simulation
 
-# LOCALIZATION NOTE (responsive.disableTouch): tooltip text for the touch
-# simulation button when it's enabled
+# LOCALIZATION NOTE (responsive.disableTouch): Tooltip text for the touch
+# simulation button when it's enabled.
 responsive.disableTouch=Disable touch simulation
 
-# LOCALIZATION NOTE  (responsive.screenshot): tooltip of the screenshot button.
+# LOCALIZATION NOTE  (responsive.screenshot): Tooltip of the screenshot button.
 responsive.screenshot=Take a screenshot of the viewport
 
 # LOCALIZATION NOTE (responsive.screenshotGeneratedFilename): The auto generated
 # filename.
 # The first argument (%1$S) is the date string in yyyy-mm-dd format and the
 # second argument (%2$S) is the time string in HH.MM.SS format.
 responsive.screenshotGeneratedFilename=Screen Shot %1$S at %2$S
 
 # LOCALIZATION NOTE (responsive.remoteOnly): Message displayed in the tab's
 # notification box if a user tries to open Responsive Design Mode in a
 # non-remote tab.
 responsive.remoteOnly=Responsive Design Mode is only available for remote browser tabs, such as those used for web content in multi-process Firefox.
 
-# LOCALIZATION NOTE (responsive.changeDevicePixelRatio): tooltip for the
+# LOCALIZATION NOTE (responsive.changeDevicePixelRatio): Tooltip for the
 # device pixel ratio dropdown when is enabled.
 responsive.changeDevicePixelRatio=Change device pixel ratio of the viewport
 
-# LOCALIZATION NOTE (responsive.devicePixelRatio.auto): tooltip for the device pixel ratio
+# LOCALIZATION NOTE (responsive.devicePixelRatio.auto): Tooltip for the device pixel ratio
 # dropdown when it is disabled because a device is selected.
 # The argument (%1$S) is the selected device (e.g. iPhone 6) that set
 # automatically the device pixel ratio value.
 responsive.devicePixelRatio.auto=Device pixel ratio automatically set by %1$S
 
 # LOCALIZATION NOTE (responsive.customDeviceName): Default value in a form to
 # add a custom device based on an arbitrary size (no association to an existing
 # device).
--- a/devtools/client/responsive.html/components/DeviceSelector.js
+++ b/devtools/client/responsive.html/components/DeviceSelector.js
@@ -6,128 +6,98 @@
 
 const { PureComponent } = require("devtools/client/shared/vendor/react");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 
 const { getStr } = require("../utils/l10n");
 const Types = require("../types");
 
-const OPEN_DEVICE_MODAL_VALUE = "OPEN_DEVICE_MODAL";
+loader.lazyRequireGetter(this, "showMenu", "devtools/client/shared/components/menu/utils", true);
 
 class DeviceSelector extends PureComponent {
   static get propTypes() {
     return {
       devices: PropTypes.shape(Types.devices).isRequired,
       selectedDevice: PropTypes.string.isRequired,
       viewportId: PropTypes.number.isRequired,
       onChangeDevice: PropTypes.func.isRequired,
       onResizeViewport: PropTypes.func.isRequired,
       onUpdateDeviceModal: PropTypes.func.isRequired,
     };
   }
 
   constructor(props) {
     super(props);
-    this.onSelectChange = this.onSelectChange.bind(this);
+    this.onShowDeviceMenu = this.onShowDeviceMenu.bind(this);
   }
 
-  onSelectChange({ target }) {
+  onShowDeviceMenu(event) {
     const {
       devices,
+      selectedDevice,
       viewportId,
       onChangeDevice,
       onResizeViewport,
       onUpdateDeviceModal,
     } = this.props;
 
-    if (target.value === OPEN_DEVICE_MODAL_VALUE) {
-      onUpdateDeviceModal(true, viewportId);
-      return;
-    }
+    const menuItems = [];
+
     for (const type of devices.types) {
       for (const device of devices[type]) {
-        if (device.name === target.value) {
-          onResizeViewport(viewportId, device.width, device.height);
-          onChangeDevice(viewportId, device, type);
-          return;
+        if (device.displayed) {
+          menuItems.push({
+            label: device.name,
+            type: "checkbox",
+            checked: selectedDevice === device.name,
+            click: () => {
+              onResizeViewport(viewportId, device.width, device.height);
+              onChangeDevice(viewportId, device, type);
+            },
+          });
         }
       }
     }
+
+    menuItems.sort(function(a, b) {
+      return a.label.localeCompare(b.label);
+    });
+
+    if (menuItems.length > 0) {
+      menuItems.push("-");
+    }
+
+    menuItems.push({
+      label: getStr("responsive.editDeviceList2"),
+      click: () => onUpdateDeviceModal(true, viewportId),
+    });
+
+    showMenu(menuItems, {
+      button: event.target,
+      useTopLevelWindow: true,
+    });
   }
 
   render() {
     const {
       devices,
       selectedDevice,
     } = this.props;
 
-    const options = [];
-    for (const type of devices.types) {
-      for (const device of devices[type]) {
-        if (device.displayed) {
-          options.push(device);
-        }
-      }
-    }
-
-    options.sort(function(a, b) {
-      return a.name.localeCompare(b.name);
-    });
-
-    let selectClass = "toolbar-dropdown";
-    if (selectedDevice) {
-      selectClass += " selected";
-    }
-
-    const state = devices.listState;
-    let listContent;
-
-    if (state == Types.loadableState.LOADED) {
-      listContent = [
-        dom.option({
-          value: "",
-          title: "",
-          disabled: true,
-          hidden: true,
+    return (
+      dom.button(
+        {
+          id: "device-selector",
+          className: "devtools-button devtools-dropdown-button",
+          disabled: devices.listState !== Types.loadableState.LOADED,
+          title: selectedDevice,
+          onClick: this.onShowDeviceMenu,
         },
-        getStr("responsive.noDeviceSelected")),
-        options.map(device => {
-          return dom.option({
-            key: device.name,
-            value: device.name,
-            title: "",
-          }, device.name);
-        }),
-        dom.option({
-          value: OPEN_DEVICE_MODAL_VALUE,
-          title: "",
-        }, getStr("responsive.editDeviceList"))];
-    } else if (state == Types.loadableState.LOADING
-      || state == Types.loadableState.INITIALIZED) {
-      listContent = [dom.option({
-        value: "",
-        title: "",
-        disabled: true,
-      }, getStr("responsive.deviceListLoading"))];
-    } else if (state == Types.loadableState.ERROR) {
-      listContent = [dom.option({
-        value: "",
-        title: "",
-        disabled: true,
-      }, getStr("responsive.deviceListError"))];
-    }
-
-    return dom.select(
-      {
-        id: "device-selector",
-        className: selectClass,
-        value: selectedDevice,
-        title: selectedDevice,
-        onChange: this.onSelectChange,
-        disabled: (state !== Types.loadableState.LOADED),
-      },
-      ...listContent
+        dom.span({ className: "title" },
+          selectedDevice || getStr("responsive.responsiveMode")
+        )
+      )
     );
   }
 }
 
 module.exports = DeviceSelector;
--- a/devtools/client/responsive.html/index.css
+++ b/devtools/client/responsive.html/index.css
@@ -24,17 +24,16 @@
 }
 
 * {
   box-sizing: border-box;
 }
 
 :root,
 input,
-select,
 button {
   font-size: 12px;
 }
 
 html,
 body,
 #root {
   height: 100%;
@@ -84,80 +83,16 @@ body,
 .toolbar-button.selected {
   color: var(--viewport-active-color);
 }
 
 .toolbar-button:not(:disabled):hover {
   color: var(--viewport-hover-color);
 }
 
-select {
-  -moz-appearance: none;
-  color: var(--viewport-color);
-  border: none;
-  height: 100%;
-  padding: 0 8px;
-  text-align: center;
-  text-overflow: ellipsis;
-}
-
-select.selected {
-  color: var(--viewport-active-color);
-}
-
-select:not(:disabled):hover {
-  color: var(--viewport-hover-color);
-}
-
-/* This is (believed to be?) separate from the identical select.selected rule
-   set so that it overrides select:hover because of file ordering once the
-   select is focused.  It's unclear whether the visual effect that results here
-   is intentional and desired. */
-select:focus {
-  color: var(--viewport-active-color);
-}
-
-select > option {
-  text-align: left;
-  padding: 5px 10px;
-}
-
-select > option,
-select > option:hover {
-  color: var(--viewport-active-color);
-}
-
-select > option.divider {
-  border-top: 1px solid var(--theme-splitter-color);
-  height: 0px;
-  padding: 0;
-  font-size: 0px;
-}
-
-/**
- * Common background for dropdowns like select and toggle menu
- */
-
-.toolbar-dropdown,
-.toolbar-dropdown.devtools-button,
-.toolbar-dropdown.devtools-button:hover:not(:empty):not(:disabled):not(.checked) {
-  background-color: var(--theme-toolbar-background);
-  background-image: var(--select-arrow-image);
-  background-position: 100% 50%;
-  background-repeat: no-repeat;
-  background-size: 7px;
-  -moz-context-properties: fill;
-  fill: currentColor;
-}
-
-.toolbar-dropdown {
-  background-position-x: right 5px;
-  padding-right: 15px;
-}
-
 /**
  * Toolbar
  */
 
 #toolbar {
   background-color: var(--theme-toolbar-background);
   border-bottom: 1px solid var(--theme-splitter-color);
   display: grid;
@@ -207,16 +142,28 @@ select > option.divider {
   background-image: url("chrome://devtools/skin/images/close.svg");
 }
 
 #screenshot-button:disabled {
   filter: var(--theme-icon-checked-filter);
   opacity: 1 !important;
 }
 
+#device-selector {
+  align-self: center;
+  background-position: right 4px center;
+  margin-inline-start: 4px;
+  padding-left: 0;
+  width: 8em;
+}
+
+#device-selector .title {
+  width: 85%;
+}
+
 #device-pixel-ratio-menu {
   width: 6em;
   /* `max-width` is here to keep the UI compact if the device pixel ratio changes to a
      repeating decimal value.  This can happen if you zoom the UI (Cmd + Plus / Minus on
      macOS for example). */
   max-width: 8em;
   background-position: right 4px center;
   padding-left: 0;