Closed Bug 1362897 Opened 7 years ago Closed 7 years ago

stylo: Make filter property animatable

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: mantaroh)

References

(
URL
)

Details

Attachments

(3 files, 4 obsolete files)

Bug 1362897 - Add w-p-t of interpolating filter property.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1362897 - Enable w-p-t of filter's animation except accumulate test.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #17288
41 bytes, text/x-github-pull-request
Details | Review 
      No description provided.
This is rough implementation for Filters. It passed almost simple test case.[1]

[1] https://people-mozilla.org/~myoshinaga/filter/sample.html

I will need to implement the following item:
 * Interpolation for SpecifiedUrl.
 * Cloning the SpecifiedUrl and DropShadow.
 * Calculating the distance of Filter.
 * Tests of interpolating the filter.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> This is rough implementation for Filters. It passed almost simple test
> case.[1]
>

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6f9872036ad5ef9b6cd276aa58d20bf9abe97da
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> This is rough implementation for Filters. It passed almost simple test
> case.[1]
> 
> [1] https://people-mozilla.org/~myoshinaga/filter/sample.html
> 
> I will need to implement the following item:
>  * Interpolation for SpecifiedUrl.
>  * Cloning the SpecifiedUrl and DropShadow.
>  * Calculating the distance of Filter.
>  * Tests of interpolating the filter.

You need to implement add(), and will need to implement accumulate() if bug 1353202 landed first.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> > This is rough implementation for Filters. It passed almost simple test
> > case.[1]
> > 
> > [1] https://people-mozilla.org/~myoshinaga/filter/sample.html
> > 
> > I will need to implement the following item:
> >  * Interpolation for SpecifiedUrl.
> >  * Cloning the SpecifiedUrl and DropShadow.
> >  * Calculating the distance of Filter.
> >  * Tests of interpolating the filter.
> 
> You need to implement add(), and will need to implement accumulate() if bug
> 1353202 landed first.

Thanks, I checked spec and current gecko's implementation.
We don't need to interpolate SpecifiedUrl, because the spec clarify that we use discrete interpolation if filter property has filter-list with url[1]. And the gecko does it.

[1] https://drafts.fxtf.org/filters/#interpolation-of-filters

Further we should consider that each function list's length are different in several case. (e.g. we specify keyframe to filter:['blur(1px) grayscale(10%)', 'blur(2px) grayscale(20% blur(10px)']. In this case, we should add blur(0px) item to first function lists. So this keyframe same to ['blur(1px) grayscale(10%) blur(0px)', 'blur(2px) grayscale(20%) blur(10px)'].)
URL: https://drafts.fxtf.org/filters/#inte...
Priority: -- → P1
Summary: Make filter property animatable → stylo: Make filter property animatable
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> > This is rough implementation for Filters. It passed almost simple test
> > case.[1]
> > 
> > [1] https://people-mozilla.org/~myoshinaga/filter/sample.html
> > 
> > I will need to implement the following item:
> >  * Interpolation for SpecifiedUrl.
> >  * Cloning the SpecifiedUrl and DropShadow.
> >  * Calculating the distance of Filter.
> >  * Tests of interpolating the filter.
> 
> You need to implement add(), and will need to implement accumulate() if bug
> 1353202 landed first.

This is rough implementation before implementing additive and accumulate behavior:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=380fe12e179d4a5cbaada6c5c22a062470b69d8e

Leave work is as follow:
 * Implement add() and accumulate() function
 * Implement compute_squared_distance() function
 * Add the additive and accumulate tests.
 * Tidy up the code.
We use 'Shadow' type in longhand::filter::computed_value::T::Filter::DropShadow, it is same to BoxShadow[1]. So we can use BoxShadow's implementation. 
[1] http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/servo/components/style/properties/longhand/effects.mako.rs#48

There are some still work which cloning the SpecifiedUrl from gecko's value.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9617ad77814a5c651798d648302562c99b88e24
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

Sorry, clear the review flag since test fail.

Interpolating the hue-rotate tests are failed on stylo. Stylo return hue-rotate's value as radian unit.

Spec:
https://drafts.fxtf.org/filters/#funcdef-filter-hue-rotate
Attachment #8872932 - Flags: review?(hikezoe)
Comment on attachment 8872933 [details]
Bug 1362897 - Add w-p-t of interpolating filter property.

https://reviewboard.mozilla.org/r/144460/#review148262

I'd like to see test cases without getNumberPercentageXX functions.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1206
(Diff revision 1)
>    },
>  };
>  
>  const filterListType = {
> +  testInterpolation: function(property, setup, options) {
> +

Nit: no blank line is needed.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1214
(Diff revision 1)
> +    }
> +    function getNumberPercentageFunctionZeroInitType(val) {

Nit: a blank line is needed.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1215
(Diff revision 1)
> +      arr.forEach(func => {
> +        retVal.push(func + '(' + initVal + ')');
> +      });
> +      return retVal.join(' ');
> +    }
> +    function getNumberPercentageFunctionZeroInitType(val) {

I don't quite understand what this function name implies. It's for number or percentage?
Also 'init' is defined in the spec?  If not, we should carefully choose the word. 
That's being said, do we really need these functions?
With these functions, each test case looks ambiguous what property exactly tests.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1218
(Diff revision 1)
> +    }
> +    function getNumberPercentageFunctionOneInitType(val) {

Likewise.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1223
(Diff revision 1)
> +    }
> +    function getNumberPercentageFunction(val) {

Likewise.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1237
(Diff revision 1)
> +      var animation = target.animate({ [idlName]: ['blur(20px)',
> +                                                   'blur(50px)'] },
> +                                     { duration: 1000 });
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,    expected: 'blur(35px)' }]);
> +    }, property + ': interpolate length function(blur)' );

