1216844 - implement setting effect composition

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Cameron McCormack (:heycam)]

In order to support animations where values for the 0% or 100% keyframes are omitted, where the underlying value is effectively used, we need to support additive values in KeyframeEffects.  This will involve marking the mFromValue/mToValue StyleAnimationValues in the AnimationPropertySegments as being additive or not, and taking that into account when computing the effect.

Once that is done we can stop throwing an exception when properties have missing values on 0%/100% keyframes and instead generate additive "0" values.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Just for my reference.

When we implement effect composition, we need to compose additive or accumulate effects onto CSS transition's effect.  For example;

  div.style.width = "0px";
  div.style.transition = "width 1s linear";
  div.style.width = "100px";

  div.animate({ width: ['100px', '200px'] },
              { composite: 'additive', duration: 1000 });

In this case, div.style.width should be 300px (= 50px + 150px) at 0.5s.

Currently we have two animation rules respectively for the transition and the animation.  So we should compose this two rules somewhere.  A way what I can think of is to unify the animation rules into a single rule and call ComposeStyle() with the single rule.  This way could be done easily for compositable CSS properties, but for other CSS properties we need to modify nsStyleSet::FileRules() somehow.

Comment 2

[Brian Birtles (:birtles)]

Yes, I think the spec says:

"The level of the cascade to which this specified value is added depends on the class of the animation associated with the effect with the highest composite order in the effect stack for a given property."[1]

So, in this example, we should composite the two animations, then put the result in the transitions rule.

[1] https://w3c.github.io/web-animations/#applying-the-composited-result

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I've decided to implement accumulate and add in a separate bug respectively.

In this bug, setting effect composite will be implemented.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Changing title to match what I will do in this bug.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0348f6ce61d147ea59501aa45f09c8444bee222

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I was really really wondering why a test case which uses scale() failed on Android[1].  On Android it seemed that getOMTAStyle() can't get correct transform if composite:'add' is specified.  I did prepare for building for Android binary, and debugged it.  Finally I found the reason!  The element size was too big to run on the compositor after scaling!! I should have notice the fact soon,  I had doubted the implementation of additive animation.  This is my two days work...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0348f6ce61d147ea59501aa45f09c8444bee222&selectedJob=32123363

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11ea85b51e0aaa2d2bfdd6595d64690eb7744da9

Comment 8

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae1c69f87bfd1d1d1b402cb1f83593029f6277c

This bug depends on bug 1311620, but I think the patch for this bug can be reviewed independently. Actually just a test case in the patch relies on bug 1311620 (changing composite operation from additive to accumulates).

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1216844 - Implement KeyframeEffect::SetComposite().

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

The patch is based on the patches in bug 1321879.
Hello Olli, could you please take a look into change in KeyframeEffect.webidl?  I did actually try to request bz to review it because he worked bug 1321879, but unfortunately he don't accept any review requests for now. 

The CompositeOperation is very similar to IterationCompositeOperation, it's behind the pref controlled by IsCoreAPIEnabledForCaller().  

Thank you always in your busy time.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

https://reviewboard.mozilla.org/r/97406/#review97798

