1396483 - 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, defect, P3)

Description

[Mantaroh Yoshinaga[:mantaroh]]

+++ This bug was initially created as a clone of Bug #1390364 +++

The bug 1390364 fixed the interpolating of 'fill:none' with 'fill:none' values. But current Servo failed this interpolation tests:

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)"
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)"
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)"
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)"
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)"
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)"
FAIL | dom/smil/test/test_smilMappedAttrFromBy.xhtml | fill: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect got "none", expected "rgb(0, 0, 0)"
FAIL | dom/smil/test/test_smilMappedAttrFromBy.xhtml | fill: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect got "none", expected "rgb(0, 0, 0)"
FAIL | dom/smil/test/test_smilMappedAttrFromBy.xhtml | fill: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect got "none", expected "rgb(0, 0, 0)"
FAIL | dom/smil/test/test_smilMappedAttrFromBy.xhtml | fill: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect got "none", expected "rgb(0, 0, 0)"
FAIL | dom/smil/test/test_smilMappedAttrFromBy.xhtml | fill: shouldn't be affected by animation (at animation end) - testcase specified to have no effect got "none", expected "rgb(0, 0, 0)"
FAIL | dom/smil/test/test_smilMappedAttrFromBy.xhtml | fill: shouldn't be affected by animation (after animation end) - testcase specified to have no effect got "none", expected "rgb(0, 0, 0)"

https://treeherder.mozilla.org/logviewer.html#?job_id=128167454&repo=try

Servo uses Animate derive for SVGPaintKind, but there aren't animation(error) hint for SVGPaintKind::None value.

Comment 1

[Brian Birtles (:birtles)]

Regressed by https://github.com/servo/servo/pull/18239

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1396483 - Don't allow interpolating 'fill:none' with 'fill:none'.

Bug 1390364[1] fixed this type interpolating, but the recently changes[2]
uses Animate derive for this type(SVGPaintKind), then Servo can interpolate
this types(i.e. fill:none and fill:none).

This patch add animation(error) hint to SVGPaintKind::None in order to
disallow this interpolation.

[1] https://hg.mozilla.org/mozilla-central/rev/cf750e1140c5
[2] https://hg.mozilla.org/mozilla-central/rev/fad84f0b6567#l16.33

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

Comment 3

[Brian Birtles (:birtles)]

Comment on attachment 8904114 [details]
Bug 1396483 - Don't allow interpolating 'fill:none' with 'fill:none'.

https://reviewboard.mozilla.org/r/175894/#review180872

::: commit-message-8e052:3
(Diff revision 1)
> +Bug 1390364[1] fixed this type interpolating, but the recently changes[2]
> +uses Animate derive for this type(SVGPaintKind), then Servo can interpolate
> +this types(i.e. fill:none and fill:none).
> +
> +This patch add animation(error) hint to SVGPaintKind::None in order to
> +disallow this interpolation.
> +
> +[1] https://hg.mozilla.org/mozilla-central/rev/cf750e1140c5
> +[2] https://hg.mozilla.org/mozilla-central/rev/fad84f0b6567#l16.33

Since this patch is going to land in Servo, let's update the comment to reference the relevant Servo PRs as follows:


#18103 disallowed interpolation between fill:none and fill:none, but that change was regressed by the refactoring in #18239.

This patch restores the intended behavior, disabling animation of SVGPaintKind::None.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Servo PR, #18364

(In reply to Brian Birtles (:birtles) from comment #3)
> Comment on attachment 8904114 [details]
> Bug 1396483 - Don't allow interpolating 'fill:none' with 'fill:none'.
> 
> https://reviewboard.mozilla.org/r/175894/#review180872
> 
> ::: commit-message-8e052:3
> (Diff revision 1)
> > +Bug 1390364[1] fixed this type interpolating, but the recently changes[2]
> > +uses Animate derive for this type(SVGPaintKind), then Servo can interpolate
> > +this types(i.e. fill:none and fill:none).
> > +
> > +This patch add animation(error) hint to SVGPaintKind::None in order to
> > +disallow this interpolation.
> > +
> > +[1] https://hg.mozilla.org/mozilla-central/rev/cf750e1140c5
> > +[2] https://hg.mozilla.org/mozilla-central/rev/fad84f0b6567#l16.33
> 
> Since this patch is going to land in Servo, let's update the comment to
> reference the relevant Servo PRs as follows:
> 
> 
> #18103 disallowed interpolation between fill:none and fill:none, but that
> change was regressed by the refactoring in #18239.
> 
> This patch restores the intended behavior, disabling animation of
> SVGPaintKind::None.

Thanks Brian.

I fixed these comments and created PR.

Comment 5

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/autoland/rev/cc1dd06b7688

Looks like this hasn't been merged to m-c yet, however.

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=35bd47b6e5ac
Merged.