Closed Bug 1216844 Opened 4 years ago Closed 3 years ago

implement setting effect composition

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox44 --- affected
firefox53 --- fixed

People

(Reporter: heycam, Assigned: hiro)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

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.
Depends on: 1211783
Summary: implement additive animations → implement effect composition (add/accumulate)
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.
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
Depends on: 1300472
Depends on: 1303233
Depends on: 1305325
Depends on: 1291468
Depends on: 1311620
I've decided to implement accumulate and add in a separate bug respectively.

In this bug, setting effect composite will be implemented.
Changing title to match what I will do in this bug.
Summary: implement effect composition (add/accumulate) → implement setting effect composition
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
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).
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.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
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?
Attachment #8816870 - Flags: review?(bugs) → review-
(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 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 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.
Attachment #8816870 - Flags: review?(boris.chiou) → review+
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!
Attachment #8816870 - Flags: review- → review?(bugs)
So it wasn't explained yet why we want to expose the setter unconditionally.
And in which case do we have IsWebAnimationsEnabled returning true, but IsCoreAPIEnabledForCaller returns false?
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;' ?
Attachment #8816870 - Flags: review?(bugs) → review-
(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]?
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.
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!
(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 on attachment 8816870 [details]
Bug 1216844 - Implement KeyframeEffect::SetComposite().

https://reviewboard.mozilla.org/r/97406/#review98836
Attachment #8816870 - Flags: review?(bugs) → review+
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/66e19bfe6624
Implement KeyframeEffect::SetComposite(). r=boris,smaug
https://hg.mozilla.org/mozilla-central/rev/66e19bfe6624
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Is there anything here that would require manual testing?
Flags: needinfo?(cam)
No, this should be covered sufficiently by automated testing, thanks!
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.