Bug 1453428 - Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels. r=gl draft
authorRazvan Caliman <rcaliman@mozilla.com>
Wed, 11 Apr 2018 22:21:18 +0200
changeset 782864 fa560c5e8e470a188f5d159930ca4bf56eb1a3ea
parent 780394 f6c3a0a19d82db25750d8badccd5cf37e79d028e
push id106557
push userbmo:rcaliman@mozilla.com
push dateMon, 16 Apr 2018 09:16:13 +0000
reviewersgl
bugs1453428, 0
milestone61.0a1
Bug 1453428 - Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels. r=gl The previous implementation treated any zero coordinate as pixels, regardless of its unit type. For example, 0% would be converted to 0px. This changed the shape value resulting in unintentional unit mismatch after editing a coordinate which started off as 0% or 0em or 0vh, etc. This patch fixes that and preserves the unit type for zero coordinates. It changes the implementation of isUnitless() to return false for values like 0%, 0px, 0em, 0.00000%, etc. It introduces getUnitToPixelRatio() to get the ratio by which to multiply a pixel value to convert it to the given unit during shape editing operations (point move, scale, translate, rotate, etc.) The ratio is constant by unit type. Previously, this ratio was calculated in-place for each unit value. Values which started as zero, always resulted in a ratio equal to zero. This multiplied with a pixel value resulted zero. So the previous code defaulted to ratio of 1 when the ratio was zero, but this meant that a 1:1 ratio between pixels and another unit type (%, em, vh) would result in disproportionate shape changes (1px of mouse travel would result in 1em). This is why isUnitless() originally discarded the original unit from a zero coordinate and replaced it with pixels; to account for this fallback ratio of 1. MozReview-Commit-ID: 49tyLfYjkLO
devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js
devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js
devtools/client/inspector/test/doc_inspector_highlighter_cssshapes.html
devtools/server/actors/highlighters/shapes.js
devtools/server/tests/unit/test_shapes_highlighter_helpers.js
--- a/devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js
+++ b/devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js
@@ -57,30 +57,32 @@ async function testPolygonMovePoint(conf
   let points = await testActor.getHighlighterNodeAttribute(
     "shapes-polygon", "points", highlighters.highlighters[HIGHLIGHTER_TYPE]);
   let [x, y] = points.split(" ")[0].split(",");
   let quads = await testActor.getAllAdjustedQuads(selector);
   let { top, left, width, height } = quads.border[0].bounds;
   x = left + width * x / 100;
   y = top + height * y / 100;
   let dx = width / 10;
-  let dy = height / 10;
+  let dyPercent = 10;
+  let dy = height / dyPercent;
 
   let onRuleViewChanged = view.once("ruleview-changed");
   info("Moving first polygon point");
   let { mouse } = helper;
   await mouse.down(x, y);
   await mouse.move(x + dx, y + dy);
   await mouse.up();
   await testActor.reflow();
   info("Waiting for rule view changed from shape change");
   await onRuleViewChanged;
 
   let definition = await getComputedPropertyValue(selector, property, inspector);
-  ok(definition.includes(`${dx}px ${dy}px`), `Point moved to ${dx}px ${dy}px`);
+  ok(definition.includes(`${dx}px ${dyPercent}%`),
+    `Point moved to ${dx}px ${dyPercent}%`);
 
   await teardown({selector, property, ...config});
 }
 
 async function testPolygonAddPoint(config) {
   const {inspector, view, highlighters, testActor, helper} = config;
   const selector = "#polygon";
   const property = "clip-path";
--- a/devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js
+++ b/devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js
@@ -30,30 +30,34 @@ async function testPolygonIframeMovePoin
   let highlightedNode = await getNodeFrontInFrame(selector, "#frame", inspector);
   // Select the nested node so toggling of the shapes highlighter works from the rule view
   await selectNode(highlightedNode, inspector);
   await toggleShapesHighlighter(view, selector, property, true);
   let { mouse } = helper;
 
   let onRuleViewChanged = view.once("ruleview-changed");
   info("Moving polygon point visible in iframe");
+  // Iframe has 10px margin. Element in iframe is 800px by 800px. First point is at 0 0%
   await mouse.down(10, 10);
   await mouse.move(20, 20);
   await mouse.up();
   await testActor.reflow();
   await onRuleViewChanged;
 
   let computedStyle = await inspector.pageStyle.getComputed(highlightedNode);
   let definition = computedStyle["clip-path"].value;
-  ok(definition.includes("10px 10px"), "Point moved to 10px 10px");
+  ok(definition.includes("10px 1.25%"), "Point moved to 10px 1.25%");
 
   onRuleViewChanged = view.once("ruleview-changed");
   info("Moving polygon point not visible in iframe");
   await mouse.down(110, 410);
   await mouse.move(120, 420);
   await mouse.up();
   await testActor.reflow();
   await onRuleViewChanged;
 
   computedStyle = await inspector.pageStyle.getComputed(highlightedNode);
   definition = computedStyle["clip-path"].value;
   ok(definition.includes("110px 51.25%"), "Point moved to 110px 51.25%");
+
+  info(`Turn off shapes highlighter for ${selector}`);
+  await toggleShapesHighlighter(view, selector, property, false);
 }
