getComputedStyle returns wrong value for basic-shapes containing calc() that should resolve to negative value

RESOLVED FIXED in Firefox 58

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: mparkms, Assigned: jeremychen)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

8 months ago
Created attachment 8883560 [details]
shapes-negative-value.html

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 months ago
Priority: -- → P3
Created attachment 8912418 [details]
testcase 2

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().
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.
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
status-firefox58: --- → affected
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

5 months 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)
I've discussed with Louis offline. So, steal this from him since he's busy at the moment.
Assignee: lochang → jeremychen
Status: NEW → ASSIGNED
Attachment #8918752 - Flags: review?(tlin)
Attachment #8918753 - Flags: review?(tlin)
Attachment #8918754 - Flags: review?(tlin)
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 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 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

4 months 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

4 months 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)

Comment 16

4 months 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
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
Last Resolved: 4 months ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.