1241770 - EffectCompositeOrderComparator should consider same animations in EffectSet in some cases.

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Currently we create new temporary animations in nsAnimationManager::BuildAnimations when animation style is updated.  If EffectCompositeOrderComparator is used while 
EffectSet has both new temporary animation and current animation corresponding to the new animation.  

Fortunately we are not facing such cases in automation tests (and real world?).  But we will face the cases soon once we have the fixes for bug 1166500.  An example is a try result with the fixes[1].

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=15711955&repo=try

We should take case such cases in EffectCompositeOrderComparator.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

As far as I can tell there are two cases:

a) animation index is different
b) the same animation index but AnimationProperty or AnimationEffectTiming in KeyframeEffectReadOnly is different.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: EffectCompositeOrderComparator should check AnimationProperty and AnimationEffectTiming in KeyframeEffectReadOnly

This is a fix for case b).

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Move SetAnimationIndex at last of animation loop in BuildAnimations

If we do it before PlayFromStyle, EffectSet has two animation which has the same index.

This patch makes test_animations-dynamic-changes.html failure at https://dxr.mozilla.org/mozilla-central/rev/66e07ef46853709e3fa91e7c9ad9fe6abf0d5f06/dom/animation/test/css-animations/file_animations-dynamic-changes.html#144

firstAddedAnimation is positioned at the second place in getAnimations' array somehow.
I am now investigating the reason.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Move SetAnimationIndex after calling PlayFromStyle in BuildAnimations v2

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

> This patch makes test_animations-dynamic-changes.html failure at
> https://dxr.mozilla.org/mozilla-central/rev/
> 66e07ef46853709e3fa91e7c9ad9fe6abf0d5f06/dom/animation/test/css-animations/
> file_animations-dynamic-changes.html#144
> 
> firstAddedAnimation is positioned at the second place in getAnimations'
> array somehow.
> I am now investigating the reason.

I found the reason.
The reason is that anim2 in file_animations-dynamic-changes.html has empty keyframe.
Animations have empty keyframe skips at [1], so we need to set animation index before the skip.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp#661

Comment 5

[Brian Birtles (:birtles)]

Can't we just avoid putting two effects with equivalent Animations in the EffectSet at once? That seems like the more correct (and efficient) thing to do here.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thanks! That makes much sense!  I will try it.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away 23~31 Jan, busy 1~9 Feb) from comment #5)
> Can't we just avoid putting two effects with equivalent Animations in the
> EffectSet at once? That seems like the more correct (and efficient) thing to
> do here.

To compare animations (and effects) properly we need to set all properties and timing for the effect before calling SetEffect.  To achieve this we should move a bunch of stuff in BuildAnimations into another KeyframeEffectReadOnly constructor.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> (In reply to Brian Birtles (:birtles, away 23~31 Jan, busy 1~9 Feb) from
> comment #5)
> > Can't we just avoid putting two effects with equivalent Animations in the
> > EffectSet at once? That seems like the more correct (and efficient) thing to
> > do here.
> 
> To compare animations (and effects) properly we need to set all properties
> and timing for the effect before calling SetEffect.  To achieve this we
> should move a bunch of stuff in BuildAnimations into another
> KeyframeEffectReadOnly constructor.

I had overlooked CSS animation's name.  I was going to check the equality of Animations in EffectSet::AddEffect(), but there are other animations which don't have the name in the function.

I am going to do the check in BuildAnimations for now.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > (In reply to Brian Birtles (:birtles, away 23~31 Jan, busy 1~9 Feb) from
> > comment #5)
> > > Can't we just avoid putting two effects with equivalent Animations in the
> > > EffectSet at once? That seems like the more correct (and efficient) thing to
> > > do here.
> > 
> > To compare animations (and effects) properly we need to set all properties
> > and timing for the effect before calling SetEffect.  To achieve this we
> > should move a bunch of stuff in BuildAnimations into another
> > KeyframeEffectReadOnly constructor.
> 
> I had overlooked CSS animation's name.  I was going to check the equality of
> Animations in EffectSet::AddEffect(), but there are other animations which
> don't have the name in the function.
> 
> I am going to do the check in BuildAnimations for now.

It turns out we should refactor CheckAnimationRule and BuildAnimations to avoid creating temporary animations.  I'd want to do such refactoring once we fix bug 1166500, so I'd like to take part 1 and 2 patches here.

Brian, what do you think?

Comment 10

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > > (In reply to Brian Birtles (:birtles, away 23~31 Jan, busy 1~9 Feb) from
> > > comment #5)
> > > > Can't we just avoid putting two effects with equivalent Animations in the
> > > > EffectSet at once? That seems like the more correct (and efficient) thing to
> > > > do here.
> > > 
> > > To compare animations (and effects) properly we need to set all properties
> > > and timing for the effect before calling SetEffect.  To achieve this we
> > > should move a bunch of stuff in BuildAnimations into another
> > > KeyframeEffectReadOnly constructor.
> > 
> > I had overlooked CSS animation's name.  I was going to check the equality of
> > Animations in EffectSet::AddEffect(), but there are other animations which
> > don't have the name in the function.
> > 
> > I am going to do the check in BuildAnimations for now.
> 
> It turns out we should refactor CheckAnimationRule and BuildAnimations to
> avoid creating temporary animations.  I'd want to do such refactoring once
> we fix bug 1166500, so I'd like to take part 1 and 2 patches here.
> 
> Brian, what do you think?

Yes, I think we should avoid creating temporary animations in BuildAnimations (and perhaps create some simplified representation of them?)

I'm a bit concerned about adding part 1. It's a pretty expensive comparison to do every time we sort animations. I'd like to avoid adding that cost if possible.

Do we need *both* parts 1 and 2?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away 23~31 Jan, busy 1~9 Feb) from comment #10)
> > It turns out we should refactor CheckAnimationRule and BuildAnimations to
> > avoid creating temporary animations.  I'd want to do such refactoring once
> > we fix bug 1166500, so I'd like to take part 1 and 2 patches here.
> > 
> > Brian, what do you think?
> 
> Yes, I think we should avoid creating temporary animations in
> BuildAnimations (and perhaps create some simplified representation of them?)
> 
> I'm a bit concerned about adding part 1. It's a pretty expensive comparison
> to do every time we sort animations. I'd like to avoid adding that cost if
> possible.
> 
> Do we need *both* parts 1 and 2?

Yes. And your concern is quite right.  OK. I can go with the refactoring.  We need the refactoring anyway sooner or later.  Now it's the time.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Clearing ni.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Fixed by bug 1242872.