Closed Bug 1390364 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

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.
No longer depends on: 1390357
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?
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Try seems to think that just disallowing interpolating/adding none → none is ok and that matches what Gecko does.
Comment on attachment 8897726 [details]
Bug 1390364 - Don't allow interpolating 'fill:none' with 'fill:none';

https://reviewboard.mozilla.org/r/169034/#review174336
Attachment #8897726 - Flags: review?(hikezoe) → review+
Attached file Servo PR #18103
https://hg.mozilla.org/mozilla-central/rev/cf750e1140c5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
No longer blocks: 1396483
Depends on: 1396483
You need to log in before you can comment on or make changes to this bug.