1408851 - negative calc()s for inset() should be valid

Status: RESOLVED FIXED

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

Description

[Jeremy Chen ([e-mail])]

Per bug 1378368 comment 9.
> ::: 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 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1408851 - part1: do not always clamp negative calc values in BoxValuesToString.

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

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

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

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: Bug 1408851 - part2: do not always clamp negative calc values while serializing basic-shape inset.

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

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

Comment 3

[Jeremy Chen ([e-mail])]

Attachment: Bug 1408851 - part3: add test.

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

Comment 4

[Jeremy Chen ([e-mail])]

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

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8918793 [details]
Bug 1408851 - part1: do not always clamp negative calc values in BoxValuesToString.

https://reviewboard.mozilla.org/r/189640/#review195232

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8918794 [details]
Bug 1408851 - part2: do not always clamp negative calc values while serializing basic-shape inset.

https://reviewboard.mozilla.org/r/189642/#review195234

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8918795 [details]
Bug 1408851 - part3: add test.

https://reviewboard.mozilla.org/r/189644/#review195238

Thanks for doing this!

Comment 8

[Jeremy Chen ([e-mail])]

Comment on attachment 8918795 [details]
Bug 1408851 - part3: add test.

https://reviewboard.mozilla.org/r/189644/#review195238

Thanks for the review! :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60d9e8195020
part1: do not always clamp negative calc values in BoxValuesToString. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/3eeaa611682e
part2: do not always clamp negative calc values while serializing basic-shape inset. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/637c31729e35
part3: add test. r=TYLin

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/60d9e8195020
https://hg.mozilla.org/mozilla-central/rev/3eeaa611682e
https://hg.mozilla.org/mozilla-central/rev/637c31729e35