--- a/devtools/client/inspector/test/doc_inspector_highlighter_cssshapes.html
+++ b/devtools/client/inspector/test/doc_inspector_highlighter_cssshapes.html
@@ -8,17 +8,17 @@
     margin: 0;
   }
   .wrapper {
     width: 800px;
     height: 800px;
     background: #f06;
   }
   #polygon {
-    clip-path: polygon(0 0,
+    clip-path: polygon(0 0%,
                        100px 50%,
                        200px 0,
                        300px 50%,
                        400px 0,
                        500px 50%,
                        600px 0,
                        700px 50%,
                        800px 0,
--- a/devtools/server/actors/highlighters/shapes.js
+++ b/devtools/server/actors/highlighters/shapes.js
@@ -558,18 +558,18 @@ class ShapesHighlighter extends AutoRefr
     let pointsInfo = this.origCoordUnits.map(([x, y], i) => {
       let xComputed = this.origCoordinates[i][0] / 100 * width;
       let yComputed = this.origCoordinates[i][1] / 100 * height;
       let unitX = getUnit(x);
       let unitY = getUnit(y);
       let valueX = (isUnitless(x)) ? xComputed : parseFloat(x);
       let valueY = (isUnitless(y)) ? yComputed : parseFloat(y);
 
-      let ratioX = (valueX / xComputed) || 1;
-      let ratioY = (valueY / yComputed) || 1;
+      let ratioX = this.getUnitToPixelRatio(unitX, width);
+      let ratioY = this.getUnitToPixelRatio(unitY, height);
       return { unitX, unitY, valueX, valueY, ratioX, ratioY };
     });
     this[_dragging] = { type, pointsInfo, x: pageX, y: pageY, bb: this.boundingBox,
                         matrix: this.transformMatrix,
                         transformedBB: this.transformedBoundingBox };
   }
 
   /**
@@ -583,26 +583,26 @@ class ShapesHighlighter extends AutoRefr
     let { cx, cy } = this.origCoordUnits;
     let cxComputed = this.origCoordinates.cx / 100 * width;
     let cyComputed = this.origCoordinates.cy / 100 * height;
     let unitX = getUnit(cx);
     let unitY = getUnit(cy);
     let valueX = (isUnitless(cx)) ? cxComputed : parseFloat(cx);
     let valueY = (isUnitless(cy)) ? cyComputed : parseFloat(cy);
 
-    let ratioX = (valueX / cxComputed) || 1;
-    let ratioY = (valueY / cyComputed) || 1;
+    let ratioX = this.getUnitToPixelRatio(unitX, width);
+    let ratioY = this.getUnitToPixelRatio(unitY, height);
 
     let { radius } = this.origCoordinates;
     let computedSize = Math.sqrt((width ** 2) + (height ** 2)) / Math.sqrt(2);
     radius = radius / 100 * computedSize;
     let valueRad = this.origCoordUnits.radius;
     let unitRad = getUnit(valueRad);
     valueRad = (isUnitless(valueRad)) ? radius : parseFloat(valueRad);
-    let ratioRad = (valueRad / radius) || 1;
+    let ratioRad = this.getUnitToPixelRatio(unitRad, computedSize);
 
     this[_dragging] = { type, unitX, unitY, unitRad, valueX, valueY,
                         ratioX, ratioY, ratioRad, x: pageX, y: pageY,
                         bb: this.boundingBox, matrix: this.transformMatrix,
                         transformedBB: this.transformedBoundingBox };
   }
 
   /**
@@ -616,18 +616,18 @@ class ShapesHighlighter extends AutoRefr
     let { cx, cy } = this.origCoordUnits;
     let cxComputed = this.origCoordinates.cx / 100 * width;
     let cyComputed = this.origCoordinates.cy / 100 * height;
     let unitX = getUnit(cx);
     let unitY = getUnit(cy);
     let valueX = (isUnitless(cx)) ? cxComputed : parseFloat(cx);
     let valueY = (isUnitless(cy)) ? cyComputed : parseFloat(cy);
 
-    let ratioX = (valueX / cxComputed) || 1;
-    let ratioY = (valueY / cyComputed) || 1;
+    let ratioX = this.getUnitToPixelRatio(unitX, width);
+    let ratioY = this.getUnitToPixelRatio(unitY, height);
 
     let { rx, ry } = this.origCoordinates;
     rx = rx / 100 * width;
     let valueRX = this.origCoordUnits.rx;
     let unitRX = getUnit(valueRX);
     valueRX = (isUnitless(valueRX)) ? rx : parseFloat(valueRX);
     let ratioRX = (valueRX / rx) || 1;
     ry = ry / 100 * height;
@@ -653,17 +653,17 @@ class ShapesHighlighter extends AutoRefr
     let { width, height } = this.currentDimensions;
     let pointsInfo = {};
     ["top", "right", "bottom", "left"].forEach(point => {
       let value = this.origCoordUnits[point];
       let size = (point === "left" || point === "right") ? width : height;
       let computedValue = this.origCoordinates[point] / 100 * size;
       let unit = getUnit(value);
       value = (isUnitless(value)) ? computedValue : parseFloat(value);
-      let ratio = (value / computedValue) || 1;
+      let ratio = this.getUnitToPixelRatio(unit, size);
 
       pointsInfo[point] = { value, unit, ratio };
     });
     this[_dragging] = { type, pointsInfo, x: pageX, y: pageY, bb: this.boundingBox,
                         matrix: this.transformMatrix,
                         transformedBB: this.transformedBoundingBox };
   }
 
@@ -941,18 +941,18 @@ class ShapesHighlighter extends AutoRefr
     let [x, y] = this.coordUnits[point];
     let xComputed = this.coordinates[point][0] / 100 * width;
     let yComputed = this.coordinates[point][1] / 100 * height;
     let unitX = getUnit(x);
     let unitY = getUnit(y);
     let valueX = (isUnitless(x)) ? xComputed : parseFloat(x);
     let valueY = (isUnitless(y)) ? yComputed : parseFloat(y);
 
-    let ratioX = (valueX / xComputed) || 1;
-    let ratioY = (valueY / yComputed) || 1;
+    let ratioX = this.getUnitToPixelRatio(unitX, width);
+    let ratioY = this.getUnitToPixelRatio(unitY, height);
 
     this.setCursor("grabbing");
     this[_dragging] = { point, unitX, unitY, valueX, valueY,
                         ratioX, ratioY, x: pageX, y: pageY };
   }
 
   /**
    * Update the dragged polygon point with the given x/y coords and update
@@ -1036,29 +1036,29 @@ class ShapesHighlighter extends AutoRefr
       let { cx, cy } = this.coordUnits;
       let cxComputed = this.coordinates.cx / 100 * width;
       let cyComputed = this.coordinates.cy / 100 * height;
       let unitX = getUnit(cx);
       let unitY = getUnit(cy);
       let valueX = (isUnitless(cx)) ? cxComputed : parseFloat(cx);
       let valueY = (isUnitless(cy)) ? cyComputed : parseFloat(cy);
 
-      let ratioX = (valueX / cxComputed) || 1;
-      let ratioY = (valueY / cyComputed) || 1;
+      let ratioX = this.getUnitToPixelRatio(unitX, width);
+      let ratioY = this.getUnitToPixelRatio(unitY, height);
 
       this[_dragging] = { point, unitX, unitY, valueX, valueY,
                           ratioX, ratioY, x: pageX, y: pageY };
     } else if (point === "radius") {
       let { radius } = this.coordinates;
       let computedSize = Math.sqrt((width ** 2) + (height ** 2)) / Math.sqrt(2);
       radius = radius / 100 * computedSize;
       let value = this.coordUnits.radius;
       let unit = getUnit(value);
       value = (isUnitless(value)) ? radius : parseFloat(value);
-      let ratio = (value / radius) || 1;
+      let ratio = this.getUnitToPixelRatio(unit, computedSize);
 
       this[_dragging] = { point, value, origRadius: radius, unit, ratio };
     }
   }
 
   /**
    * Set the center/radius of the circle according to the mouse position and
    * update the element style.
@@ -1115,37 +1115,37 @@ class ShapesHighlighter extends AutoRefr
       let { cx, cy } = this.coordUnits;
       let cxComputed = this.coordinates.cx / 100 * width;
       let cyComputed = this.coordinates.cy / 100 * height;
       let unitX = getUnit(cx);
       let unitY = getUnit(cy);
       let valueX = (isUnitless(cx)) ? cxComputed : parseFloat(cx);
       let valueY = (isUnitless(cy)) ? cyComputed : parseFloat(cy);
 
-      let ratioX = (valueX / cxComputed) || 1;
-      let ratioY = (valueY / cyComputed) || 1;
+      let ratioX = this.getUnitToPixelRatio(unitX, width);
+      let ratioY = this.getUnitToPixelRatio(unitY, height);
 
       this[_dragging] = { point, unitX, unitY, valueX, valueY,
                           ratioX, ratioY, x: pageX, y: pageY };
     } else if (point === "rx") {
       let { rx } = this.coordinates;
       rx = rx / 100 * width;
       let value = this.coordUnits.rx;
       let unit = getUnit(value);
       value = (isUnitless(value)) ? rx : parseFloat(value);
-      let ratio = (value / rx) || 1;
+      let ratio = this.getUnitToPixelRatio(unit, width);
 
       this[_dragging] = { point, value, origRadius: rx, unit, ratio };
     } else if (point === "ry") {
       let { ry } = this.coordinates;
       ry = ry / 100 * height;
       let value = this.coordUnits.ry;
       let unit = getUnit(value);
       value = (isUnitless(value)) ? ry : parseFloat(value);
-      let ratio = (value / ry) || 1;
+      let ratio = this.getUnitToPixelRatio(unit, height);
 
       this[_dragging] = { point, value, origRadius: ry, unit, ratio };
     }
   }
 
   /**
    * Set center/rx/ry of the ellispe according to the mouse position and update the
    * element style.
@@ -1208,17 +1208,17 @@ class ShapesHighlighter extends AutoRefr
     }
 
     this.setCursor("grabbing");
     let value = this.coordUnits[point];
     let size = (point === "left" || point === "right") ? width : height;
     let computedValue = this.coordinates[point] / 100 * size;
     let unit = getUnit(value);
     value = (isUnitless(value)) ? computedValue : parseFloat(value);
-    let ratio = (value / computedValue) || 1;
+    let ratio = this.getUnitToPixelRatio(unit, size);
     let origValue = (point === "left" || point === "right") ? pageX : pageY;
 
     this[_dragging] = { point, value, origValue, unit, ratio };
   }
 
   /**
    * Set the top/left/right/bottom of the inset shape according to the mouse position
    * and update the element style.
@@ -2495,16 +2495,56 @@ class ShapesHighlighter extends AutoRefr
       return "ns";
     } else if (dy >= -0.33 && dy <= 0.33) {
       return "ew";
     } else if ((dx > 0.33 && dy < -0.33) || (dx < -0.33 && dy > 0.33)) {
       return "nesw";
     }
     return "nwse";
   }
+
+  /**
+   * Given a unit type, get the ratio by which to multiply a pixel value in order to
+   * convert pixels to that unit.
+   *
+   * Percentage units (%) are relative to a size. This must be provided when requesting
+   * a ratio for converting from pixels to percentages.
+   *
+   * @param {String} unit
+   *        One of: %, em, rem, vw, vh
+   * @param {Number} size
+   *        Size to which percentage values are relative to.
+   * @return {Number}
+   */
+  getUnitToPixelRatio(unit, size) {
+    let ratio;
+    switch (unit) {
+      case "%":
+        ratio = 100 / size;
+        break;
+      case "em":
+        ratio = 1 / parseFloat(getComputedStyle(this.currentNode).fontSize);
+        break;
+      case "rem":
+        const root = this.currentNode.ownerDocument.documentElement;
+        ratio = 1 / parseFloat(getComputedStyle(root).fontSize);
+        break;
+      case "vw":
+        ratio = 100 / this.win.innerWidth;
+        break;
+      case "vh":
+        ratio = 100 / this.win.innerHeight;
+        break;
+      default:
+        // If unit is not recognized, peg ratio 1:1 to pixels.
+        ratio = 1;
+    }
+
+    return ratio;
+  }
 }
 
 /**
  * Get the "raw" (i.e. non-computed) shape definition on the given node.
  * @param {nsIDOMNode} node the node to analyze
  * @param {String} property the CSS property for which a value should be retrieved.
  * @returns {String} the value of the given CSS property on the given node.
  */