testInterpolation is called only from interpolation-per-property.html, so I don't think we need 'interpolate' in the test name.
Also 'length function' looks like length(100px) or something.  I think we can say just 'blur' here.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1256
(Diff revision 1)
> +      var animation = target.animate({ [idlName]: ['hue-rotate(0deg)',
> +                                                   'hue-rotate(100deg)'] },

If we are going to test each function respectively, how about changing its unit? Just like hue-rotate(10deg) -> hue-rotate(100grad).

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1261
(Diff revision 1)
> +      var animation = target.animate({ [idlName]: ['hue-rotate(0deg)',
> +                                                   'hue-rotate(100deg)'] },
> +                                     { duration: 1000 });
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,    expected: 'hue-rotate(50deg)' }]);
> +    }, property + ': interpolate angle function(hue-rotate)' );

Nit: 'hue-rotate'.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1276
(Diff revision 1)
> +
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 500,
> +            expected: 'drop-shadow(rgba(85, 0, 170, 0.6) 35px 35px 35px)' }]);
> +    }, property + ': interpolate box-shadow function(drop-shadow)' );

This message is pretty confusing! It's not box-shadow!

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1314
(Diff revision 1)
> +                     'blur(5px) drop-shadow(rgba(100, 100, 100, 0.5) ' +
> +                     '50px 50px 50px)' }]);

It's not clear to me what the default color value of drop-shadow is.
From the spec;
 The lacuna value for interpolation is all length values set to 0 and the used color is taken from the color property.
 
Is that mean currentColor?
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

https://reviewboard.mozilla.org/r/144458/#review148280

I don't know why we still use Vec for filter property instead of SmallVec though.
Anyway, I think this patch needs more work. I will review again once it's updated.
Ah, also computed distance part should be reviewed by Boris.

::: servo/components/style/gecko/url.rs:51
(Diff revision 1)
> +    pub unsafe fn parse_from_url_value(url: &URLValue)
> +                                       -> Result<SpecifiedUrl, ()> {
> +        Ok(SpecifiedUrl {
> +            serialization: Arc::new((url._base.mString.to_string())),
> +            extra_data:
> +                (*RefPtr::from_ptr_ref(&url._base.mExtraData.mRawPtr)).clone(),
> +            image_value: None,
> +        })
> +    }

URL handling is out of my knowledge, this part should be reviwed by someone familiar with this stuff. Xidorn?

::: servo/components/style/properties/gecko.mako.rs:3449
(Diff revision 1)
> +        <%
> +         FUNCTIONS = [ 'Brightness', 'Contrast', 'Grayscale', 'Invert',
> +                       'Opacity', 'Saturate', 'Sepia' ]
> +         %>

Can't we specify this outside the function so what we can reuse them for clone method as well.  We should name more appropriate one though.

::: servo/components/style/properties/gecko.mako.rs:3544
(Diff revision 1)
> +        <%
> +         FUNCTIONS = [ 'Brightness', 'Contrast', 'Invert', 'Opacity',
> +                       'Saturate', 'Sepia', 'Grayscale' ]
> +         %>
> +
> +        let mut filters = Vec::new();

We should use SmallVec instead of Vec.

::: servo/components/style/properties/gecko.mako.rs:3565
(Diff revision 1)
> +                        let shadow_arr =
> +                            filter.__bindgen_anon_1.mDropShadow.as_ref();

