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

VERIFIED FIXED in Firefox 60

Status

P2
enhancement
VERIFIED FIXED
a year ago
7 months ago

People

(Reporter: pbro, Assigned: rcaliman)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 60

Firefox Tracking Flags

(firefox60 verified, firefox61 verified, firefox62 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8947956 [details]
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.
(Reporter)

Updated

a year ago
Severity: normal → enhancement
Priority: -- → P2
(Reporter)

Comment 1

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → rcaliman
(Reporter)

Updated

a year ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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

a year ago
mozreview-review
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.
Attachment #8948738 - Flags: review?(gl)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
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]
These 2 commits should really be squashed into one.

Comment 9

11 months ago
mozreview-review
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("%").
Attachment #8949785 - Flags: review?(gl)
Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

Needs to be squashed.
Attachment #8948738 - Flags: review?(gl)
(Assignee)

Comment 11

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8949785 - Attachment is obsolete: true

Comment 13

11 months ago
mozreview-review
Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

https://reviewboard.mozilla.org/r/218130/#review225294
Attachment #8948738 - Flags: review?(gl) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
I cannot land this patch because it has 3 open issues.
Flags: needinfo?(rcaliman)
Keywords: checkin-needed
(Reporter)

Comment 16

11 months ago
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!
(Assignee)

Comment 17

11 months ago
Thanks, Patrick! Issues closed. "Land commits" in mozreview isn't available for me yet. Will add "checkin-needed" here again.
Flags: needinfo?(rcaliman)
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Reporter)

Comment 18

11 months ago
mozreview-review
Comment on attachment 8948738 [details]
Bug 1435368 - Implement precision when rounding polygon coordinates on Shapes editor.

https://reviewboard.mozilla.org/r/218130/#review226026
Attachment #8948738 - Flags: review+
(Reporter)

Updated

11 months ago
Keywords: checkin-needed

Comment 19

11 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/623b184072c9
Implement precision when rounding polygon coordinates on Shapes editor. r=gl,pbro

Comment 20

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/623b184072c9
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Comment 21

9 months ago
Created attachment 8969589 [details]
EllipseDecimals.png

Hello, This issue still occurs for Ellipse and Circle shapes, is this behavior intended?
Please check the attachment.
Flags: needinfo?(rcaliman)
(Assignee)

Comment 22

9 months ago
No, this is not intended. The bug was originally fixed just for polygons. Reopening.
Status: RESOLVED → REOPENED
Flags: needinfo?(rcaliman)
Resolution: FIXED → ---
(Assignee)

Updated

9 months ago
Target Milestone: Firefox 60 → Firefox 61
(Assignee)

Updated

9 months ago
See Also: → bug 1457206
(Assignee)

Comment 23

9 months ago
I closed back this bug and opened a new one instead, Bug 1457206, because this one was already tracked as fixed for Firefox 60.
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago9 months ago
Resolution: --- → FIXED
Target Milestone: Firefox 61 → Firefox 60

Comment 24

8 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: --- → verified
status-firefox62: --- → verified

Updated

7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.