1369277 - stylo: Make fill, stroke animatable

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

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

Comment 1

[Brian Birtles (:birtles)]

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.

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1369277 - Part 1: stylo: Make SVGPaint and SVGPaintKind generic;

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1369277 - Part 2: stylo: Make SVGPaint and SVGPaintKind animatable;

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

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1369277 - Part 3: stylo: Animate fill and stroke;

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

Comment 6

[Manish Goregaokar [:manishearth]]

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 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874659 [details]
Bug 1369277 - Part 3: stylo: Animate fill and stroke;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146010/diff/1-2/

Comment 8

[Brian Birtles (:birtles)]

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.)

Comment 9

[Brian Birtles (:birtles)]

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.

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874658 [details]
Bug 1369277 - Part 2: stylo: Make SVGPaint and SVGPaintKind animatable;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146008/diff/1-2/

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874659 [details]
Bug 1369277 - Part 3: stylo: Animate fill and stroke;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146010/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a36d3b0c90df
Update test expectations. r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50a086be7e63
More reftest expectations. r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5bd43668caa
Even more reftest updates. r=me

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f94e7abdc7d5
More reftest expectation updates; r=orange

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a36d3b0c90df
https://hg.mozilla.org/mozilla-central/rev/50a086be7e63
https://hg.mozilla.org/mozilla-central/rev/e5bd43668caa
https://hg.mozilla.org/mozilla-central/rev/f94e7abdc7d5