1453428 - Shape editor: coordinate units are replaced with px when coordinate value is zero.

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[Razvan Caliman [:rcaliman]]

Steps:

- Paste and run this in the URL bar:
data:text/html,<style>div {width:300px;height:300px;background:red;clip-path:polygon(0% 0%, 0% 100%, 100% 100%);}</style><div>

- Open the Inspector and inspect the div.
- Trigger the shapes editor on the "clip-path" property.
- Observe the polygon's first coordinate, 0% 0%, has percentages as units.
- Using the shape editor, drag the first point.

Result:
Coordinate unit types changed to pixels replacing percentages.

Expected:
Values should update, but coordinate unit types should not change. 
For unit-less zero, it's ok to default to pixels.

The same behaviour occurs for other all unit types which are not pixels (%, em, rem, vh, vw). It also happens when editing values of the `shape-outside` CSS property.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1453428 - Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels.

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.

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

Comment 2

[Razvan Caliman [:rcaliman]]

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

Comment 3

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

Comment on attachment 8967318 [details]
Bug 1453428 - Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels.

https://reviewboard.mozilla.org/r/236026/#review242482

::: devtools/server/actors/highlighters/shapes.js:2515
(Diff revision 1)
> +   *
> +   * @param {String} unit
> +   *        One of: %, em, rem, vw, vh
> +   * @param {Number} size
> +   *        Size to which percentage values are relative to.
> +   *

Remove this new line between the @param and @return

Comment 4

[Razvan Caliman [:rcaliman]]

Comment on attachment 8967318 [details]
Bug 1453428 - Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels.

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

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Again the r+ in MozReview has been lost. Razvan, Gabriel, please track down why this always happens for you and how you can prevent or fix that. Thank you. r+ on the left side in MozReview is needed to land the patch.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f95bbb60e43
Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels. r=gl

Comment 7

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 3f95bbb60e43 (bug 1453428) for mochitest devtools failures on browser_inspector_highlighter-cssshape_iframe_01.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/bdbed4e49efb5756f97cb2a913eb96f62495429a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3f95bbb60e4399e9b7533aec0f42e52539f8b235&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=retry&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=superseded&selectedJob=174126579

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174126579&repo=mozilla-inbound&lineNumber=1857

08:52:25     INFO - TEST-START | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js
08:52:27     INFO - TEST-INFO | started process screencapture
08:52:27     INFO - TEST-INFO | screencapture: exit 0
08:52:27     INFO - Buffered messages logged at 08:52:25
08:52:27     INFO - Entering test bound 
08:52:27     INFO - Adding a new tab with URL: http://example.com/browser/devtools/client/inspector/test/doc_inspector_highlighter_cssshapes_iframe.html
08:52:27     INFO - Buffered messages logged at 08:52:26
08:52:27     INFO - Tab added and finished loading
08:52:27     INFO - Opening the inspector
08:52:27     INFO - Opening the toolbox
08:52:27     INFO - Toolbox opened and focused
08:52:27     INFO - Turn on shapes highlighter for #polygon
08:52:27     INFO - Selecting the node for '[Front for domnode/server1.conn36.child1/domnode43]'
08:52:27     INFO - Toggle shapes highlighter on for clip-path on #polygon
08:52:27     INFO - Buffered messages logged at 08:52:27
08:52:27     INFO - Moving polygon point visible in iframe
08:52:27     INFO - Buffered messages finished
08:52:27     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js | Point moved to 10px 1.25% - 
08:52:27     INFO - Stack trace:
08:52:27     INFO - chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js:testPolygonIframeMovePoint:47
08:52:27     INFO - Moving polygon point not visible in iframe
08:52:27     INFO - TEST-PASS | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js | Point moved to 110px 51.25% - 
08:52:27     INFO - Turn off shapes highlighter for #polygon
08:52:27     INFO - Toggle shapes highlighter off for clip-path on #polygon
08:52:27     INFO - Leaving test bound 
08:52:27     INFO - Removing tab.

Comment 8

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1453428 - Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels.

Forwarding r+ from previous patch.

Strange if that mochitest failed like that. The patch introduces a fix for that exact issue. 

I rebased my changes, ran mochitests (ok), and pushed to try (green). Attaching patch for check-in again.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a808050676
Handle cases for shape coordinates like 0%, 0em, 0rem, etc. without overwriting to pixels. r=gl

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b4a808050676

Comment 11

[Sajedul Islam]

I have reproduced this bug with Nightly 61.0a1 (2018-04-11) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Beta !

Build   ID    20180517141400
User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

[bugday-20180516]

Comment 12

[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-25) and i can Confirm it as Fixed.
I will mark it accordingly.