Closed
Bug 1378368
Opened 7 years ago
Closed 7 years ago
getComputedStyle returns wrong value for basic-shapes containing calc() that should resolve to negative value
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: mpark, Assigned: chenpighead)
Details
Attachments
(5 files)
There appears to be a bug where calc()s in basic-shapes (e.g. polygon(), circle()) that should resolve to a negative pixel value are instead always resolved to 0px. User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:56.0) Gecko/20100101 Firefox/56.0 1. Open attached test case. 2. In console, run getComputedStyle(document.getElementById("shape")).clipPath Expected: "polygon(-10px 0px, 100px 100px, 0px 100px)" Actual: "polygon(0px 0px, 100px 100px, 0px 100px)"
Updated•7 years ago
|
Priority: -- → P3
Comment 1•7 years ago
|
||
Here's a testcase with an alert() so that you don't have to open the console. This also demonstrates that we do accept *explicit* negative values inside of polygon() -- and hence, it makes sense that we should accept negative results of calc().
Comment 2•7 years ago
|
||
Doesn't seem critical enough to take the risk of a code-change to fix this in 57 (current beta) or 56 (impending release), but it'd be nice to get this fixed.
Comment 3•7 years ago
|
||
lochang, looks like you've worked on this a bit -- perhaps you'd be up for taking this, or you know who might be interested? (It's also possible that the code to be fixed on Trunk here would be Rust code from Stylo, rather than C++ nsCSSParser-and-friends code. Not sure where this calc() zero-clamping is happening.)
Flags: needinfo?(lochang)
Comment 4•7 years ago
|
||
Ok, I will try look into it. But I might not have time recently. If someone is interested in the bug, feel free to take it.
Assignee: nobody → lochang
Flags: needinfo?(lochang)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
I've discussed with Louis offline. So, steal this from him since he's busy at the moment.
Assignee: lochang → jeremychen
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Attachment #8918752 -
Flags: review?(tlin)
Attachment #8918753 -
Flags: review?(tlin)
Attachment #8918754 -
Flags: review?(tlin)
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8918752 [details] Bug 1378368 - part1: do not always clamp negative calc values in SetCssTextToCoord. https://reviewboard.mozilla.org/r/189586/#review194776 ::: layout/style/nsComputedDOMStyle.cpp:6421 (Diff revision 1) > position->GetCssText(positionString); > shapeFunctionString.Append(positionString); > break; > } > case StyleBasicShapeType::Inset: { > BoxValuesToString(shapeFunctionString, aStyleBasicShape->Coordinates()); Negative values for `inset` like `inset(calc(10px - 20px))` should be valid. Would you file a follow-up, please? The fix should be similar to this bug, but we need to add extra argument for `BoxValuesToString` and pass `false` here.
Attachment #8918752 -
Flags: review?(tlin) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8918753 [details] Bug 1378368 - part2: do not clamp negative calc values while serializing basic-shape polygon. https://reviewboard.mozilla.org/r/189588/#review194778
Attachment #8918753 -
Flags: review?(tlin) → review+
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8918754 [details] Bug 1378368 - part3: add tests. https://reviewboard.mozilla.org/r/189590/#review194786 ::: layout/style/test/test_computed_style.html:722 (Diff revision 1) > + for (var i = 0; i < clipPaths.length; ++i) { > + var test = clipPaths[i]; Nit: This can be `for (let test of clipPaths) {`
Attachment #8918754 -
Flags: review?(tlin) → review+
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8918752 [details] Bug 1378368 - part1: do not always clamp negative calc values in SetCssTextToCoord. https://reviewboard.mozilla.org/r/189586/#review194776 > Negative values for `inset` like `inset(calc(10px - 20px))` should be valid. Would you file a follow-up, please? > > The fix should be similar to this bug, but we need to add extra argument for `BoxValuesToString` and pass `false` here. No problem. Filed Bug 1408851 for this.
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8918754 [details] Bug 1378368 - part3: add tests. https://reviewboard.mozilla.org/r/189590/#review194786 > Nit: This can be `for (let test of clipPaths) {` Thank you for the review. :)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9264e430b6264d9f4ce94f1a2d5fbc5dedcaeda5
Comment 16•7 years ago
|
||
Pushed by jichen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6dd0a3dd4494 part1: do not always clamp negative calc values in SetCssTextToCoord. r=TYLin https://hg.mozilla.org/integration/autoland/rev/32fa3e870f7b part2: do not clamp negative calc values while serializing basic-shape polygon. r=TYLin https://hg.mozilla.org/integration/autoland/rev/985aaff8c657 part3: add tests. r=TYLin
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6dd0a3dd4494 https://hg.mozilla.org/mozilla-central/rev/32fa3e870f7b https://hg.mozilla.org/mozilla-central/rev/985aaff8c657
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•