Bug 1435368 - Add tests for decimal precision rounding on Shapes editor. Fix broken test. Address comments from previous review. r=gl draft
authorRazvan Caliman <rcaliman@mozilla.com>
Fri, 09 Feb 2018 17:21:05 +0100
changeset 753072 c00c706641d8483907e01d5bec7ae6181ca61049
parent 753071 82bc9161d14d0184d6f2a760ebc66e89fa351f71
push id98476
push userbmo:rcaliman@mozilla.com
push dateFri, 09 Feb 2018 16:29:28 +0000
reviewersgl
bugs1435368
milestone60.0a1
Bug 1435368 - Add tests for decimal precision rounding on Shapes editor. Fix broken test. Address comments from previous review. r=gl MozReview-Commit-ID: 8wEB05Z58vS
devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js
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
@@ -82,17 +82,23 @@ function* testPolygonAddPoint(testActor,
   };
 
   info("Adding new polygon point");
   yield testActor.synthesizeMouse(options);
   yield testActor.reflow();
 
   let computedStyle = yield highlightedNode.getComputedStyle();
   let definition = computedStyle["clip-path"].value;
-  ok(definition.includes(`${newPointX * 100 / width}% ${newPointY * 100 / height}%`),
+  // Decimal precision for coordinates with percentage units is 2
+  let precisionX = 2;
+  let precisionY = 2;
+  // Round to the desired decimal precision and cast to Number to remove trailing zeroes.
+  newPointX = Number((newPointX * 100 / width).toFixed(precisionX));
+  newPointY = Number((newPointY * 100 / height).toFixed(precisionY));
+  ok(definition.includes(`${newPointX}% ${newPointY}%`),
      "Point successfuly added");
 }
 
 function* testPolygonRemovePoint(testActor, helper) {
   yield helper.show("#polygon", {mode: "cssClipPath"});
   let { highlightedNode } = helper;
 
   let points = yield helper.getElementAttribute("shapes-polygon", "points");
--- a/devtools/server/actors/highlighters/shapes.js
+++ b/devtools/server/actors/highlighters/shapes.js
@@ -663,39 +663,16 @@ class ShapesHighlighter extends AutoRefr
       pointsInfo[point] = { value, unit, ratio };
     });
     this[_dragging] = { type, pointsInfo, x: pageX, y: pageY, bb: this.boundingBox,
                         matrix: this.transformMatrix,
                         transformedBB: this.transformedBoundingBox };
   }
 
   /**
-  * Get the decimal point precision for values depending on unit type.
-  * Used when transforming shapes and inserting new points on a polygon.
-  *
-  * @param {String} unitType any one of the accepted CSS unit types for position.
-  *
-  * @return {Number} decimal precision when rounding a value
-  */
-  _getDecimalPrecision(unitType) {
-    // Only handle pixels for now. Round them to the nearest integer value.
-    // Falsy unit type values behave like pixels.
-    // All other unit types, round to two decimal points.
-    switch (unitType) {
-      case "px":
-      case "":
-      case undefined:
-        return 0;
-        break;
-      default:
-        return 2;
-    }
-  }
-
-  /**
    * Handle mouse movement after a click on a handle in transform mode.
    * @param {Number} pageX the x coordinate of the mouse
    * @param {Number} pageY the y coordinate of the mouse
    */
   _handleTransformMove(pageX, pageY) {
     let { type } = this[_dragging];
     if (type === "translate") {
       this._translateShape(pageX, pageY);
@@ -852,18 +829,18 @@ class ShapesHighlighter extends AutoRefr
   _transformPolygon() {
     let { pointsInfo } = this[_dragging];
 
     let polygonDef = (this.fillRule) ? `${this.fillRule}, ` : "";
     polygonDef += pointsInfo.map(point => {
       let { unitX, unitY, valueX, valueY, ratioX, ratioY } = point;
       let vector = [valueX / ratioX, valueY / ratioY];
       let [newX, newY] = apply(this.transformMatrix, vector);
-      let precisionX = this._getDecimalPrecision(unitX);
-      let precisionY = this._getDecimalPrecision(unitY);
+      let precisionX = getDecimalPrecision(unitX);
+      let precisionY = getDecimalPrecision(unitY);
       newX = (newX * ratioX).toFixed(precisionX);
       newY = (newY * ratioY).toFixed(precisionY);
 
       return `${newX}${unitX} ${newY}${unitY}`;
     }).join(", ");
     polygonDef = (this.geometryBox) ? `polygon(${polygonDef}) ${this.geometryBox}` :
                                       `polygon(${polygonDef})`;
 
@@ -988,34 +965,36 @@ class ShapesHighlighter extends AutoRefr
    * coords.
    * @param {Number} pageX the new x coordinate of the point
    * @param {Number} pageY the new y coordinate of the point
    */
   _handlePolygonMove(pageX, pageY) {
     let { point, unitX, unitY, valueX, valueY, ratioX, ratioY, x, y } = this[_dragging];
     let deltaX = (pageX - x) * ratioX;
     let deltaY = (pageY - y) * ratioY;
-    let precisionX = this._getDecimalPrecision(unitX);
-    let precisionY = this._getDecimalPrecision(unitY);
+    let precisionX = getDecimalPrecision(unitX);
+    let precisionY = getDecimalPrecision(unitY);
     let newX = (valueX + deltaX).toFixed(precisionX);
     let newY = (valueY + deltaY).toFixed(precisionY);
 
     let polygonDef = (this.fillRule) ? `${this.fillRule}, ` : "";
     polygonDef += this.coordUnits.map((coords, i) => {
-      return (i === point) ? `${newX}${unitX} ${newY}${unitY}` : `${coords[0]} ${coords[1]}`;
+      return (i === point) ?
+        `${newX}${unitX} ${newY}${unitY}` : `${coords[0]} ${coords[1]}`;
     }).join(", ");
     polygonDef = (this.geometryBox) ? `polygon(${polygonDef}) ${this.geometryBox}` :
                                       `polygon(${polygonDef})`;
 
     this.currentNode.style.setProperty(this.property, polygonDef, "important");
   }
 
   /**
    * Set the inline style of the polygon, adding a new point.
-   * TODO: Do not default to percentage unit when inserting new point. Look at adjacent coordinates.
+   * TODO: Do not default to percentage unit when inserting new point.
+   * Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1436054
    *
    * @param {Number} after the index of the point that the new point should be added after
    * @param {Number} x the x coordinate of the new point
    * @param {Number} y the y coordinate of the new point
    */
   _addPolygonPoint(after, x, y) {
     let polygonDef = (this.fillRule) ? `${this.fillRule}, ` : "";
     polygonDef += this.coordUnits.map((coords, i) => {
@@ -1594,18 +1573,19 @@ class ShapesHighlighter extends AutoRefr
       let distance = distanceToLine(x1, y1, x2, y2, pageX, pageY);
       if (distance <= clickWidth &&
           Math.min(x1, x2) - clickWidth <= pageX &&
           pageX <= Math.max(x1, x2) + clickWidth &&
           Math.min(y1, y2) - clickWidth <= pageY &&
           pageY <= Math.max(y1, y2) + clickWidth) {
         // Get the point on the line closest to the clicked point.
         let [newX, newY] = projection(x1, y1, x2, y2, pageX, pageY);
-        let precisionX = this._getDecimalPrecision();
-        let precisionY = this._getDecimalPrecision();
+        // Default unit for new points is percentages
+        let precisionX = getDecimalPrecision("%");
+        let precisionY = getDecimalPrecision("%");
         this._addPolygonPoint(i, newX.toFixed(precisionX), newY.toFixed(precisionY));
         return;
       }
     }
   }
 
   /**
    * Check if the center point or radius of the circle highlighter is at given coords
@@ -2757,9 +2737,32 @@ const getAnchorPoint = (type) => {
     anchor = "n" + anchor;
   } else if (anchor === "n" || anchor === "s") {
     anchor = anchor + "w";
   }
 
   return anchor;
 };
 
+/**
+* Get the decimal point precision for values depending on unit type.
+* Used as argument for `toFixed()` on coordinate values when:
+* - transforming shapes
+* - inserting new points on a polygon.
+* Only handle pixels and falsy values for now. Round them to the nearest integer value.
+* All other unit types round to two decimal points.
+*
+* @param {String|undefined} unitType any one of the accepted CSS unit types for position.
+* @return {Number} decimal precision when rounding a value
+*/
+function getDecimalPrecision(unitType) {
+  switch (unitType) {
+    case "px":
+    case "":
+    case undefined:
+      return 0;
+    default:
+      return 2;
+  }
+}
+exports.getDecimalPrecision = getDecimalPrecision;
+
 exports.ShapesHighlighter = ShapesHighlighter;
--- a/devtools/server/tests/unit/test_shapes_highlighter_helpers.js
+++ b/devtools/server/tests/unit/test_shapes_highlighter_helpers.js
@@ -8,25 +8,27 @@
 "use strict";
 
 const {
   splitCoords,
   coordToPercent,
   evalCalcExpression,
   shapeModeToCssPropertyName,
   getCirclePath,
+  getDecimalPrecision,
   getUnit
 } = require("devtools/server/actors/highlighters/shapes");
 
 function run_test() {
   test_split_coords();
   test_coord_to_percent();
   test_eval_calc_expression();
   test_shape_mode_to_css_property_name();
   test_get_circle_path();
+  test_get_decimal_precision();
   test_get_unit();
   run_next_test();
 }
 
 function test_split_coords() {
   const tests = [{
     desc: "splitCoords for basic coordinate pair",
     expr: "30% 20%",
@@ -123,16 +125,39 @@ function test_get_circle_path() {
     expected: "M-2.5,0a2.5,1.25 0 1,0 5,0a2.5,1.25 0 1,0 -5,0"
   }];
 
   for (let { desc, size, cx, cy, width, height, zoom, expected } of tests) {
     equal(getCirclePath(size, cx, cy, width, height, zoom), expected, desc);
   }
 }
 
+function test_get_decimal_precision() {
+  const tests = [{
+    desc: "getDecimalPrecision with px",
+    expr: "px", expected: 0
+  }, {
+    desc: "getDecimalPrecision with %",
+    expr: "%", expected: 2
+  }, {
+    desc: "getDecimalPrecision with em",
+    expr: "em", expected: 2
+  }, {
+    desc: "getDecimalPrecision with undefined",
+    expr: undefined, expected: 0
+  }, {
+    desc: "getDecimalPrecision with empty string",
+    expr: "", expected: 0
+  }];
+
+  for (let { desc, expr, expected } of tests) {
+    equal(getDecimalPrecision(expr), expected, desc);
+  }
+}
+
 function test_get_unit() {
   const tests = [{
     desc: "getUnit with %",
     expr: "30%", expected: "%"
   }, {
     desc: "getUnit with px",
     expr: "400px", expected: "px"
   }, {