Closed Bug 1356162 Opened 7 years ago Closed 7 years ago

stylo: Make clip animatable

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1356162 - Enable web-platform-tests of clip property animation.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
We will need to make clip property animatable on stylo.

This property type in servo is ClipRectOrAuto. This type's interpolate function is already exist. So we will need to implement clone function only.
Assignee: nobody → mantaroh
Priority: -- → P1
Status: NEW → ASSIGNED
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review132864

::: servo/components/style/properties/longhand/effects.mako.rs:80
(Diff revision 1)
>      pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<specified::Shadow, ()> {
>          specified::Shadow::parse(context, input, false)
>      }
>  </%helpers:vector_longhand>
>  
>  // FIXME: This prop should be animatable

shouldn't the FIXME be removed?
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review132864

> shouldn't the FIXME be removed?

Thanks, robert,
I forgot removing this comment. I addressed it.
Comment on attachment 8858230 [details]
Bug 1356162 - Enable web-platform-tests of clip property animation.

https://reviewboard.mozilla.org/r/130206/#review132952

It would be nice to have 'auto' value cases.
Also, you should definetely run these test on gecko before requesting review.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:986
(Diff revision 2)
> +                                         ['rect(10px, 10px, 10px, 10px)',
> +					  'rect(10px, 10px, 10px, 10px)'] },
> +                                     { duration: 1000, composite: 'add' });
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 0, expected: 'rect(110px, 100px, 100px, 100xp)' }]);

I think this expected value is wrong. All parameters should be 110px.  '100xp' mush be a typo.
Attachment #8858230 - Flags: review?(hikezoe)
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review132954

::: servo/components/style/properties/gecko.mako.rs:2720
(Diff revision 2)
> +        use values::computed::{Auto, ClipRect};
> +        use values::Either;
> +
> +        let current_clip_flags = self.gecko.mClipFlags as u32;
> +
> +        if (current_clip_flags & NS_STYLE_CLIP_AUTO) != 0 {

I don't think this is correct. NS_STYLE_CLIP_AUTO is defined as 0.
Attachment #8858229 - Flags: review?(hikezoe) → review-
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review132954

Thanks, hiro.

I fixed clone function.
Comment on attachment 8858230 [details]
Bug 1356162 - Enable web-platform-tests of clip property animation.

https://reviewboard.mozilla.org/r/130206/#review133282

Please add 'auto' value case.  Thanks!
Attachment #8858230 - Flags: review?(hikezoe) → review+
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review133284

::: servo/components/style/properties/gecko.mako.rs:2808
(Diff revision 3)
> +        let current_clip_flags = self.gecko.mClipFlags as u32;
> +
> +        if current_clip_flags == NS_STYLE_CLIP_AUTO {

Instead of doing this, we should cast as 'u8' just like we do in set_clip():

if self.gecko.mClipFlags == NS_STYLE_CLIP_AUTO as u8 {
  ...
}

::: servo/components/style/properties/gecko.mako.rs:2810
(Diff revision 3)
> +        if current_clip_flags == NS_STYLE_CLIP_AUTO {
> +            Either::First(ClipRect {
> +                            top: Some(Au(self.gecko.mClip.y)),
> +                            right: Some(Au(self.gecko.mClip.x + self.gecko.mClip.width)),
> +                            bottom: Some(Au(self.gecko.mClip.y + self.gecko.mClip.height)),
> +                            left: Some(Au(self.gecko.mClip.x)),
> +                          })
> +        } else {
> +            Either::Second(Auto)
> +        }

This looks still wrong.
At first glance the if condition is wrong.  Isn't it the case for non-'auto' case?  Actually I wanted to comment that we should check the equality of auto case instead of inequality of auto case in bug 1355732, but I quitted since I think you'd prefer to it.  I should have commented there since it's error-prone.

Also, when mClip.x == 0 we need to set None for the servo's value. etc.
Attachment #8858229 - Flags: review?(hikezoe)
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review133284

> This looks still wrong.
> At first glance the if condition is wrong.  Isn't it the case for non-'auto' case?  Actually I wanted to comment that we should check the equality of auto case instead of inequality of auto case in bug 1355732, but I quitted since I think you'd prefer to it.  I should have commented there since it's error-prone.
> 
> Also, when mClip.x == 0 we need to set None for the servo's value. etc.

Sorry, it is my mistake. I addressed it.

And I set the None value when mClipFlags has each *_AUTO value. (e.g. when flags has NS_STYLE_CLIP_TOP_AUTO flag, we will set the mClip.y value, otherwise, set the None)
Comment on attachment 8858230 [details]
Bug 1356162 - Enable web-platform-tests of clip property animation.

https://reviewboard.mozilla.org/r/130206/#review133556

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:969
(Diff revision 4)
> +          [{ time: 0,    expected: 'rect(10px, 10px, 10px, 10px)' },
> +           { time: 500,  expected: 'rect(30px, 30px, 30px, 30px)' },
> +           { time: 1000, expected: 'rect(50px, 50px, 50px, 50px)' }]);

I just realized we just check interpolated value at 500ms in other test cases, so we don't need to check  the values at 0ms and 1000ms.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:974
(Diff revision 4)
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      var animation = target.animate({ [idlName]:
> +                                         ['rect(10px, 10px, 10px, 10px)',
> +					  'auto'] },
> +                                     { duration: 1000, fill: 'both' });
> +	testAnimationSamples(
> +          animation, idlName,
> +          [{ time: 0,    expected: 'rect(10px, 10px, 10px, 10px)' },
> +           { time: 499,  expected: 'rect(10px, 10px, 10px, 10px)' },
> +           { time: 500,  expected: 'auto' },
> +           { time: 1000, expected: 'auto' }]);
> +    }, property + 'supports animation as a rect with auto');

