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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Mike Park [:mparkms]]

Attachment: 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)"

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: 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().

Comment 2

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

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.)

Comment 4

[Louis Chang [:louis]]

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.

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: Bug 1378368 - part1: do not always clamp negative calc values in SetCssTextToCoord.

nsComputedDOMStyle::SetCssTextToCoord() is a helper function for computing and
serializing a nsStyleCoord. In the current implementation, SetCssTextToCoord
implicitly clamp negative calc values, which is pretty non-trivial.

In this patch, we expose an extra aClampNegativeCalc parameter for SetCssTextToCoord,
so the callers can explicitly set the clamping mode as needed.

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

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1378368 - part2: do not clamp negative calc values while serializing basic-shape polygon.

According to the spec, negative values are valid for polygon(), so we should not
clamp it while computing and serializing negative calc values for polygon().

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

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 1378368 - part3: add tests.

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

Comment 8

[Jeremy Chen ([e-mail])]

I've discussed with Louis offline. So, steal this from him since he's busy at the moment.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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) {`

Comment 12

[Jeremy Chen ([e-mail])]

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.

Comment 13

[Jeremy Chen ([e-mail])]

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 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8918754 [details]
Bug 1378368 - part3: add tests.

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

Comment 15

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9264e430b6264d9f4ce94f1a2d5fbc5dedcaeda5

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

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

https://hg.mozilla.org/mozilla-central/rev/6dd0a3dd4494
https://hg.mozilla.org/mozilla-central/rev/32fa3e870f7b
https://hg.mozilla.org/mozilla-central/rev/985aaff8c657