Bug 1289553 - Move the eyedropper label so it's always visible. r=jdescottes, a=ritu
authorPatrick Brosset <pbrosset@mozilla.com>
Thu, 18 Aug 2016 15:17:47 +0200
changeset 347856 e851620e760b9cd4171da96fb67ed929bbe1431e
parent 347855 f23d8ea54ced4980a8c375f41fce8afe5b2bdcab
child 347857 b0b52bebfd3dff0705f0efddc1d0827144eb1e21
push id6389
push userraliiev@mozilla.com
push dateMon, 19 Sep 2016 13:38:22 +0000
treeherdermozilla-beta@01d67bfe6c81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes, ritu
bugs1289553
milestone50.0a2
Bug 1289553 - Move the eyedropper label so it's always visible. r=jdescottes, a=ritu This adds a few new CSS selectors that are used to move the label to the top and/or left/right of the eyedropper canvas. The CSS rules use transform and a quick transition. The eyedropper highlighter then just makes use of this by adding top, left, right attributes to the DOM depending on its position. This also adds a test for this, and while testing, I discovered a bug in shared/layout/utils.js that I fixed here too. Sometimes, the node passed is actually a DOCUMENT_NODE and so we must account for this in a couple of places in this file to avoid JS errors. MozReview-Commit-ID: H969k3mEDJE MozReview-Commit-ID: 9qOCYVp4mld
devtools/client/inspector/test/browser.ini
devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-label.js
devtools/client/shared/test/test-actor.js
devtools/server/actors/highlighters.css
devtools/server/actors/highlighters/eye-dropper.js
devtools/shared/layout/utils.js
--- a/devtools/client/inspector/test/browser.ini
+++ b/devtools/client/inspector/test/browser.ini
@@ -61,16 +61,17 @@ skip-if = os == "mac" # Full keyboard na
 [browser_inspector_highlighter-by-type.js]
 [browser_inspector_highlighter-comments.js]
 [browser_inspector_highlighter-csstransform_01.js]
 [browser_inspector_highlighter-csstransform_02.js]
 [browser_inspector_highlighter-embed.js]
 [browser_inspector_highlighter-eyedropper-clipboard.js]
 subsuite = clipboard
 [browser_inspector_highlighter-eyedropper-events.js]
