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

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

(Blocks: 2 bugs, {regression})

Trunk
mozilla57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 months ago
+++ 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.
Regressed by https://github.com/servo/servo/pull/18239
Blocks: 1390364
No longer depends on: 1390364
Keywords: regression
Blocks: 1302948, 1320841
Comment hidden (mozreview-request)
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED

Comment 3

3 months ago
mozreview-review
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.
Attachment #8904114 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 4

3 months ago
Created attachment 8904155 [details] [review]
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.
Attachment #8904114 - Attachment is obsolete: true
https://hg.mozilla.org/integration/autoland/rev/cc1dd06b7688

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

Comment 6

3 months ago
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=35bd47b6e5ac
Merged.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
status-firefox57: affected → fixed
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.