1390357 - stylo: Some SMIL mochitests fail due to differences between Servo and Gecko clamping of colors

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

e.g. in dom/smil/test/test_smilCSSFromBy.xhtml we run the following test from db_smilCSSFromBy.js:

    // Note: technically, the "from" and "by" values in the test case below
    // would overflow the maxium color-channel values when added together.
    // (e.g. for red [ignoring alpha for now], 100 + 240 = 340 which is > 255)
    // The SVG Animation spec says we should clamp color values "as late as
    // possible," i.e. allow the channel overflow and clamp at paint-time.
    // But for now, we instead clamp the implicit "to" value for the animation
    // and interpolate up to that clamped result.
    new AnimTestcaseFromBy("rgba(100, 100, 100, 0.6)", "rgba(240, 240, 240, 1)",
                             // (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(255, 255, 255) * 1.0 * 0.5) * (1 / 0.8)
                           { midComp: "rgba(197, 197, 197, 0.8)",
                             // (rgb(100, 100, 100) * 0.6 + rgb(240, 240, 240) is overflowed
                             toComp:  "rgb(255, 255, 255)"}),


Which, in Stylo fails as follows:

  INFO TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: checking value halfway through animation - got "rgba(225, 225, 225, 0.8)", expected "rgba(197, 197, 197, 0.8)"

i.e. it seems Stylo is correctly clamping as late as possible.

I suggest the test needs to be updated to either switch between the different behaviors (preferred) or just test the Servo behavior and skip the test in Gecko (if necessary).

This would be a P4 since it is a test-only change that does not need to block shipping but since it prevents us from turning on certain mochitests we should try to fix this before shipping so marking as P3 for now.

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1390357 - Switch test result for clamping RGBA on stylo and gecko by using preference.

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

Comment 2

[Brian Birtles (:birtles)]

Comment on attachment 8897303 [details]
Bug 1390357 - Switch test result for clamping RGBA on stylo and gecko by using preference.

https://reviewboard.mozilla.org/r/168608/#review173848

Thanks for taking this. I'd like to have one last check after the following changes, however.

::: dom/smil/test/db_smilCSSFromBy.js:27
(Diff revision 1)
>      // Note: technically, the "from" and "by" values in the test case below
>      // would overflow the maxium color-channel values when added together.
>      // (e.g. for red [ignoring alpha for now], 100 + 240 = 340 which is > 255)
>      // The SVG Animation spec says we should clamp color values "as late as
>      // possible," i.e. allow the channel overflow and clamp at paint-time.
>      // But for now, we instead clamp the implicit "to" value for the animation
>      // and interpolate up to that clamped result.
> +    // Servo will clamp this value `as late as possible`.
> +    // The middle value is as follow:
> +    //   Gecko: (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(255, 255, 255) * 1.0 * 0.5) * (1 / 0.8)
> +    //   Servo: (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(300, 300, 300) * 1.0 * 0.5) * (1 / 0.8)
> +    // The to value is overflowed on both of gecko and servo.
>      new AnimTestcaseFromBy("rgba(100, 100, 100, 0.6)", "rgba(240, 240, 240, 1)",
> -                             // (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(255, 255, 255) * 1.0 * 0.5) * (1 / 0.8)
> +                           !isServoEnabled() ?
> -                           { midComp: "rgba(197, 197, 197, 0.8)",
> +                             { midComp: "rgba(197, 197, 197, 0.8)",
> -                             // (rgb(100, 100, 100) * 0.6 + rgb(240, 240, 240) is overflowed
> +                               toComp:  "rgb(255, 255, 255)"}
> +                           : { midComp: "rgba(225, 225, 225, 0.8)",
> -                             toComp:  "rgb(255, 255, 255)"}),
> +                               toComp:  "rgb(255, 255, 255)"}),

I think this comment needs rewriting. Something like:

    // The "from" and "by" values in the test case below overflow the maxium
    // color-channel values when added together.
    // (e.g. for red [ignoring alpha for now], 100 + 240 = 340 which is > 255)
    // The SVG Animation spec says we should clamp color values "as late as
    // possible," i.e. allow the channel overflow and clamp at paint-time.
    //
    // Servo does this, and gives us:
    //
    //   to-value = (rgb(100, 100, 100) * 0.6 + rgb(240, 240, 240) * 1.0)) * 1
    //            = rgb(300, 300, 300)
    //   midComp  = (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(300, 300, 300) * 1.0 * 0.5) * (1 / 0.8)
    //            = rgb(225, 225, 225)
    //
    // Gecko, however, clamps the "to" value and interpolates up to that
    // clamped result giving:
    //
    //   midComp  = (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(255, 255, 255) * 1.0 * 0.5) * (1 / 0.8)
    //            = rgb(197, 197, 197)
    //
    new AnimTestcaseFromBy("rgba(100, 100, 100, 0.6)", "rgba(240, 240, 240, 1)",
                           { midComp:
                             isServo ? "rgba(225, 225, 225, 0.8)"
                                     : "rgba(197, 197, 197, 0.8)",
                             toComp:  "rgb(255, 255, 255)" }),

::: dom/smil/test/db_smilCSSFromBy.js:40
(Diff revision 1)
> +    // The middle value is as follow:
> +    //   Gecko: (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(255, 255, 255) * 1.0 * 0.5) * (1 / 0.8)
> +    //   Servo: (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(300, 300, 300) * 1.0 * 0.5) * (1 / 0.8)
> +    // The to value is overflowed on both of gecko and servo.
>      new AnimTestcaseFromBy("rgba(100, 100, 100, 0.6)", "rgba(240, 240, 240, 1)",
> -                             // (rgb(100, 100, 100) * 0.6 * 0.5 + rgb(255, 255, 255) * 1.0 * 0.5) * (1 / 0.8)
> +                           !isServoEnabled() ?

In other tests I think we've been calling this function once and then storing the result in a const variable

e.g.

  http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/layout/style/test/test_keyframes_vendor_prefix.html#65
  http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/layout/style/test/test_transitions_per_property.html#62
  http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/layout/style/test/animation_utils.js#415

Also, the ? operator goes at the start of the line.

::: dom/smil/test/smilTestUtils.js:859
(Diff revision 1)
> +
> +// Return servo feature is enabled or not.
> +function isServoEnabled() {
> +  return SpecialPowers.getBoolPref("layout.css.servo.enabled");
> +}

We just should use SpecialPowers.DOMWindowUtils.isStyledByServo

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8897303 [details]
Bug 1390357 - Switch test result for clamping RGBA on stylo and gecko by using preference.

https://reviewboard.mozilla.org/r/168608/#review173848

Thank you for reviewing this.
I've addressed it.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8897303 [details]
Bug 1390357 - Switch test result for clamping RGBA on stylo and gecko by using preference.

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

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8897303 [details]
Bug 1390357 - Switch test result for clamping RGBA on stylo and gecko by using preference.

https://reviewboard.mozilla.org/r/168608/#review174260

Great! Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2194847315b1
Switch test result for clamping RGBA on stylo and gecko by using preference. r=birtles

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2194847315b1