Bug 1453428 - Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels. r=gl
☠☠ backed out by bdbed4e49efb ☠ ☠
authorRazvan Caliman <rcaliman@mozilla.com>
Wed, 11 Apr 2018 22:21:18 +0200
changeset 467691 3f95bbb60e4399e9b7533aec0f42e52539f8b235
parent 467690 3ab522801015f2ff098f9a7958f8348a862955bf
child 467692 19faa628ee0317fa2435be2aa85e24166a63a834
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1453428, 0
milestone61.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 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
@@ -588,18 +588,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 };
   }
 
   /**
@@ -613,26 +613,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 };
   }
 
   /**
@@ -646,18 +646,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;
@@ -683,17 +683,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 };
   }
 
@@ -971,18 +971,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
@@ -1066,29 +1066,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.
@@ -1145,37 +1145,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.
@@ -1238,17 +1238,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.
@@ -2525,16 +2525,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.
  */
@@ -2714,21 +2754,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",