@@ -2684,21 +2724,20 @@ const getUnit = (point) => {
 exports.getUnit = getUnit;
 
 /**
  * Check if the given point value has a unit.
  * @param {any} point a point value.
  * @returns {Boolean} whether the given value has a unit.
  */
 const isUnitless = (point) => {
-  // We treat all values that evaluate to 0 as unitless, regardless of whether
-  // they originally had a unit.
   return !point ||
          !point.match(/[^\d]+$/) ||
-         parseFloat(point) === 0 ||
+         // If zero doesn't have a unit, its numeric and string forms should be equal.
+         (parseFloat(point) === 0 && (parseFloat(point).toString() === point)) ||
          point.includes("(") ||
          point === "closest-side" ||
          point === "farthest-side";
 };
 
 /**
  * Return the anchor corresponding to the given scale type.
  * @param {String} type a scale type, of form "scale-[direction]"
--- a/devtools/server/tests/unit/test_shapes_highlighter_helpers.js
+++ b/devtools/server/tests/unit/test_shapes_highlighter_helpers.js
@@ -163,20 +163,26 @@ function test_get_unit() {
   }, {
     desc: "getUnit with em",
     expr: "4em", expected: "em"
   }, {
     desc: "getUnit with 0",
     expr: "0", expected: "px"
   }, {
     desc: "getUnit with 0%",
-    expr: "0%", expected: "px"
+    expr: "0%", expected: "%"
+  }, {
+    desc: "getUnit with 0.00%",
+    expr: "0.00%", expected: "%"
   }, {
-    desc: "getUnit with no unit",
-    expr: "30", expected: "px"
+    desc: "getUnit with 0px",
+    expr: "0px", expected: "px"
+  }, {
+    desc: "getUnit with 0em",
+    expr: "0em", expected: "em"
   }, {
     desc: "getUnit with calc",
     expr: "calc(30px + 5%)", expected: "px"
   }, {
     desc: "getUnit with var",
     expr: "var(--variable)", expected: "px"
   }, {
     desc: "getUnit with closest-side",