+[browser_inspector_highlighter-eyedropper-label.js]
 [browser_inspector_highlighter-eyedropper-show-hide.js]
 [browser_inspector_highlighter-geometry_01.js]
 [browser_inspector_highlighter-geometry_02.js]
 [browser_inspector_highlighter-geometry_03.js]
 [browser_inspector_highlighter-geometry_04.js]
 [browser_inspector_highlighter-geometry_05.js]
 [browser_inspector_highlighter-geometry_06.js]
 [browser_inspector_highlighter-hover_01.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-label.js
@@ -0,0 +1,115 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+// Test the position of the eyedropper label.
+// It should move around when the eyedropper is close to the edges of the viewport so as
+// to always stay visible.
+
+const HIGHLIGHTER_TYPE = "EyeDropper";
+const ID = "eye-dropper-";
+
+const HTML = `
+<style>
+html, body {height: 100%; margin: 0;}
+body {background: linear-gradient(red, gold); display: flex; justify-content: center;
+      align-items: center;}
+</style>
+Eyedropper label position test
+`;
+const TEST_PAGE = "data:text/html;charset=utf-8," + encodeURI(HTML);
+
+const TEST_DATA = [{
+  desc: "Move the mouse to the center of the screen",
+  getCoordinates: (width, height) => {
+    return {x: width / 2, y: height / 2};
+  },
+  expectedPositions: {top: false, right: false, left: false}
+}, {
+  desc: "Move the mouse to the center left",
+  getCoordinates: (width, height) => {
+    return {x: 0, y: height / 2};
+  },
+  expectedPositions: {top: false, right: true, left: false}
+}, {
+  desc: "Move the mouse to the center right",
+  getCoordinates: (width, height) => {
+    return {x: width, y: height / 2};
+  },
+  expectedPositions: {top: false, right: false, left: true}
+}, {
+  desc: "Move the mouse to the bottom center",
+  getCoordinates: (width, height) => {
+    return {x: width / 2, y: height};
+  },
+  expectedPositions: {top: true, right: false, left: false}
+}, {
+  desc: "Move the mouse to the bottom left",
+  getCoordinates: (width, height) => {
+    return {x: 0, y: height};
+  },
+  expectedPositions: {top: true, right: true, left: false}
+}, {
+  desc: "Move the mouse to the bottom right",
+  getCoordinates: (width, height) => {
+    return {x: width, y: height};
+  },
+  expectedPositions: {top: true, right: false, left: true}
+}, {
+  desc: "Move the mouse to the top left",
+  getCoordinates: (width, height) => {
+    return {x: 0, y: 0};
+  },
+  expectedPositions: {top: false, right: true, left: false}
+}, {
+  desc: "Move the mouse to the top right",
+  getCoordinates: (width, height) => {
+    return {x: width, y: 0};
+  },
+  expectedPositions: {top: false, right: false, left: true}
+}];
+
+add_task(function* () {
+  let {inspector, testActor} = yield openInspectorForURL(TEST_PAGE);
+  let helper = yield getHighlighterHelperFor(HIGHLIGHTER_TYPE)({inspector, testActor});
+  helper.prefix = ID;
+
+  let {mouse, show, hide, finalize} = helper;
+  let {width, height} = yield testActor.getBoundingClientRect("html");
+
+  // This test fails in non-e10s windows if we use width and height. For some reasons, the
+  // mouse events can't be dispatched/handled properly when we try to move the eyedropper
+  // to the far right and/or bottom of the screen. So just removing 10px from each side
+  // fixes it.
+  width -= 10;
+  height -= 10;
+
+  info("Show the eyedropper on the page");
+  yield show("html");
+
+  info("Move the eyedropper around and check that the label appears at the right place");
+  for (let {desc, getCoordinates, expectedPositions} of TEST_DATA) {
+    info(desc);
+    let {x, y} = getCoordinates(width, height);
+    info(`Moving the mouse to ${x} ${y}`);
+    yield mouse.move(x, y);
+    yield checkLabelPositionAttributes(helper, expectedPositions);
+  }
+
+  info("Hide the eyedropper");
+  yield hide();
+  finalize();
+});
+
+function* checkLabelPositionAttributes(helper, positions) {
+  for (let position in positions) {
+    is((yield hasAttribute(helper, position)), positions[position],
+       `The label was ${positions[position] ? "" : "not "}moved to the ${position}`);
+  }
+}
+
+function* hasAttribute({getElementAttribute}, name) {
+  let value = yield getElementAttribute("root", name);
+  return value !== null;
+}
--- a/devtools/client/shared/test/test-actor.js
+++ b/devtools/client/shared/test/test-actor.js
@@ -426,17 +426,28 @@ var TestActor = exports.TestActor = prot
 
   /**
    * Get the bounding rect for a given DOM node once.
    * @param {String} selector selector identifier to select the DOM node
    * @return {json} the bounding rect info
    */
   getBoundingClientRect: protocol.method(function (selector) {
     let node = this._querySelector(selector);
-    return node.getBoundingClientRect();
+    let rect = node.getBoundingClientRect();
+    // DOMRect can't be stringified directly, so return a simple object instead.
+    return {
+      x: rect.x,
+      y: rect.y,
+      width: rect.width,
+      height: rect.height,
+      top: rect.top,
+      right: rect.right,
+      bottom: rect.bottom,
+      left: rect.left
+    };
   }, {
     request: {
       selector: Arg(0, "string"),
     },
     response: {
       value: RetVal("json")
     }
   }),
--- a/devtools/server/actors/highlighters.css
+++ b/devtools/server/actors/highlighters.css
@@ -405,16 +405,17 @@
 
 /* Eye dropper */
 
 :-moz-native-anonymous .eye-dropper-root {
   --magnifier-width: 96px;
   --magnifier-height: 96px;
   /* Width accounts for all color formats (hsl being the longest) */
   --label-width: 160px;
+  --label-height: 23px;
   --color: #e0e0e0;
 
   position: absolute;
   /* Tool start position. This should match the X/Y defines in JS */
   top: 100px;
   left: 100px;
 
   /* Prevent interacting with the page when hovering and clicking */
@@ -440,18 +441,55 @@
   box-shadow: 0 0 0 3px var(--color);
   display: block;
 }
 
 :-moz-native-anonymous .eye-dropper-color-container {
   background-color: var(--color);
   border-radius: 2px;
   width: var(--label-width);
-  transform: translateX(calc((var(--magnifier-width) - var(--label-width)) / 2));
+  height: var(--label-height);
   position: relative;
+
+  --label-horizontal-center:
+    translateX(calc((var(--magnifier-width) - var(--label-width)) / 2));
+  --label-horizontal-left:
+    translateX(calc((-1 * var(--label-width) + var(--magnifier-width) / 2)));
+  --label-horizontal-right:
+    translateX(calc(var(--magnifier-width) / 2));
+  --label-vertical-top:
+    translateY(calc((-1 * var(--magnifier-height)) - var(--label-height)));
+
+  /* By default the color label container sits below the canvas.
+     Here we just center it horizontally */
+  transform: var(--label-horizontal-center);
+  transition: transform .1s ease-in-out;
+}
+
+/* If there isn't enough space below the canvas, we move the label container to the top */
+:-moz-native-anonymous .eye-dropper-root[top] .eye-dropper-color-container {
+  transform: var(--label-horizontal-center) var(--label-vertical-top);
+}
+
+/* If there isn't enough space right of the canvas to horizontally center the label
+   container, offset it to the left */
+:-moz-native-anonymous .eye-dropper-root[left] .eye-dropper-color-container {
+  transform: var(--label-horizontal-left);
+}
+:-moz-native-anonymous .eye-dropper-root[left][top] .eye-dropper-color-container {
+  transform: var(--label-horizontal-left) var(--label-vertical-top);
+}
+
+/* If there isn't enough space left of the canvas to horizontally center the label
+   container, offset it to the right */
+:-moz-native-anonymous .eye-dropper-root[right] .eye-dropper-color-container {
+  transform: var(--label-horizontal-right);
+}
+:-moz-native-anonymous .eye-dropper-root[right][top] .eye-dropper-color-container {
+  transform: var(--label-horizontal-right) var(--label-vertical-top);
 }
 
 :-moz-native-anonymous .eye-dropper-color-preview {
   width: 16px;
   height: 16px;
   position: absolute;
   offset-inline-start: 3px;
   offset-block-start: 3px;
--- a/devtools/server/actors/highlighters/eye-dropper.js
+++ b/devtools/server/actors/highlighters/eye-dropper.js
@@ -333,17 +333,35 @@ EyeDropper.prototype = {
       case "FullZoomChange":
         this.hide();
         this.show();
         break;
     }
   },
 
   moveTo(x, y) {
-    this.getElement("root").setAttribute("style", `top:${y}px;left:${x}px;`);
+    let root = this.getElement("root");
+    root.setAttribute("style", `top:${y}px;left:${x}px;`);
+
+    // Move the label container to the top if the magnifier is close to the bottom edge.
+    if (y >= this.win.innerHeight - MAGNIFIER_HEIGHT) {
+      root.setAttribute("top", "");
+    } else {
+      root.removeAttribute("top");
+    }
+
+    // Also offset the label container to the right or left if the magnifier is close to
+    // the edge.
+    root.removeAttribute("left");
+    root.removeAttribute("right");
+    if (x <= MAGNIFIER_WIDTH) {
+      root.setAttribute("right", "");
+    } else if (x >= this.win.innerWidth - MAGNIFIER_WIDTH) {
+      root.setAttribute("left", "");
+    }
   },
 
   /**
    * Select the current color that's being previewed. Depending on the current options,
    * selecting might mean copying to the clipboard and closing the
    */
   selectColor() {
     let onColorSelected = Promise.resolve();
--- a/devtools/shared/layout/utils.js
+++ b/devtools/shared/layout/utils.js
@@ -131,17 +131,18 @@ exports.getFrameElement = getFrameElemen
  * @param {DOMNode} node
  *        The node for which we are to get the offset
  * @return {Array}
  *         The frame offset [x, y]
  */
 function getFrameOffsets(boundaryWindow, node) {
   let xOffset = 0;
   let yOffset = 0;
-  let frameWin = node.ownerDocument.defaultView;
+
+  let frameWin = getWindowFor(node);
   let scale = getCurrentZoom(node);
 
   if (boundaryWindow === null) {
     boundaryWindow = getTopWindow(frameWin);
   } else if (typeof boundaryWindow === "undefined") {
     throw new Error("No boundaryWindow given. Use null for the default one.");
   }
 
@@ -656,23 +657,37 @@ exports.isShadowAnonymous = isShadowAnon
  * nsIDOMWindowUtils instance to avoid querying it every time.
  *
  * @param {DOMNode|DOMWindow}
  *        The node for which the zoom factor should be calculated, or its
  *        owner window.
  * @return {Number}
  */
 function getCurrentZoom(node) {
-  let win = null;
-
-  if (node instanceof Ci.nsIDOMNode) {
-    win = node.ownerDocument.defaultView;
-  } else if (node instanceof Ci.nsIDOMWindow) {
-    win = node;
-  }
+  let win = getWindowFor(node);
 
   if (!win) {
     throw new Error("Unable to get the zoom from the given argument.");
   }
 
   return utilsFor(win).fullZoom;
 }
 exports.getCurrentZoom = getCurrentZoom;
+
+/**
+ * Return the default view for a given node, where node can be:
+ * - a DOM node
+ * - the document node
+ * - the window itself
+ * @param {DOMNode|DOMWindow|DOMDocument} node The node to get the window for.
+ * @return {DOMWindow}
+ */
+function getWindowFor(node) {
+  if (node instanceof Ci.nsIDOMNode) {
+    if (node.nodeType === node.DOCUMENT_NODE) {
+      return node.defaultView;
+    }
+    return node.ownerDocument.defaultView;
+  } else if (node instanceof Ci.nsIDOMWindow) {
+    return node;
+  }
+  return null;
+}