This test case should be run as 'discrete' type?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1004
(Diff revision 4)
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      target.style[idlName] = 'rect(10px, 10px, 10px, 10px)';
> +      var animation = target.animate({ [idlName]:
> +                                         ['auto',
> +                                          'rect(10px, 10px, 10px, 10px)'] },
> +                                     { duration: 1000, composite: 'add' });
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 0, expected: 'auto' }]);
> +    }, property + ': rect with auto');

Likewise this test case?
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review133558

::: servo/components/style/properties/gecko.mako.rs:2813
(Diff revision 4)
> +	use gecko_bindings::structs::NS_STYLE_CLIP_LEFT_AUTO;
> +        use values::computed::{Auto, ClipRect};
> +        use values::Either;
> +
> +        if self.gecko.mClipFlags != NS_STYLE_CLIP_AUTO as u8 {
> +            let top = if self.gecko.mClipFlags & (NS_STYLE_CLIP_TOP_AUTO as u8) == 0 {

The condition looks wrong to me.
We need to return None if NS_STYLE_CLIP_TOP_AUTO is set.
Also we can add debug_assert!(self.gecko.mClip.y == 0) here?

We might need at least one test case that 'left' is 'auto'.
Also this checking order should be matched to set_clip(), i.e. left, top, bottom, right.

::: servo/components/style/properties/gecko.mako.rs:2822
(Diff revision 4)
> +            };
> +
> +            let right = if self.gecko.mClipFlags & (NS_STYLE_CLIP_RIGHT_AUTO as u8) == 0 {
> +                None
> +            } else {
> +                Some(Au(self.gecko.mClip.y + self.gecko.mClip.width))

This is wrong, should be mClip.x.
Attachment #8858229 - Flags: review?(hikezoe)
Comment on attachment 8858230 [details]
Bug 1356162 - Enable web-platform-tests of clip property animation.

https://reviewboard.mozilla.org/r/130206/#review133556

> This test case should be run as 'discrete' type?

It is similar to discrete type.
In my understand, we can't animate 'auto' value[1]. If we can't interpolate from/to value, we treat like discrete[2].

[1] https://lists.w3.org/Archives/Public/www-style/2012Nov/0387.html
[2] https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/dom/animation/KeyframeEffectReadOnly.cpp#646-656

Other User Agent like chrome is same behavior, but Edge/Safari is NOT same. And as far as I searched, this behavior didn't define any spec.
So I think that we had better to drop this test.

hiro,
How do you think about removing this tests of 'auto' value?
Flags: needinfo?(hikezoe)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #21)
> Comment on attachment 8858230 [details]
> Bug 1356162 part 2 - Enable w-p-t of clip animation.
> 
> https://reviewboard.mozilla.org/r/130206/#review133556
> 
> > This test case should be run as 'discrete' type?
> 
> It is similar to discrete type.
> In my understand, we can't animate 'auto' value[1]. If we can't interpolate
> from/to value, we treat like discrete[2].

We don't create any CSS transition for 'auto' value, but we do create CSS animations for 'auto' as discrete type.
Flags: needinfo?(hikezoe)
Attachment #8858229 - Flags: review?(hikezoe)
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review133558

Sorry, the condition is not correct.
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review134208

::: servo/components/style/properties/gecko.mako.rs:2827
(Diff revision 6)
> +        use gecko_bindings::structs::NS_STYLE_CLIP_TOP_AUTO;
> +	use gecko_bindings::structs::NS_STYLE_CLIP_RIGHT_AUTO;
> +	use gecko_bindings::structs::NS_STYLE_CLIP_BOTTOM_AUTO;
> +	use gecko_bindings::structs::NS_STYLE_CLIP_LEFT_AUTO;

Please sort by alphabetical order.

::: servo/components/style/properties/gecko.mako.rs:2827
(Diff revision 6)
> +        use gecko_bindings::structs::NS_STYLE_CLIP_TOP_AUTO;
> +	use gecko_bindings::structs::NS_STYLE_CLIP_RIGHT_AUTO;
> +	use gecko_bindings::structs::NS_STYLE_CLIP_BOTTOM_AUTO;

Nit: Here are invisbile tabs.  MozReview does not show us them?

::: servo/components/style/properties/gecko.mako.rs:2834
(Diff revision 6)
> +	use gecko_bindings::structs::NS_STYLE_CLIP_BOTTOM_AUTO;
> +	use gecko_bindings::structs::NS_STYLE_CLIP_LEFT_AUTO;
> +        use values::computed::{Auto, ClipRect};
> +        use values::Either;
> +
> +        if self.gecko.mClipFlags != NS_STYLE_CLIP_AUTO as u8 {

I am wondering why you'd prefer to put 'auto' case block (i.e. returning Eiter::Second(Auto)) far from NS_STYLE_CLIP_AUTO check.  It would be easy to understand that the result stands close to the check like this;

if self.gecko.mCliFlags == NS_STYLE_CLIP_AUTO as u8 {
    Either::Second(Auto)
} else {
}

::: servo/components/style/properties/gecko.mako.rs:2835
(Diff revision 6)
> +	use gecko_bindings::structs::NS_STYLE_CLIP_LEFT_AUTO;
> +        use values::computed::{Auto, ClipRect};
> +        use values::Either;
> +
> +        if self.gecko.mClipFlags != NS_STYLE_CLIP_AUTO as u8 {
> +            let left = if self.gecko.mClipFlags & (NS_STYLE_CLIP_LEFT_AUTO as u8) == 0 {

Nit: You can drop parentheses here.
I am also wondering why you do something like this;

let left = if self.gecko.mClipFlags & NS_TYLE_CLIP_LEFT_AUTO as u8) != 0 {
    None
} else {
    Some(...)
}

::: servo/components/style/properties/gecko.mako.rs:2852
(Diff revision 6)
> +            };
> +
> +            let bottom = if self.gecko.mClipFlags & (NS_STYLE_CLIP_BOTTOM_AUTO as u8) == 0 {
> +                Some(Au(self.gecko.mClip.y + self.gecko.mClip.height))
> +            } else {
> +                debug_assert!(self.gecko.mClip.height == 1 << 30);

We should comment about 30 just like we do in set_clip().

::: servo/components/style/properties/gecko.mako.rs:2865
(Diff revision 6)
> +                None
> +            };
> +
> +            Either::First(ClipRect { top: top, right: right, bottom: bottom, left: left, })
> +        } else {
> +            Either::Second(Auto)

We can use ClipRectOrAuto::auto().
Attachment #8858229 - Flags: review?(hikezoe) → review+
Comment on attachment 8858229 [details]
Bug 1356162 part 1 - Make clip animatable.

https://reviewboard.mozilla.org/r/130204/#review134208

> Nit: You can drop parentheses here.
> I am also wondering why you do something like this;
> 
> let left = if self.gecko.mClipFlags & NS_TYLE_CLIP_LEFT_AUTO as u8) != 0 {
>     None
> } else {
>     Some(...)
> }

Oops. I forgot to drop a parenthesis.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #32)
> Comment on attachment 8858229 [details]
> Bug 1356162 part 1 - Make clip animatable.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/130204/diff/7-8/

As discussed with hiro, I changed condition of if statement for consistency with other implementation.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f25c58d5708d47d61e9b4c8abe26f66863188b3
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #34)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #32)
> > Comment on attachment 8858229 [details]
> > Bug 1356162 part 1 - Make clip animatable.
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/130204/diff/7-8/
> 
> As discussed with hiro, I changed condition of if statement for consistency
> with other implementation.
> 
> try :
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0f25c58d5708d47d61e9b4c8abe26f66863188b3

It seems to pass the adding tests. I've sent the PR of servo part:
https://github.com/servo/servo/pull/16555
The PR has been merged in servo, and merged in to mozilla-central. We should land test cases as well.
Flags: needinfo?(mantaroh)
The lack of gecko patch in this bug might cause stylo build failure on m-c now (I just re-based to the latest m-c). I guess stylo build on try is still green because it's configuration might be different with mine...
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #37)
> The lack of gecko patch in this bug might cause stylo build failure on m-c
> now (I just re-based to the latest m-c). I guess stylo build on try is still
> green because it's configuration might be different with mine...

Thanks for reporting. I'll try it and check it.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #38)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #37)
> > The lack of gecko patch in this bug might cause stylo build failure on m-c
> > now (I just re-based to the latest m-c). I guess stylo build on try is still
> > green because it's configuration might be different with mine...
> 
> Thanks for reporting. I'll try it and check it.

In the spec, Rect defined as Length or 'auto'[1]. However, stylo defined as number.[2]
It is my mistake when checking theresult. We should implement as LengthOrAuto type.

[1] https://drafts.fxtf.org/css-masking-1/#funcdef-clip-rect
[2] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/servo/components/style/values/computed/mod.rs#495-500

This test will fail following case:
 Interpolate between clip: "rect(10px, 10px, 10px, 10px)" and "rect(10px, 10px, 10px, auto)".

I'll submit third patch for resolving this.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #39)
> It is my mistake when checking theresult. We should implement as
> LengthOrAuto type.

Perhaps, this phenomenon is not related with LengthOrAuto problem.

If we animate from 'auto' to length, stylo doesn't reflect the timing function from keyframe.

For example(http://output.jsbin.com/nisodi/59): 
-----------------------------------------------------------------------------------
div.animate({width: ['auto', '200px'],
             easing: 'cubic-bezier(0.68,0,1,0.01)'},
            { duration: 1000, fill:'both', iterations:Infinity, direction:'alternate'});
-----------------------------------------------------------------------------------

If we use above example code, stylo animate as linear.
And it work well when specify this timing function to the effect part(i.e. {duration: 1000, ...., easing: 'cubic-bezier(..)'} ).

hiro,
Do you know that stylo's timing function implementation is difference from gecko?
It seems to me that stylo ignore the cubic-bezier.
Flags: needinfo?(hikezoe)
I don't quite follow what the problem is.

> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #39)
> > It is my mistake when checking theresult. We should implement as
> > LengthOrAuto type.
> 
> Perhaps, this phenomenon is not related with LengthOrAuto problem.
> 
> If we animate from 'auto' to length, stylo doesn't reflect the timing
> function from keyframe.
> 
> For example(http://output.jsbin.com/nisodi/59): 
> -----------------------------------------------------------------------------
> ------
> div.animate({width: ['auto', '200px'],
>              easing: 'cubic-bezier(0.68,0,1,0.01)'},
>             { duration: 1000, fill:'both', iterations:Infinity,
> direction:'alternate'});
> -----------------------------------------------------------------------------
> ------
> 
> If we use above example code, stylo animate as linear.
> And it work well when specify this timing function to the effect part(i.e.
> {duration: 1000, ...., easing: 'cubic-bezier(..)'} ).

If parsing easing in keyframes is broken, all of discrete type test cases fail on stylo?
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> I don't quite follow what the problem is.
> 

Ah, Sorry.
I misunderstood that test is failed. This phenomenon is not related this bug, so I will separate this bug. The problem is that stylo's doesn't use the cubic bezier function in th keyframes when animating 'auto' value.

Some tests fail on stylo(e.g. 'ime-mode' / 'hyphen' / ..etc). In any case, I'll submit it as an another bug.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #42)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> > I don't quite follow what the problem is.
> > 
> 
> Ah, Sorry.
> I misunderstood that test is failed. This phenomenon is not related this
> bug, so I will separate this bug. The problem is that stylo's doesn't use
> the cubic bezier function in th keyframes when animating 'auto' value.
> 
> Some tests fail on stylo(e.g. 'ime-mode' / 'hyphen' / ..etc). In any case,
> I'll submit it as an another bug.

Neither ime-mode nor hyphen is animatable on stylo yet...
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #43)
> Rebase and try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e4061d436ee7d83495a77cda7b5d89fd06d9afe7

You should run only web-platform-tests also with --artifact. And please update MANIFEST.json too.
Attachment #8858229 - Attachment is obsolete: true
Comment on attachment 8858230 [details]
Bug 1356162 - Enable web-platform-tests of clip property animation.

https://reviewboard.mozilla.org/r/130206/#review136658

::: testing/web-platform/meta/MANIFEST.json:63839
(Diff revision 9)
>     "web-animations/animation-model/animation-types/property-list.js": [
>      [
>       {}
>      ]
>     ],
> +   "web-animations/animation-model/animation-types/property-list.js.orig": [

Please drop this file.

::: testing/web-platform/meta/MANIFEST.json:211705
(Diff revision 9)
>    ],
>    "web-animations/animation-model/animation-types/property-list.js": [
> -   "0d7f75b05e66da256fe4d2ecb17332ec7abccfd2",
> +   "283bee6ac9dd238697fe7412c55f72a1ac47e7a3",
> +   "support"
> +  ],
> +  "web-animations/animation-model/animation-types/property-list.js.orig": [

Likewise.
Comment on attachment 8858230 [details]
Bug 1356162 - Enable web-platform-tests of clip property animation.

https://reviewboard.mozilla.org/r/130206/#review136658

I forgot obsoleting previous patch before starting to fix it.
Sorry to take up your time.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/02673f6ec65e
Enable web-platform-tests of clip property animation. r=hiro
https://hg.mozilla.org/mozilla-central/rev/02673f6ec65e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(mantaroh)
You need to log in before you can comment on or make changes to this bug.