1435368 - Too many decimal points are displayed in the rule-view when editing a shape

Status: VERIFIED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

Attachment: Screen Shot 2018-02-02 at 15.02.18.png

When using the shape editor tool, it's pretty common that editing a point will result in very very precise x/y coordinates in the rule-view.

It often goes as far as 4 decimal points like shown in the screenshot.

I think this is unnecessarily complex, and doesn't help the user experience.
We should fix this.

Comment 1

[Patrick Brosset <:pbro>]

The code that generates those coordinates is in /devtools/server/actors/highlighters/shapes.js
In particular, some of it is done in convertCoordsToPercent() I believe.
I think this problem only occurs with % units anyway. But I can't be sure, there are many places in this file where we calculate some sort of coordinates.

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

Review commit: https://reviewboard.mozilla.org/r/218130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218130/

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

https://reviewboard.mozilla.org/r/218130/#review223974


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/highlighters/shapes.js:687
(Diff revision 1)
> +    switch (unitType) {
> +      case "px":
> +      case "":
> +      case undefined:
> +        return 0;
> +        break;

Error: Unreachable code. [eslint: no-unreachable]

::: devtools/server/actors/highlighters/shapes.js:1003
(Diff revision 1)
> +    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} ${newY}` : `${coords[0]} ${coords[1]}`;
> +      return (i === point) ? `${newX}${unitX} ${newY}${unitY}` : `${coords[0]} ${coords[1]}`;

Error: Line 1003 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/shapes.js:1013
(Diff revision 1)
>      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.

Error: Line 1013 exceeds the maximum line length of 90. [eslint: max-len]

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

https://reviewboard.mozilla.org/r/218130/#review223972

We still need to make sure unit tests are passing, and I assume we need to fix or add some new test cases.

::: devtools/server/actors/highlighters/shapes.js:674
(Diff revision 1)
>  
>    /**
> +  * 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.

s/String/String|undefined

::: devtools/server/actors/highlighters/shapes.js:675
(Diff revision 1)
>    /**
> +  * 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.
> +  *

Remove new line

::: devtools/server/actors/highlighters/shapes.js:678
(Diff revision 1)
> +  *
> +  * @param {String} unitType any one of the accepted CSS unit types for position.
> +  *
> +  * @return {Number} decimal precision when rounding a value
> +  */
> +  _getDecimalPrecision(unitType) {

Move this to a util function at the bottom file.

::: devtools/server/actors/highlighters/shapes.js:679
(Diff revision 1)
> +  * @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.

Move this into the JSDoc

::: devtools/server/actors/highlighters/shapes.js:1013
(Diff revision 1)
>      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.

Add Bug Number.

Comment 5

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1435368 - Add tests for decimal precision rounding on Shapes editor. Fix broken test. Address comments from previous review.

Review commit: https://reviewboard.mozilla.org/r/219090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219090/

Comment 6

[Razvan Caliman [:rcaliman]]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218130/diff/1-2/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

https://reviewboard.mozilla.org/r/218130/#review224872


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/highlighters/shapes.js:687
(Diff revision 2)
> +    switch (unitType) {
> +      case "px":
> +      case "":
> +      case undefined:
> +        return 0;
> +        break;

Error: Unreachable code. [eslint: no-unreachable]

::: devtools/server/actors/highlighters/shapes.js:1003
(Diff revision 2)
> +    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} ${newY}` : `${coords[0]} ${coords[1]}`;
> +      return (i === point) ? `${newX}${unitX} ${newY}${unitY}` : `${coords[0]} ${coords[1]}`;

Error: Line 1003 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/shapes.js:1013
(Diff revision 2)
>      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.

Error: Line 1013 exceeds the maximum line length of 90. [eslint: max-len]

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

These 2 commits should really be squashed into one.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8949785 [details]
Bug 1435368 - Add tests for decimal precision rounding on Shapes editor. Fix broken test. Address comments from previous review.

https://reviewboard.mozilla.org/r/219090/#review224942

These 2 commits need to be squashed together before I can provide an r+

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:91
(Diff revision 1)
>    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;

This seems unnecssary to set both precisionX/Y to 2 since it's not being used more than once.

I would rather do let precision = 2 or toFixed(2) since we already have the comment that we are setting the decimal precision to 2 anyways.

::: devtools/server/actors/highlighters/shapes.js:992
(Diff revision 1)
>    }
>  
>    /**
>     * 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

Reformat it to:

TODO: Bug 1436054 - Do not default to percentage unit when inserting new point. https://bugzilla.mozilla.org/show_bug.cgi?id=1436054

::: devtools/server/actors/highlighters/shapes.js:1582
(Diff revision 1)
>            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("%");

It seems unnecessary to get the decimal precision twice for the same value. I would just set:

let precision = getDecimalPrecision("%").

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

Needs to be squashed.

Comment 11

[Razvan Caliman [:rcaliman]]

Comment on attachment 8949785 [details]
Bug 1435368 - Add tests for decimal precision rounding on Shapes editor. Fix broken test. Address comments from previous review.

https://reviewboard.mozilla.org/r/219090/#review224942

> It seems unnecessary to get the decimal precision twice for the same value. I would just set:
> 
> let precision = getDecimalPrecision("%").

I put precision there so explicity divided on X/Y as a reminder that coordinate pairs could have different unit types. But that will happen only with the implementation of the functionality to infer new coordinate units instead of defaulting to percentages. Since that doesn't exist yet, I'm ok with simplifying for now.

Comment 12

[Razvan Caliman [:rcaliman]]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218130/diff/2-3/

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

https://reviewboard.mozilla.org/r/218130/#review225294

Comment 14

[Razvan Caliman [:rcaliman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b04049a5de849f073ebb1ecd6a2d009d80546131

Comment 15

[Stefan Hindli [:stefan_hindli]]

I cannot land this patch because it has 3 open issues.

Comment 16

[Patrick Brosset <:pbro>]

Razvan: for info, because this is the first time you do this: when a reviewer adds comments on mozreview, these count as issues. If you address those issues by pushing a new commit, you need to go manually in mozreview and mark them as closed. Otherwise, the commit can't be landed.
Also, note that since you are using mozreview, you don't have to add the checkin-needed flag in bugzilla. You can do this instead:
- push your new commit
- close the issues you addressed
- go in the "automation" menu in mozreview and select "land commits"
even faster!

Comment 17

[Razvan Caliman [:rcaliman]]

Thanks, Patrick! Issues closed. "Land commits" in mozreview isn't available for me yet. Will add "checkin-needed" here again.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

https://reviewboard.mozilla.org/r/218130/#review226026

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/623b184072c9
Implement precision when rounding polygon coordinates on Shapes editor. r=gl,pbro

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/623b184072c9

Comment 21

[Rares Doghi, Desktop QA]

Attachment: EllipseDecimals.png

Hello, This issue still occurs for Ellipse and Circle shapes, is this behavior intended?
Please check the attachment.

Comment 22

[Razvan Caliman [:rcaliman]]

No, this is not intended. The bug was originally fixed just for polygons. Reopening.

Comment 23

[Razvan Caliman [:rcaliman]]

I closed back this bug and opened a new one instead, Bug 1457206, because this one was already tracked as fixed for Firefox 60.

Comment 24

[Rares Doghi, Desktop QA]

Hi everyone, i have retested this issue on Windows 10 , Windows 7 , Mac OSx and Linux using the latest version of nightly 62.0a1 (2018-05-29) and i can Confirm it as Fixed.
I will mark it accordingly.