Closed Bug 1453428 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

(firefox61 verified, firefox62 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: rcaliman, Assigned: rcaliman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
See Also: → 1452985
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
Attachment #8967318 - Flags: review?(gl) → review+
Keywords: checkin-needed
Priority: -- → P1
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.
Flags: needinfo?(gl)
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
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
Keywords: checkin-needed
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.
Flags: needinfo?(rcaliman)
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
Attachment #8967318 - Attachment is obsolete: true
Flags: needinfo?(rcaliman)
Attachment #8968632 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b4a808050676
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
QA Whiteboard: [good first verify]
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]
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.