mDropShadow is a member of union field, we really can access it directly?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1381
(Diff revision 1)
> +        (&DropShadow(from_value), &DropShadow(to_value)) => {
> +            Ok(DropShadow(try!(from_value.add_weighted(&to_value,
> +                                                       self_portion,
> +                                                       other_portion))))

We should create an intermediate type for DropShadow since DropShadow has color component.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1395
(Diff revision 1)
> +fn add_weighted_filter_function(from: Option<Filter>,
> +                                to: Option<Filter>,

Should we use Option<&Filter> here?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1400
(Diff revision 1)
> +fn add_weighted_filter_function(from: Option<Filter>,
> +                                to: Option<Filter>,
> +                                self_portion: f64,
> +                                other_portion: f64) -> Result<Filter, ()> {
> +    match (from, to) {
> +        (Some(ref f),Some(ref t)) => {

Nit: a space.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1400
(Diff revision 1)
> +fn add_weighted_filter_function(from: Option<Filter>,
> +                                to: Option<Filter>,
> +                                self_portion: f64,
> +                                other_portion: f64) -> Result<Filter, ()> {
> +    match (from, to) {
> +        (Some(ref f),Some(ref t)) => {

Nit: a space.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1454
(Diff revision 1)
> +        let from_list = self.filters.clone();
> +        let to_list = (&other).filters.clone();

Why do we need to clone?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1462
(Diff revision 1)
> +            filters.push(try!(add_weighted_filter_function(from.cloned(),
> +                                                           to.cloned(),

If we change the arguments of add_weighted_filter_function to Option<&>, we don't need to do cloned()?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> ::: servo/components/style/properties/gecko.mako.rs:3544
> (Diff revision 1)
> > +        <%
> > +         FUNCTIONS = [ 'Brightness', 'Contrast', 'Invert', 'Opacity',
> > +                       'Saturate', 'Sepia', 'Grayscale' ]
> > +         %>
> > +
> > +        let mut filters = Vec::new();
> 
> We should use SmallVec instead of Vec.

Ignore this.
Thank you so much.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Comment on attachment 8872932 [details]
> Bug 1362897 - make filter property animatable.
> 
> https://reviewboard.mozilla.org/r/144458/#review148280
> 
> I don't know why we still use Vec for filter property instead of SmallVec
> though.

I didn't know this crate. Should we use it?
I looked bug 1360881 and small vec documents, I think that it is useful for storing the limited amount filter functions. But I'm not sure that using small vec is right in this case. The specifiable function amount is not limited.(e.g. filter: blur(10px) opacity(0.5) .... blur(20px) grayscale(0.4)...). However we can specify this small vec limited amount as hard-code.
 
> ::: servo/components/style/properties/gecko.mako.rs:3565
> (Diff revision 1)
> > +                        let shadow_arr =
> > +                            filter.__bindgen_anon_1.mDropShadow.as_ref();
> 
> mDropShadow is a member of union field, we really can access it directly?
> 

Ah, you mean we should access via ffi?

> :::
> servo/components/style/properties/helpers/animated_properties.mako.rs:1381
> (Diff revision 1)
> > +        (&DropShadow(from_value), &DropShadow(to_value)) => {
> > +            Ok(DropShadow(try!(from_value.add_weighted(&to_value,
> > +                                                       self_portion,
> > +                                                       other_portion))))
> 
> We should create an intermediate type for DropShadow since DropShadow has
> color component.
> 

As far as I can say, DropShadow's value is 'Shadow', it is values::specified::Shadow which same to BoxShadow[1][2]. And DropShadow's value don't List type. So I thought that we can reuse BoxShadow's interpolation function as DropShadow's interpolation function[3]. Could you please point out my wrong?

[1] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/servo/components/style/properties/longhand/effects.mako.rs#51
[2] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/servo/components/style/properties/longhand/effects.mako.rs#135 
[3] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/servo/components/style/properties/helpers/animated_properties.mako.rs#2932-2933,2936
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Comment on attachment 8872932 [details]
> Bug 1362897 - make filter property animatable.
> 
> https://reviewboard.mozilla.org/r/144458/#review148280
> 
> ::: servo/components/style/gecko/url.rs:51
> (Diff revision 1)
> > +    pub unsafe fn parse_from_url_value(url: &URLValue)
> > +                                       -> Result<SpecifiedUrl, ()> {
> > +        Ok(SpecifiedUrl {
> > +            serialization: Arc::new((url._base.mString.to_string())),
> > +            extra_data:
> > +                (*RefPtr::from_ptr_ref(&url._base.mExtraData.mRawPtr)).clone(),
> > +            image_value: None,
> > +        })
> > +    }
> 
> URL handling is out of my knowledge, this part should be reviwed by someone
> familiar with this stuff. Xidorn?
> 

Ok. I'll ask xidorn after splitting the patch.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #13)
> Thank you so much.
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > Comment on attachment 8872932 [details]
> > Bug 1362897 - make filter property animatable.
> > 
> > https://reviewboard.mozilla.org/r/144458/#review148280
> > 
> > I don't know why we still use Vec for filter property instead of SmallVec
> > though.
> 
> I didn't know this crate. Should we use it?
> I looked bug 1360881 and small vec documents, I think that it is useful for
> storing the limited amount filter functions. But I'm not sure that using
> small vec is right in this case. The specifiable function amount is not
> limited.(e.g. filter: blur(10px) opacity(0.5) .... blur(20px)
> grayscale(0.4)...). However we can specify this small vec limited amount as
> hard-code.

I think we should use SmallVec for filter list, but it's another problem.
 
> > ::: servo/components/style/properties/gecko.mako.rs:3565
> > (Diff revision 1)
> > > +                        let shadow_arr =
> > > +                            filter.__bindgen_anon_1.mDropShadow.as_ref();
> > 
> > mDropShadow is a member of union field, we really can access it directly?
> > 
> 
> Ah, you mean we should access via ffi?

I don't think we need to add a new FFI function for that, but in the setter code for filter property, we get __bindgen_anon_1 once as a variable of union, and then access to mDropShadow.  Yeah, it's actually a union, we can access directly it's memory, do we directly access in other places? 

> > :::
> > servo/components/style/properties/helpers/animated_properties.mako.rs:1381
> > (Diff revision 1)
> > > +        (&DropShadow(from_value), &DropShadow(to_value)) => {
> > > +            Ok(DropShadow(try!(from_value.add_weighted(&to_value,
> > > +                                                       self_portion,
> > > +                                                       other_portion))))
> > 
> > We should create an intermediate type for DropShadow since DropShadow has
> > color component.
> > 
> 
> As far as I can say, DropShadow's value is 'Shadow', it is
> values::specified::Shadow which same to BoxShadow[1][2]. And DropShadow's
> value don't List type. So I thought that we can reuse BoxShadow's
> interpolation function as DropShadow's interpolation function[3]. Could you
> please point out my wrong?

DropShadow is not equal to IntermediateDropShadow at all (There is no IntermediateDropShadow in your patch).
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> I think we should use SmallVec for filter list, but it's another problem.

OK. I'll file another bug.
  
> I don't think we need to add a new FFI function for that, but in the setter
> code for filter property, we get __bindgen_anon_1 once as a variable of
> union, and then access to mDropShadow.  Yeah, it's actually a union, we can
> access directly it's memory, do we directly access in other places? 

Here:
http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/servo/components/style/gecko_bindings/sugar/ns_timing_function.rs#124

function.__bindgen_anon_1.__bindgen_anon_1 is:
http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/servo/components/style/gecko/generated/structs_debug.rs#30609-30637
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> > I think we should use SmallVec for filter list, but it's another problem.
> 
> OK. I'll file another bug.
>   
> > I don't think we need to add a new FFI function for that, but in the setter
> > code for filter property, we get __bindgen_anon_1 once as a variable of
> > union, and then access to mDropShadow.  Yeah, it's actually a union, we can
> > access directly it's memory, do we directly access in other places? 
> 
> Here:
> http://searchfox.org/mozilla-central/rev/
> 1a0d9545b9805f50a70de703a3c04fc0d22e3839/servo/components/style/
> gecko_bindings/sugar/ns_timing_function.rs#124
> 
> function.__bindgen_anon_1.__bindgen_anon_1 is:
> http://searchfox.org/mozilla-central/rev/
> 1a0d9545b9805f50a70de703a3c04fc0d22e3839/servo/components/style/gecko/
> generated/structs_debug.rs#30609-30637

Oh, this isn't good to me.. If you prefer to access directly, it's OK.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Comment on attachment 8872932 [details]
> Bug 1362897 - make filter property animatable.
> 
> https://reviewboard.mozilla.org/r/144458/#review148280
> 
> :::
> servo/components/style/properties/helpers/animated_properties.mako.rs:1395
> (Diff revision 1)
> > +fn add_weighted_filter_function(from: Option<Filter>,
> > +                                to: Option<Filter>,
> 
> Should we use Option<&Filter> here?
> 

Oh,, mako template will remove '<' character if we sepcified parameter as 'Option<&Filter>'.
I'm not sure why mako remove this character, but we can prevent this preprocess if we use the space like 'Option< &Filter>'...
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #18)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > Comment on attachment 8872932 [details]
> > Bug 1362897 - make filter property animatable.
> > 
> > https://reviewboard.mozilla.org/r/144458/#review148280
> > 
> > :::
> > servo/components/style/properties/helpers/animated_properties.mako.rs:1395
> > (Diff revision 1)
> > > +fn add_weighted_filter_function(from: Option<Filter>,
> > > +                                to: Option<Filter>,
> > 
> > Should we use Option<&Filter> here?
> > 
> 
> Oh,, mako template will remove '<' character if we sepcified parameter as
> 'Option<&Filter>'.
> I'm not sure why mako remove this character, but we can prevent this
> preprocess if we use the space like 'Option< &Filter>'...

We use '<<'.
You need to rebase on this PR<https://github.com/servo/servo/pull/17157>.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> You need to rebase on this PR<https://github.com/servo/servo/pull/17157>.

Thanks.
But this patch is not finished one. I'll merge these changes before requesting review.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Comment on attachment 8872933 [details]
> bug 1362897 - add w-p-t of interpolating filter property.
> 
> https://reviewboard.mozilla.org/r/144460/#review148262
> :::
> testing/web-platform/tests/web-animations/animation-model/animation-types/
> property-types.js:1314
> (Diff revision 1)
> > +                     'blur(5px) drop-shadow(rgba(100, 100, 100, 0.5) ' +
> > +                     '50px 50px 50px)' }]);
> 
> It's not clear to me what the default color value of drop-shadow is.
> From the spec;
>  The lacuna value for interpolation is all length values set to 0 and the
> used color is taken from the color property.
>  
> Is that mean currentColor?

Perhaps, it is not currentColor keyword. I think this value is depends on UA, like color property's initial value.
I tried the following test-case, each browser(Chrome/Firefox) result is different.
(Safari/Edge does not support interpolate from none...)

test-case: http://output.jsbin.com/toquwiq

The gecko will treat as transparent color[1].

[1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/layout/style/StyleAnimationValue.cpp#1988-2001
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #22)
> > It's not clear to me what the default color value of drop-shadow is.
> > From the spec;
> >  The lacuna value for interpolation is all length values set to 0 and the
> > used color is taken from the color property.
> >  
> > Is that mean currentColor?
> 
> Perhaps, it is not currentColor keyword. I think this value is depends on
> UA, like color property's initial value.
> I tried the following test-case, each browser(Chrome/Firefox) result is
> different.
> (Safari/Edge does not support interpolate from none...)

I did ask Xidorn on IRC about this. His answer is it's currentColor.
I believe web-platform-test should be implemented as what the spec defines.
Attachment #8874720 - Flags: review?(xidorn+moz) → review?(cam)
Comment on attachment 8874719 [details]
Bug 1362897 - Enable w-p-t of filter's animation except accumulate test.

https://reviewboard.mozilla.org/r/146086/#review150024

::: commit-message-2c628:1
(Diff revision 1)
> +Bug 1362897 - Add interpolation for Angle except radian. r?birtles

Preserve the unit when interpolating/adding angles with matching units

::: commit-message-2c628:3
(Diff revision 1)
> +Current the unit of angle interpolation is radian. However the gecko will use
> +the specified unit, if specified value's unit is same(e.g. if we use
> +hue-rotate(10deg) to hue-rotate(100deg), then the unit of interpolation result
> +is deg).

If the units of two angles being interpolated/added matches, we should preserve the original unit; otherwise, we fall back to radians. This matches the behavior of Gecko.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:876
(Diff revision 1)
>  /// https://drafts.csswg.org/css-transitions/#animtype-number
>  impl Animatable for Angle {
>      #[inline]
>      fn add_weighted(&self, other: &Angle, self_portion: f64, other_portion: f64) -> Result<Self, ()> {
> +        match (*self, *other) {
> +            % for angle_type in [ 'Degree', 'Gradian', 'Turn', 'Radian' ]:

Do we need Radian here do we?
Attachment #8874719 - Flags: review?(bbirtles) → review+
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

https://reviewboard.mozilla.org/r/144458/#review150032

I will review this once this patch gets rebased on Xidorn's current color patch.
But at first glance this still looks incorrect.

::: servo/components/style/properties/longhand/effects.mako.rs:85
(Diff revision 2)
>                            animation_value_type="ComputedValue",
>                            boxed="True",
>                            allow_quirks=True,
>                            spec="https://drafts.fxtf.org/css-masking/#clip-property")}
>  
> -// FIXME: This prop should be animatable
> +<%helpers:longhand name="filter" animation_value_type="ComputedValue" extra_prefixes="webkit"

If you properly make filter animatable, this animation_value_type will be changed to IntermediateFilter or something.
Attachment #8872932 - Flags: review?(hikezoe)
Comment on attachment 8872933 [details]
Bug 1362897 - Add w-p-t of interpolating filter property.

https://reviewboard.mozilla.org/r/144460/#review150016

Looks good to me. Thanks for doing!

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1211
(Diff revision 2)
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      var animation = target.animate({ [idlName]:
> +                                       ['blur(10px)', 'blur(50px)'] },
> +                                     { duration: 1000 });

Nit: we can just put 1000 without js object. And also in rest of cases.
Attachment #8872933 - Flags: review?(hikezoe) → review+
Comment on attachment 8874721 [details]
Bug 1362897 - Add compute distance for Filter.

https://reviewboard.mozilla.org/r/146090/#review150040

Thanks for implementing this function.
r=me with the folloing comments addressed.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1391
(Diff revision 1)
> +                       'Brightness', 'Contrast', 'Opacity', 'Saturate' ]
> +     %>
> +    match (from, to) {
> +        % for func in FUNCTION_LIST :
> +        (&${func}(f), &${func}(t)) => {
> +             Ok(try!(f.compute_distance(&t)))

should be ```f.compute_squared_distance()```.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1397
(Diff revision 1)
> +        },
> +        % endfor
> +        (&DropShadow(f), &DropShadow(t)) => {
> +            let from_shadow = IntermediateFilterDropShadow::from(f);
> +            let to_shadow = IntermediateFilterDropShadow::from(t);
> +            let interpolated_value = from_shadow.compute_distance(&to_shadow);

should be ```from_shadow.compute_squared_distance(...)```.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1441
(Diff revision 1)
>          let filters = vec![&from_list[..], &to_list[..]].concat();
>          Ok(Filters::new(filters))
>      }
> +
> +    #[inline]
> +    fn compute_distance(&self, other: &Self) -> Result<f64, ()> {

The default behavior of ```compute_squared_distance()``` is: square the result of ```compute_distance()```. However, in the current implementation, if we call ```compute_sqaured_distance()```, we compute the squared distance and then get its square-root in ```compute_distance()``` first, and then square this square-root. This is not efficient. Therefore, I suggest you implement ```compute_squared_distance()``` first. In other words, just rename this function as ```compute_squared_distance()```, and make it return ```square_distance```, instead of ```square_distance.sqrt()```. In ```compute_distance()```, just get the square-root of the result of ```compute_squared_distance()```. Just like what we did for CalcLengthOrPercentage [1].

[1] http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/servo/components/style/properties/helpers/animated_properties.mako.rs#1014-1017

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1455
(Diff revision 1)
> +            if from == None {
> +                let none = try!(add_weighted_filter_function(to, to, 0.0, 0.0));
> +                current_square_distance =
> +                    compute_filter_square_distance(&none, &(to.unwrap())).unwrap();
> +
> +                from = from_iter.next();

if ```from == None```, we should call next on ```to```, right?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1459
(Diff revision 1)
> +
> +                from = from_iter.next();
> +            } else if to == None {
> +                let none = try!(add_weighted_filter_function(from, from, 0.0, 0.0));
> +                current_square_distance =
> +                    compute_filter_square_distance(&none, &(to.unwrap())).unwrap();

r/to.unwrap()/from.unwrap()/

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1461
(Diff revision 1)
> +            } else if to == None {
> +                let none = try!(add_weighted_filter_function(from, from, 0.0, 0.0));
> +                current_square_distance =
> +                    compute_filter_square_distance(&none, &(to.unwrap())).unwrap();
> +
> +                to = to_iter.next();

if ```to == None```, we should call next on ```from```, right?
Attachment #8874721 - Flags: review?(boris.chiou) → review+
Comment on attachment 8874720 [details]
Bug 1362897 - Add function which convert from URLValueData to SpecifiedUrl.

https://reviewboard.mozilla.org/r/146088/#review150506

r=me with that.

::: servo/components/style/gecko/url.rs:58
(Diff revision 1)
>      pub fn is_invalid(&self) -> bool {
>          false
>      }
>  
> +    /// Convert from URLValueData to SpecifiedUrl.
> +    pub unsafe fn parse_from_url_value(url: &URLValueData)

Nit: I don't think "parse" is the right word, since we're really just converting the existing string, we're not parsing it.  How about "from_url_value_data"?

::: servo/components/style/gecko/url.rs:61
(Diff revision 1)
>  
> +    /// Convert from URLValueData to SpecifiedUrl.
> +    pub unsafe fn parse_from_url_value(url: &URLValueData)
> +                                       -> Result<SpecifiedUrl, ()> {
> +        Ok(SpecifiedUrl {
> +            serialization: Arc::new((url.mString.to_string())),

Nit: drop the extra parens around the Arc::new argument.

::: servo/components/style/gecko/url.rs:62
(Diff revision 1)
> +            extra_data:
> +                (*RefPtr::from_ptr_ref(&url.mExtraData.mRawPtr)).clone(),

Can this just be:

  extra_data: url.mExtraData.to_safe(),

I think the mExtraData will never be null (which to_safe() requires).
Attachment #8874720 - Flags: review?(cam) → review+
Comment on attachment 8874719 [details]
Bug 1362897 - Enable w-p-t of filter's animation except accumulate test.

https://reviewboard.mozilla.org/r/146086/#review150024

Thanks.

> Do we need Radian here do we?

Fixed. It is enough to fallback to the last arms.
Comment on attachment 8874720 [details]
Bug 1362897 - Add function which convert from URLValueData to SpecifiedUrl.

https://reviewboard.mozilla.org/r/146088/#review150506

Thanks.
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

https://reviewboard.mozilla.org/r/144458/#review150032

Thanks, I updated the patch. Could you please review it again?
Comment on attachment 8874721 [details]
Bug 1362897 - Add compute distance for Filter.

https://reviewboard.mozilla.org/r/146090/#review150040

Thank you so much.
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

https://reviewboard.mozilla.org/r/144458/#review151794

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1373
(Diff revision 3)
> +<%
> +    FILTER_FUNCTIONS = [ 'Blur', 'Brightness', 'Contrast', 'Grayscale',
> +                         'HueRotate', 'Invert', 'Opacity', 'Saturate',
> +                         'Sepia' ]
> +%>
> +impl From<Filters> for IntermediateFilters {

What we need here is 
 impl From<Filter> for IntermediateFilter

and vice versa.

For the traits for Filters and IntermediateFilters, we can use into_iter().map(|f| f.into()).collect()?
Attachment #8872932 - Flags: review?(hikezoe)
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

https://reviewboard.mozilla.org/r/144458/#review151794

> What we need here is 
>  impl From<Filter> for IntermediateFilter
> 
> and vice versa.
> 
> For the traits for Filters and IntermediateFilters, we can use into_iter().map(|f| f.into()).collect()?

Thanks.

OK.
I've separated the From trait into Filter and Filters.
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

https://reviewboard.mozilla.org/r/144458/#review152206

r=me with comments addressed.

Thanks you for doing this!

::: servo/components/style/properties/gecko.mako.rs:3555
(Diff revision 4)
> +                        filters.push(DropShadow(Shadow {
> +                            offset_x: Au(shadow.mXOffset),
> +                            offset_y: Au(shadow.mYOffset),
> +                            blur_radius: Au(shadow.mRadius),
> +                            spread_radius: Au(shadow.mSpread),
> +                            inset: shadow.mInset,
> +                            color: convert_nscolor_to_rgba(shadow.mColor).into(),

Can't we use to_shadow()?

::: servo/components/style/properties/gecko.mako.rs:3555
(Diff revision 4)
> +                NS_STYLE_FILTER_DROP_SHADOW => {
> +                    unsafe {
> +                        let ref shadow =
> +                            (**filter.__bindgen_anon_1.mDropShadow.as_ref()).mArray[0];
> +
> +                        filters.push(DropShadow(Shadow {

Nit: filters.push() can be put outside unsafe?

::: servo/components/style/properties/gecko.mako.rs:3570
(Diff revision 4)
> +                NS_STYLE_FILTER_URL => {
> +                    unsafe {
> +                        let ref url = (**filter.__bindgen_anon_1.mURL.as_ref())._base;
> +                        let specified =
> +                            (SpecifiedUrl::from_url_value_data(url)).unwrap();
> +                        filters.push(Url(specified));

Nit: filters.push() can be put outside unsafe block?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1347
(Diff revision 4)
>              _ => Err(()),
>          }
>      }
>  }
>  
> +#[derive(Clone, PartialEq, Debug)]

This whole block should be at the end of this file.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1351
(Diff revision 4)
>  
> +#[derive(Clone, PartialEq, Debug)]
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +#[allow(missing_docs)]
> +/// Intermediate type for filter property.
> +/// The difference between normal filter type is that this structure uses

difference from

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1353
(Diff revision 4)
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +#[allow(missing_docs)]
> +/// Intermediate type for filter property.
> +/// The difference between normal filter type is that this structure uses
> +/// IntermediateColor into DropShadow's value.
> +pub struct IntermediateFilters { pub filters: Vec<IntermediateFilter> }

I wonder we can use Vec<IntermediateFilter> as IntermediateFilters type?  I mean

pub type IntermediateFilters = Vec<IntermediateFilter>;

?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1407
(Diff revision 4)
> +            % for func in FILTER_FUNCTIONS:
> +                ${func}(val) => IntermediateFilter::${func}(val),
> +            % endfor
> +            % if product == "gecko":
> +                DropShadow(shadow) => {
> +                    IntermediateFilter::DropShadow(IntermediateShadow::from(shadow))

Nit: shadow.into()

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1426
(Diff revision 4)
> +            % for func in FILTER_FUNCTIONS:
> +                IntermediateFilter::${func}(val) => Filter::${func}(val),
> +            % endfor
> +            % if product == "gecko":
> +                IntermediateFilter::DropShadow(shadow) => {
> +                    DropShadow(Shadow::from(shadow))

Likewise: shadow.into()

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1483
(Diff revision 4)
> +                    from_value.add_weighted(&to_value,
> +                                            self_portion,
> +                                            other_portion))))
> +            },
> +        % endif
> +        _ => {

Should we speficy (&Url, &Url) arm here to distinguish other interpolation with different type filter functions?
IIUC, add_weighted_filter_function_impl() is not called with different filter type funcions, if so we should use unreachable() instead of returning Err(()) for such cases?
Attachment #8872932 - Flags: review?(hikezoe) → review+
Blocks: stylo-wpt
Comment on attachment 8872932 [details]
Bug 1362897 - Make filter property animatable.

https://reviewboard.mozilla.org/r/144458/#review152206

Thank you for reviewing this patch.

> Should we speficy (&Url, &Url) arm here to distinguish other interpolation with different type filter functions?
> IIUC, add_weighted_filter_function_impl() is not called with different filter type funcions, if so we should use unreachable() instead of returning Err(()) for such cases?

If we specify the '''from { filter: blur(10px); } to { filter: contrast(0.5); }''', add_weighted_filter_function_impl() is called with different filter type functions.
If we changed to the 'unreachable', we can't interpolate as discrete. So I'll use Err(()) here.
Attached file Servo PR, #17288 (obsolete) — 

    
    

    
  
Comment on attachment 8876962 [details]
Servo PR, #17288

Error retrieving Github pull request diff for https://github.com/servo/servo/pull/17288

        Attachment #8876962 -
        Attachment mime type: text/plain → text/x-github-pull-request

    
  

        Attachment #8876962 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Servo PR, #17288
        — 
      

    


  
Blocks: 1292283

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8874720 -
        Attachment is obsolete: true

    
  

        Attachment #8872932 -
        Attachment is obsolete: true

    
  

        Attachment #8874721 -
        Attachment is obsolete: true

    
  
Blocks: 1374151

    
    

    
  
Recently, wpt enabled on Stylo, So I ran latest patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa2ab0063875168893d68c50505469ddc1fce2a3

The accumulate test of filter is failed.[1]

[1] http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js#1237-1241

Filter of Stylo doesn't restrict negative value if accumulated value will be negative. (i.e. if value will be -0.6, it will return not 0.0 but -0.6.)
In gecko, we used RestrictValue function in order to restrict negative[2].

[2] http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/style/StyleAnimationValue.cpp#550-571

I'll follow up this on bug 1374151.

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #61)
> Comment on attachment 8874719 [details]
> Bug 1362897 - Enable w-p-t of filter's animation.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/146086/diff/4-5/

This patch is enable other filter's tests.

    
    

    
  
Comment on attachment 8874719 [details]
Bug 1362897 - Enable w-p-t of filter's animation except accumulate test.

https://reviewboard.mozilla.org/r/146086/#review154882

::: commit-message-2c628:1
(Diff revision 5)
> +Bug 1362897 - Enable w-p-t of filter's animation. r?hiro

This commit message misleads that all test cases for filter property can be passed now. We should note that there is still a failure test for filter property.

        Attachment #8874719 -
        Flags: review?(hikezoe) → review+

    
    

    
      
  

    
    

    
  
Comment on attachment 8874719 [details]
Bug 1362897 - Enable w-p-t of filter's animation except accumulate test.

https://reviewboard.mozilla.org/r/146086/#review154882

Thanks for reviewing it.

    
    

    
  
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #65)
> Comment on attachment 8874719 [details]
> Bug 1362897 - Enable w-p-t of filter's animation except accumulate test.
> 
> https://reviewboard.mozilla.org/r/146086/#review154882
> 
> Thanks for reviewing it.

Next time, please stop merging the corresponding servo PR if you have additional reviews for gecko side change.

    
    

    
  
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d056ddab0b2
Add w-p-t of interpolating filter property. r=hiro
https://hg.mozilla.org/integration/autoland/rev/afb0e8bf4fcc
Enable w-p-t of filter's animation except accumulate test. r=birtles,hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7d056ddab0b2
https://hg.mozilla.org/mozilla-central/rev/afb0e8bf4fcc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox56:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: