Bug 1412359 - Filter to matching device on remove. r=gl draft
authorJ. Ryan Stinnett <jryans@gmail.com>
Mon, 30 Oct 2017 11:00:55 -0500
changeset 692846 b41493fb1afa4d9d5f8ef0189e2eb7aaa56e91eb
parent 688822 b250d69a898c12dedc43fd34f9041cb94f1d00cb
child 738864 326c735126a8f68ae1b57581277c0211c52eefa8
push id87616
push userbmo:jryans@gmail.com
push dateFri, 03 Nov 2017 16:36:51 +0000
reviewersgl
bugs1412359
milestone58.0a1
Bug 1412359 - Filter to matching device on remove. r=gl The local device removal path used by RDM had a bug in its `findIndex` call which caused it to always return `true` for the first device. Effectively this meant that each separate device removal button always removed the first device! This would lead to all sorts of user confusion and UI divergence. Here we clean this up by allowing the caller (RDM in this case) to specify via a callback which device is intended for removal. MozReview-Commit-ID: 22VwEDZAXOa
devtools/client/responsive.html/components/DeviceModal.js
devtools/client/responsive.html/test/browser/browser.ini
devtools/client/responsive.html/test/browser/browser_device_custom.js
devtools/client/responsive.html/test/browser/browser_device_custom_remove.js
devtools/client/responsive.html/test/browser/head.js
devtools/client/shared/devices.js
--- a/devtools/client/responsive.html/components/DeviceModal.js
+++ b/devtools/client/responsive.html/components/DeviceModal.js
@@ -157,17 +157,17 @@ module.exports = createClass({
         }),
         dom.div(
           {
             className: "device-modal-content",
           },
           devices.types.map(type => {
             return dom.div(
               {
-                className: "device-type",
+                className: `device-type device-type-${type}`,
                 key: type,
               },
               dom.header(
                 {
                   className: "device-header",
                 },
                 type
               ),
--- a/devtools/client/responsive.html/test/browser/browser.ini
+++ b/devtools/client/responsive.html/test/browser/browser.ini
@@ -14,16 +14,17 @@ support-files =
   !/devtools/client/framework/test/shared-head.js
   !/devtools/client/framework/test/shared-redux-head.js
   !/devtools/client/inspector/test/shared-head.js
   !/devtools/client/shared/test/test-actor.js
   !/devtools/client/shared/test/test-actor-registry.js
 
 [browser_cmd_click.js]
 [browser_device_change.js]
+[browser_device_custom_remove.js]
 [browser_device_custom.js]
 [browser_device_modal_error.js]
 [browser_device_modal_exit.js]
 [browser_device_modal_submit.js]
 [browser_device_width.js]
 [browser_dpr_change.js]
 [browser_exit_button.js]
 [browser_frame_script_active.js]
--- a/devtools/client/responsive.html/test/browser/browser_device_custom.js
+++ b/devtools/client/responsive.html/test/browser/browser_device_custom.js
@@ -7,29 +7,25 @@ http://creativecommons.org/publicdomain/
 
 const device = {
   name: "Test Device",
   width: 400,
   height: 570,
   pixelRatio: 1.5,
   userAgent: "Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0",
   touch: true,
-  firefoxOS: false,
-  os: "android",
 };
 
 const unicodeDevice = {
   name: "\u00B6\u00C7\u00DA\u00E7\u0126",
   width: 400,
   height: 570,
   pixelRatio: 1.5,
   userAgent: "Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0",
   touch: true,
-  firefoxOS: false,
-  os: "android",
 };
 
 const TEST_URL = "data:text/html;charset=utf-8,";
 const Types = require("devtools/client/responsive.html/types");
 
 addRDMTask(TEST_URL, function* ({ ui }) {
   let { toolWindow } = ui;
   let { store, document } = toolWindow;
@@ -53,21 +49,17 @@ addRDMTask(TEST_URL, function* ({ ui }) 
     width: 320,
     height: 480,
     pixelRatio: window.devicePixelRatio,
     userAgent: navigator.userAgent,
     touch: false,
   });
 
   info("Fill out device adder form and save");
-  setDeviceAdder(ui, device);
-  let adderSave = document.querySelector("#device-adder-save");
-  let saved = waitUntilState(store, state => state.devices.custom.length == 1);
-  Simulate.click(adderSave);
-  yield saved;
+  yield addDeviceInModal(ui, device);
 
   info("Verify device defaults to enabled in modal");
   let deviceCb = [...document.querySelectorAll(".device-input-checkbox")].find(cb => {
     return cb.value == device.name;
   });
   ok(deviceCb, "Custom device checkbox added to modal");
   ok(deviceCb.checked, "Custom device enabled");
   Simulate.click(submitButton);
@@ -137,21 +129,17 @@ addRDMTask(TEST_URL, function* ({ ui }) 
 
   openDeviceModal(ui);
 
   info("Reveal device adder form");
   let adderShow = document.querySelector("#device-adder-show");
   Simulate.click(adderShow);
 
   info("Fill out device adder form by setting details to unicode device and save");
-  setDeviceAdder(ui, unicodeDevice);
-  let adderSave = document.querySelector("#device-adder-save");
-  let saved = waitUntilState(store, state => state.devices.custom.length == 1);
-  Simulate.click(adderSave);
-  yield saved;
+  yield addDeviceInModal(ui, unicodeDevice);
 
   info("Verify unicode device defaults to enabled in modal");
   let deviceCb = [...document.querySelectorAll(".device-input-checkbox")].find(cb => {
     return cb.value == unicodeDevice.name;
   });
   ok(deviceCb, "Custom unicode device checkbox added to modal");
   ok(deviceCb.checked, "Custom unicode device enabled");
   Simulate.click(submitButton);
@@ -193,36 +181,8 @@ function testDeviceAdder(ui, expected) {
   is(nameInput.value, expected.name, "Device name matches");
   is(parseInt(widthInput.value, 10), expected.width, "Width matches");
   is(parseInt(heightInput.value, 10), expected.height, "Height matches");
   is(parseFloat(pixelRatioInput.value), expected.pixelRatio,
      "devicePixelRatio matches");
   is(userAgentInput.value, expected.userAgent, "User agent matches");
   is(touchInput.checked, expected.touch, "Touch matches");
 }
-
-function setDeviceAdder(ui, value) {
-  let { toolWindow } = ui;
-  let { document } = ui.toolWindow;
-  let React = toolWindow.require("devtools/client/shared/vendor/react");
-  let { Simulate } = React.addons.TestUtils;
-
-  let nameInput = document.querySelector("#device-adder-name input");
-  let [ widthInput, heightInput ] = document.querySelectorAll("#device-adder-size input");
-  let pixelRatioInput = document.querySelector("#device-adder-pixel-ratio input");
-  let userAgentInput = document.querySelector("#device-adder-user-agent input");
-  let touchInput = document.querySelector("#device-adder-touch input");
-
-  nameInput.value = value.name;
-  Simulate.change(nameInput);
-  widthInput.value = value.width;
-  Simulate.change(widthInput);
-  Simulate.blur(widthInput);
-  heightInput.value = value.height;
-  Simulate.change(heightInput);
-  Simulate.blur(heightInput);
-  pixelRatioInput.value = value.pixelRatio;
-  Simulate.change(pixelRatioInput);
-  userAgentInput.value = value.userAgent;
-  Simulate.change(userAgentInput);
-  touchInput.checked = value.touch;
-  Simulate.change(touchInput);
-}
new file mode 100644
--- /dev/null
+++ b/devtools/client/responsive.html/test/browser/browser_device_custom_remove.js
@@ -0,0 +1,125 @@
+/* Any copyright is dedicated to the Public Domain.
+http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test adding several devices and removing one to ensure the correct device is removed.
+
+const TEST_URL = "data:text/html;charset=utf-8,";
+const Types = require("devtools/client/responsive.html/types");
+
+const device = {
+  width: 400,
+  height: 570,
+  pixelRatio: 1.5,
+  userAgent: "Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0",
+  touch: true,
+};
+
+const device1 = Object.assign({}, device, {
+  name: "Test Device 1",
+});
+
+const device2 = Object.assign({}, device, {
+  name: "Test Device 2",
+});
+
+addRDMTask(TEST_URL, function* ({ ui }) {
+  let { toolWindow } = ui;
+  let { store, document } = toolWindow;
+  let React = toolWindow.require("devtools/client/shared/vendor/react");
+  let { Simulate } = React.addons.TestUtils;
+
+  info("Verify that remove buttons affect the correct device");
+
+  // Wait until the viewport has been added and the device list has been loaded
+  yield waitUntilState(store, state => state.viewports.length == 1
+    && state.devices.listState == Types.deviceListState.LOADED);
+
+  let deviceSelector = document.querySelector(".viewport-device-selector");
+  let submitButton = document.querySelector("#device-submit-button");
+
+  openDeviceModal(ui);
+
+  info("Reveal device adder form");
+  let adderShow = document.querySelector("#device-adder-show");
+  Simulate.click(adderShow);
+
+  info("Add test device 1");
+  yield addDeviceInModal(ui, device1);
+
+  info("Reveal device adder form");
+  adderShow = document.querySelector("#device-adder-show");
+  Simulate.click(adderShow);
+
+  info("Add test device 2");
+  yield addDeviceInModal(ui, device2);
+
+  info("Verify all custom devices default to enabled in modal");
+  let deviceCbs =
+    [...document.querySelectorAll(".device-type-custom .device-input-checkbox")];
+  is(deviceCbs.length, 2, "Both devices have a checkbox in modal");
+  for (let cb of deviceCbs) {
+    ok(cb.checked, "Custom device enabled");
+  }
+  Simulate.click(submitButton);
+
+  info("Look for device 1 in device selector");
+  let deviceOption1 = [...deviceSelector.options].find(opt => opt.value == device1.name);
+  ok(deviceOption1, "Test device 1 option added to device selector");
+
+  info("Look for device 2 in device selector");
+  let deviceOption2 = [...deviceSelector.options].find(opt => opt.value == device2.name);
+  ok(deviceOption2, "Test device 2 option added to device selector");
+
+  openDeviceModal(ui);
+
+  info("Remove device 2");
+  let deviceRemoveButtons = [...document.querySelectorAll(".device-remove-button")];
+  is(deviceRemoveButtons.length, 2, "Both devices have a remove button in modal");
+  let removed = waitUntilState(store, state => state.devices.custom.length == 1);
+  Simulate.click(deviceRemoveButtons[1]);
+  yield removed;
+  Simulate.click(submitButton);
+
+  info("Ensure device 1 is still in device selector");
+  deviceOption1 = [...deviceSelector.options].find(opt => opt.value == device1.name);
+  ok(deviceOption1, "Test device 1 option exists");
+
+  info("Ensure device 2 is no longer in device selector");
+  deviceOption2 = [...deviceSelector.options].find(opt => opt.value == device2.name);
+  ok(!deviceOption2, "Test device 2 option removed");
+});
+
+addRDMTask(TEST_URL, function* ({ ui }) {
+  let { toolWindow } = ui;
+  let { store, document } = toolWindow;
+
+  // Wait until the viewport has been added and the device list has been loaded
+  yield waitUntilState(store, state => state.viewports.length == 1
+    && state.devices.listState == Types.deviceListState.LOADED);
+
+  let deviceSelector = document.querySelector(".viewport-device-selector");
+
+  info("Ensure device 1 is still in device selector");
+  let deviceOption1 = [...deviceSelector.options].find(opt => opt.value == device1.name);
+  ok(deviceOption1, "Test device 1 option exists");
+
+  info("Ensure device 2 is no longer in device selector");
+  let deviceOption2 = [...deviceSelector.options].find(opt => opt.value == device2.name);
+  ok(!deviceOption2, "Test device 2 option removed");
+
+  openDeviceModal(ui);
+
+  info("Ensure device 1 is still in device modal");
+  let deviceCbs =
+    [...document.querySelectorAll(".device-type-custom .device-input-checkbox")];
+  is(deviceCbs.length, 1, "Only 1 custom present in modal");
+  let deviceCb1 = deviceCbs.find(cb => cb.value == device1.name);
+  ok(deviceCb1 && deviceCb1.checked, "Test device 1 checkbox exists and enabled");
+
+  info("Ensure device 2 is no longer in device modal");
+  let deviceCb2 = deviceCbs.find(cb => cb.value == device2.name);
+  ok(!deviceCb2, "Test device 2 checkbox does not exist");
+});
+
--- a/devtools/client/responsive.html/test/browser/head.js
+++ b/devtools/client/responsive.html/test/browser/head.js
@@ -376,8 +376,48 @@ function* toggleTouchSimulation(ui) {
 }
 
 function* testUserAgent(ui, expected) {
   let ua = yield ContentTask.spawn(ui.getViewportBrowser(), {}, function* () {
     return content.navigator.userAgent;
   });
   is(ua, expected, `UA should be set to ${expected}`);
 }
+
+/**
+ * Assuming the device modal is open and the device adder form is shown, this helper
+ * function adds `device` via the form, saves it, and waits for it to appear in the store.
+ */
+function addDeviceInModal(ui, device) {
+  let { toolWindow } = ui;
+  let { store, document } = ui.toolWindow;
+  let React = toolWindow.require("devtools/client/shared/vendor/react");
+  let { Simulate } = React.addons.TestUtils;
+
+  let nameInput = document.querySelector("#device-adder-name input");
+  let [ widthInput, heightInput ] = document.querySelectorAll("#device-adder-size input");
+  let pixelRatioInput = document.querySelector("#device-adder-pixel-ratio input");
+  let userAgentInput = document.querySelector("#device-adder-user-agent input");
+  let touchInput = document.querySelector("#device-adder-touch input");
+
+  nameInput.value = device.name;
+  Simulate.change(nameInput);
+  widthInput.value = device.width;
+  Simulate.change(widthInput);
+  Simulate.blur(widthInput);
+  heightInput.value = device.height;
+  Simulate.change(heightInput);
+  Simulate.blur(heightInput);
+  pixelRatioInput.value = device.pixelRatio;
+  Simulate.change(pixelRatioInput);
+  userAgentInput.value = device.userAgent;
+  Simulate.change(userAgentInput);
+  touchInput.checked = device.touch;
+  Simulate.change(touchInput);
+
+  let existingCustomDevices = store.getState().devices.custom.length;
+  let adderSave = document.querySelector("#device-adder-save");
+  let saved = waitUntilState(store, state =>
+    state.devices.custom.length == existingCustomDevices + 1
+  );
+  Simulate.click(adderSave);
+  return saved;
+}
--- a/devtools/client/shared/devices.js
+++ b/devtools/client/shared/devices.js
@@ -54,41 +54,50 @@ async function loadLocalDevices() {
     devicesJSON = "{}";
   }
   localDevices = JSON.parse(devicesJSON);
   localDevicesLoaded = true;
 }
 
 /**
  * Add a device to the local catalog.
+ * Returns `true` if the device is added, `false` otherwise.
  */
 async function addDevice(device, type = "phones") {
   await loadLocalDevices();
   let list = localDevices[type];
   if (!list) {
     list = localDevices[type] = [];
   }
+
+  // Ensure the new device is has a unique name
+  let exists = list.some(entry => entry.name == device.name);
+  if (exists) {
+    return false;
+  }
+
   list.push(Object.assign({}, device));
   await asyncStorage.setItem(LOCAL_DEVICES, JSON.stringify(localDevices));
+
+  return true;
 }
 
 /**
  * Remove a device from the local catalog.
  * Returns `true` if the device is removed, `false` otherwise.
  */
 async function removeDevice(device, type = "phones") {
   await loadLocalDevices();
   let list = localDevices[type];
   if (!list) {
     return false;
   }
 
-  let index = list.findIndex(item => device);
-
-  if (index === -1) {
+  let index = list.findIndex(entry => entry.name == device.name);
+  if (index == -1) {
     return false;
   }
 
   list.splice(index, 1);
   await asyncStorage.setItem(LOCAL_DEVICES, JSON.stringify(localDevices));
 
   return true;
 }