1390364 - stylo: Some SMIL mochitests fail due to getting a computed value of "none" instead of "rgb(0, 0, 0)" for fill

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

e.g. in dom/smil/test/test_smilCSSFromBy.xhtml I get:

TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "none", expected "rgb(0, 0, 0)"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "none", expected "rgb(0, 0, 0)"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "none", expected "rgb(0, 0, 0)"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "none", expected "rgb(0, 0, 0)"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: shouldn't be affected by animation (at animation end) - testcase specified to have no effect - got "none", expected "rgb(0, 0, 0)"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: shouldn't be affected by animation (after animation end) - testcase specified to have no effect - got "none", expected "rgb(0, 0, 0)"

It looks like when we don't have an animation applied getComputedStyle(elem).fill is returning 'none' instead of 'rgb(0, 0, 0)'? I'm not sure which is correct.

Comment 1

[Brian Birtles (:birtles)]

This appears to be failing on the following test:

    // The "none" keyword & URI values aren't addiditve, so the animations in
    // these testcases are expected to have no effect.
    new AnimTestcaseFromBy("none", "none",  { noEffect: 1 }),

because we successfully add "none" and "none" in animated_properties:

  impl Animatable for IntermediateSVGPaintKind {
      fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) -> Result<Self, ()> {
          match (self, other) {
            (&SVGPaintKind::Color(ref self_color), &SVGPaintKind::Color(ref other_color)) => {
                ...
            }
            (&SVGPaintKind::None, &SVGPaintKind::None) => Ok(SVGPaintKind::None),
            ...

[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/servo/components/style/properties/helpers/animated_properties.mako.rs#2745

It seems reasonable to be able to interpolate 'none' and 'none' add them too so I'm a little reluctant to drop the branch. It would also suggest we should do the same for the context-stroke / context-fill values. Perhaps we should just check for different behavior in Stylo?

Comment 2

[Brian Birtles (:birtles)]

Try seems to think that just disallowing interpolating/adding none → none is ok and that matches what Gecko does.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Bug 1390364 - Don't allow interpolating 'fill:none' with 'fill:none';

In SMIL we don't expect the 'none' value of the 'fill' property to be additive
and hence the following animation should have no effect:

  <rect width="100" height="100" y="100" fill="blue">
    <animate attributeName="fill" dur="3s" from="red" by="none"/>
  </rect>

Although SMIL doesn't make this entirely clear, [1] says that "by animation" and
"from-by animation" may only be used "with attributes that support addition
(e.g.  most numeric attributes)" and [2] says that <paint>s are "only additive
if each value can be converted to an RGB color". As a result, the animation
above should have no effect.

By extrapolation, animating from 'none' by 'none' should also have no effect:

  <rect width="100" height="100" y="100" fill="blue">
    <animate attributeName="fill" dur="3s" from="none" by="none"/>
  </rect>

However, in Servo's interpolation of <paint>s we special case the interpolation
and addition of 'none' such that if both values are 'none' it is allowed.

We should disallow this in order to produce the expected behavior and in order
to match Gecko's behavior.

[1] https://www.w3.org/TR/smil-animation/#AnimFuncValues
[2] https://www.w3.org/TR/SVG11/animate.html#AnimationAttributesAndProperties

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8897726 [details]
Bug 1390364 - Don't allow interpolating 'fill:none' with 'fill:none';

https://reviewboard.mozilla.org/r/169034/#review174336

Comment 5

[Brian Birtles (:birtles)]

Attachment: Servo PR #18103

Comment 6

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/mozilla-central/rev/cf750e1140c5