Closed Bug 1369277 Opened 4 years ago Closed 3 years ago

stylo: Make fill, stroke animatable

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: birtles, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

+++ This bug was initially created as a clone of Bug #1360133 +++
FWIW, in Gecko this has animation type eStyleAnimType_PaintServer which basically is a union of color animation, URL + fallback color (discrete), context-fill/context-stroke animation, none.
Blocks: stylo-smil
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Comment on attachment 8874639 [details]
Bug 1369277 - Part 1: stylo: Make SVGPaint and SVGPaintKind generic;

https://reviewboard.mozilla.org/r/145980/#review149884

::: servo/components/style/values/computed/mod.rs:543
(Diff revision 1)
> -}
> -
>  impl Default for SVGPaint {
>      fn default() -> Self {
>          SVGPaint {
> -            kind: SVGPaintKind::None,
> +            kind: ::values::generics::SVGPaintKind::None,

Since you have `SVGPaintKind` aliased here, couldn't you just use that?

::: servo/components/style/values/computed/mod.rs:554
(Diff revision 1)
>  impl SVGPaint {
>      /// Opaque black color
>      pub fn black() -> Self {
>          let rgba = RGBA::from_floats(0., 0., 0., 1.);
>          SVGPaint {
> -            kind: SVGPaintKind::Color(CSSColor::RGBA(rgba)),
> +            kind: ::values::generics::SVGPaintKind::Color(CSSColor::RGBA(rgba)),

Ditto.
Attachment #8874639 - Flags: review?(xidorn+moz) → review+
First patch will land early on the servo side in https://github.com/servo/servo/pull/17177 so that Xidorn doesn't have to rebase too much.

I'm unsure if I did the distance thing correctly. For the clone impl I didn't add any handling for the URL type yet, though we can land this with a followup for that for when bug 1369277 gets to that point.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31d622a66022e753a3ba0d932f889b85a4c29644
Comment on attachment 8874658 [details]
Bug 1369277 - Part 2: stylo: Make SVGPaint and SVGPaintKind animatable;

https://reviewboard.mozilla.org/r/146008/#review149984

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2887
(Diff revision 1)
> +        Ok(self.kind.compute_squared_distance(&other.kind)? +
> +            self.fallback.compute_squared_distance(&other.fallback)?)

Is this the intended indentation here? Seems like it is 1 or 3 off.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2896
(Diff revision 1)
> +            (&SVGPaintKind::Color(ref self_color), &SVGPaintKind::Color(ref other_color)) => {
> +                Ok(SVGPaintKind::Color(self_color.add_weighted(other_color, self_portion, other_portion)?))
> +            }
> +            (&SVGPaintKind::None, &SVGPaintKind::None) => Ok(SVGPaintKind::None),
> +            (&SVGPaintKind::ContextFill, &SVGPaintKind::ContextFill) => Ok(SVGPaintKind::ContextFill),
> +            (&SVGPaintKind::ContextStroke, &SVGPaintKind::ContextStroke) => Ok(SVGPaintKind::ContextStroke),

Ideally we should be able to interpolate between context-fill/context-stroke and a color. I guess Gecko doesn't currently support this though? Perhaps leave a FIXME comment for adding this?

(And as for how we'll implement this, I wonder if it should use the same class as current color.)
Attachment #8874658 - Flags: review?(bbirtles) → review+
Comment on attachment 8874659 [details]
Bug 1369277 - Part 3: stylo: Animate fill and stroke;

https://reviewboard.mozilla.org/r/146010/#review149980

::: servo/components/style/properties/gecko.mako.rs:481
(Diff revision 2)
> +                // TODO
> +                SVGPaintKind::None

Can you please leave a comment in bug 1368610 mentioning we need to update this when we implement discrete animation of URL values. Thanks.
Attachment #8874659 - Flags: review?(bbirtles) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f94e7abdc7d5
More reftest expectation updates; r=orange
You need to log in before you can comment on or make changes to this bug.