::: dom/animation/KeyframeEffect.cpp:163
(Diff revision 1)
> +KeyframeEffect::SetComposite(const CompositeOperation& aComposite,
> +                             CallerType aCallerType)
> +{
> +  // Ignore composite if the Web Animations API is not enabled, then the
> +  // default value 'Replace' will be used.
> +  if (!AnimationUtils::IsCoreAPIEnabledForCaller(aCallerType)) {

Shouldn't this be in the attribute as Func="..."?
Otherwise it looks like we support the setter but don't really, and that is detectable from web pages.


What is the difference between nsDocument::IsWebAnimationsEnabled and AnimationUtils::IsCoreAPIEnabledForCaller?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8816870 [details]
> Bug 1216844 - Implement KeyframeEffect::SetComposite().
> 
> https://reviewboard.mozilla.org/r/97406/#review97798
> 
> ::: dom/animation/KeyframeEffect.cpp:163
> (Diff revision 1)
> > +KeyframeEffect::SetComposite(const CompositeOperation& aComposite,
> > +                             CallerType aCallerType)
> > +{
> > +  // Ignore composite if the Web Animations API is not enabled, then the
> > +  // default value 'Replace' will be used.
> > +  if (!AnimationUtils::IsCoreAPIEnabledForCaller(aCallerType)) {
> 
> Shouldn't this be in the attribute as Func="..."?
> Otherwise it looks like we support the setter but don't really, and that is
> detectable from web pages.
> 
> 
> What is the difference between nsDocument::IsWebAnimationsEnabled and
> AnimationUtils::IsCoreAPIEnabledForCaller?

We want to run bunch of test cases which use chrome only APIs even if nsDocument::IsWebAnimationsEnabled returns false.  We decided to add a new function (AnimationUtils::IsCoreAPIEnabledForCaller) for that purpose in bug 1304805 comment 7.  It's bit confusing but we needed it.  I am sorry for less description in the review request.

Thanks.

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

https://reviewboard.mozilla.org/r/97406/#review97798

> Shouldn't this be in the attribute as Func="..."?
> Otherwise it looks like we support the setter but don't really, and that is detectable from web pages.
> 
> 
> What is the difference between nsDocument::IsWebAnimationsEnabled and AnimationUtils::IsCoreAPIEnabledForCaller?

I think the reason we don't use Func="..." is: we can use this api, "IsCoreAPIEnabledForCaller", not only in the Setters of composite and iterationComposite, but also during the process of constructing a KeyframeEffectReadOnly[1]. Once we are ready to ship these features, we could just remove it api or let it always return true.

[1] http://searchfox.org/mozilla-central/rev/a8b5f53e7df90df655a0982e94087ee83290c22e/dom/animation/KeyframeEffectReadOnly.cpp#584

Comment 14

[Boris Chiou [:boris]]

Comment on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

https://reviewboard.mozilla.org/r/97406/#review97954

r=me for the rest. I checked bz's patches, and think the logic of SetComposite() looks good to me. For the tests, I think you will add more for "accumulate" later. However, we need smaug's re-review and rebase this on bz's patches.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

Olli, could you please re-review this patch again?
Please see comment 12 and comment 13.  I would like to handle the nsDocument::IsWebAnimationsEnabled issue in a later bug if you can not accept it.

Thank you!

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

So it wasn't explained yet why we want to expose the setter unconditionally.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

And in which case do we have IsWebAnimationsEnabled returning true, but IsCoreAPIEnabledForCaller returns false?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

https://reviewboard.mozilla.org/r/97406/#review98560

So I don't understand why we'd add web detectable setter which does nothing in some cases.
Is it not possible to use Func with 'inherit attribute CompositeOperation          composite;' ?

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Olli Pettay [:smaug] from comment #17)
> And in which case do we have IsWebAnimationsEnabled returning true, but
> IsCoreAPIEnabledForCaller returns false?

We need the opposite case in some chrome tests that use this setter or other APIs.  We want to run those chrome tests even if IsWebAnimationsEnabled is false.  Can we annotate such thing in webidl?  Something like [Func="..", Chrome]?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

When IsWebAnimationsEnabled returns false, the interface isn't there at all for the given global.
And IsWebAnimationsEnabled returns always true for chrome context.

So I'm still missing something here.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Right!  I thought I did check before, but I did totally miss it.  Hmm, I am confused that what purpose of our IsCoreAPIEnabled() is....

Anyway, I am really sorry about that.  In this bug we don't need any preference checks in this patch, I just confirmed that test cases here runs perfectly with disabling the pref.

Thanks!

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

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

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> Right!  I thought I did check before, but I did totally miss it.  Hmm, I am
> confused that what purpose of our IsCoreAPIEnabled() is....

I meant IsCoreAPIEnabledForCaller() here.  We need IsCoreAPIEnabled() to check the pref is off even it's run on chrome privilege.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

https://reviewboard.mozilla.org/r/97406/#review98836

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66e19bfe6624
Implement KeyframeEffect::SetComposite(). r=boris,smaug

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/66e19bfe6624

Comment 27

[Cornel Ionce [:noni]]

Is there anything here that would require manual testing?

Comment 28

[Cameron McCormack (:heycam)]

No, this should be covered sufficiently by automated testing, thanks!