Closed Bug 1225699 Opened 4 years ago Closed 4 years ago

Store animation effects on their target element

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(7 files, 2 obsolete files)

Background: This is part of bug 1190235, which we need for implementing Element.animate (bug 1096773).

We want to store the list of animation effects targeting an element on that element for the time they are current or in effect. This will mean:

* We can support keep script-generated animations alive while they are active. Unlike CSS Animations and CSS Transitions, script-generated animations are not bound to markup and so there's nothing to keep them alive.

* We can support CSS Animations and Transitions that have been manipulated by script to point to a different target element.

* We have a single place to gather and sort all the animation effects applying to an element so we can efficiently implement prioritization (e.g. CSS Transitions / CSS Animation overlap) and, in future, addition without having to walk across three separate lists of animations and manage the interaction of three different managers.

* In future, we can support animation groups where we have a 1:n relationship between animations and target elements.

I plan to cover most of attachment 8681123 [details] [diff] [review] in this bug but some parts might get spun off.
Hi Olli, I thought you might want to check these (at least parts 1~3) over
since you've been fixing this code recently and I want to be sure I'm not
adding any new cycles.

The arrangement is that:

* We add a hashset that gets stored as a property on elements targetted by an
  animation.
* The hashset points to the *effects* that target that element (for reasoning,
  see comment 0).
* The hashset uses non-owning pointers because the effects (specifically
  KeyframeEffectReadOnly objects) they point to, have a strong reference back to
  their target element.
* The KeyframeEffectReadOnly objects are responsible for removing themselves
  from the hashset when they release the pointer to their target element.

Since we're only adding non-owning references I don't think this introduces any
new cycles but you might have other suggestions too regarding the use of
properties--I think you had some concerns about that.
Attachment #8688849 - Flags: review?(bugs)
Adding Cameron as well to check the use of IsRelevant() here.
Attachment #8688851 - Flags: review?(cam)
Attachment #8688851 - Flags: review?(bugs)
Adding njn to CC since he may have a comment on the next patch (part 4).
I spoke to Nicholas about doing this more generically but he didn't seem keen
(see bug 1149833 comment 9 and bug 1149833 comment 18).

I think it's still worthwhile for this case. In upcoming patches (see attachment
8681123 [review]) we iterate over this a lot, and being able to use range-based for loops
makes that code a lot more simple.

Regarding the implementation, it could probably be tweaked to, for example, use
Maybe<> to store the base Iterator so that to construct the EndIterator we don't
have to pass in a base Iterator. I'm not sure if it's worth it, however, since
we'd have to add more checks to see if the value was set or not.

In fact, we could probably tweak the implementation a few ways, but firstly I'm
just wondering if this is ok or not.
Attachment #8688853 - Flags: review?(cam)
Comment on attachment 8688849 [details] [diff] [review]
part 1 - Add EffectSet class


>+/* static */ void
>+EffectSet::PropertyDtor(void *aObject, nsIAtom *aPropertyName,
>+                        void *aPropertyValue, void *aData)
* goes with the type, not with argument name. Same also elsewhere.


>+
>+  if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
>+    aElement->SetMayHaveAnimations();
>+  }
It is not clear to me why we don't use the flag for all the cases. When I was looking at the code last week, it looks like that
flag could be easily set whenever there is some animation, even for pseudo element, and that would speed up things, among others 
UnbindFromTree (which wouldn't need to delete properties unless there actually are some animations.)
But I guess that isn't about this bug.

>+  static EffectSet* GetEffectSet(dom::Element* aElement,
>+                                    nsCSSPseudoElements::Type aPseudoType);
align params here and elsewhere


This bug doesn't still tell what EffectSet is for, but I assume that becomes clear from the next patch.
Attachment #8688849 - Flags: review?(bugs) → review+
Comment on attachment 8688850 [details] [diff] [review]
part 2 - Add Add/RemoveEffect to EffectSet

>+  // We store a raw (non-owning) pointer here because KeyframeEffectReadOnly
>+  // keeps a strong reference to the target element where this object is
>+  // stored. KeyframeEffectReadOnly is responsible for removing itself from
>+  // this set when it releases its reference to the target element.
>+  typedef nsTHashtable<nsPtrHashKey<dom::KeyframeEffectReadOnly>> EffectHashSet;
Just curious, why animations code uses typedef so often.
Typedeffing (when the new name doesn't hint about ownership model) tends to make reviewing harder since one needs to always 
check what the type is about.
While I was debugging, it was rather odd to see AnimationSet some times meaning a set which has strong refs (PendingAnimationTracker)
and some times raw refs (AnimationTimeline). (now they both use strong refs)
Attachment #8688850 - Flags: review?(bugs) → review+
Attachment #8688851 - Flags: review?(bugs) → review+
Comment on attachment 8688849 [details] [diff] [review]
part 1 - Add EffectSet class

>+  nsresult rv = aElement->SetProperty(propName, effectSet,
>+                                      &EffectSet::PropertyDtor, false);
oh, hmm, how is this all supposed to work when an element is moved to another document?
The last param (false) is about that. Does the spec define that case?
(In reply to Olli Pettay [:smaug] from comment #7)
> >+
> >+  if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> >+    aElement->SetMayHaveAnimations();
> >+  }
> It is not clear to me why we don't use the flag for all the cases. When I
> was looking at the code last week, it looks like that
> flag could be easily set whenever there is some animation, even for pseudo
> element, and that would speed up things, among others 
> UnbindFromTree (which wouldn't need to delete properties unless there
> actually are some animations.)
> But I guess that isn't about this bug.

Filed bug 1226091 for that.

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8688849 [details] [diff] [review]
> part 1 - Add EffectSet class
> 
> >+  nsresult rv = aElement->SetProperty(propName, effectSet,
> >+                                      &EffectSet::PropertyDtor, false);
> oh, hmm, how is this all supposed to work when an element is moved to
> another document?
> The last param (false) is about that. Does the spec define that case?

That's a really good question.

Firstly, I realized yesterday that actually we do want EffectSet to keep strong references. That's because when we do:

  elem.animate({ opacity: 0 }, 1000)

This generates an Animation object with an attached KeyframeEffect object. Something has to keep those objects alive until the animation finishes.

The plan was to have:

  elem --(strong)--> KeyframeEffect --(strong)--> Animation

(The elem--(strong)-->Animation reference is only present for CSS Transitions/CSS Animations and the identity of 'elem' in that case is not always the target element.)

When I wrote these patches, however, I was so concerned about not adding more cycles that I convinced myself we only needed a weak reference from elem to effect, but that's wrong.

So if we add a strong reference from target element to effect, we need to work out in what circumstances we can release that reference.

The spec only requires us to keep that reference as long as the animation is actually affecting the target element or is scheduled to do so. That means, we can release it when:

  * the animation finishes if it is not filling forwards
  * the animation is cancelled
  * the animation is not started to begin with
  * the animation has its fill-mode changed to 'none' or 'backwards' while it is finished

In other words, if the animation fills forwards we can never release it. I wanted to add requirements to the spec about when implementations can drop these forwards-filling animations but Google resisted, saying just let authors deal with the problem.

I'm not really happy with that but I'm also not sure what to do. The best I can think of is adding a step to collapse multiple forwards-filling animations targeting the same element.

I'm not sure if we should also add a condition that says that animations are cancelled when an element is unbound from the tree or moved to a different document (and hence could be released).

For the former--unbinding--I suspect animations should not be cancelled. At least our experience with SVG SMIL was that authors expected to be able to move animated elements around and have the animations continue to run. Also, I can imagine authors creating new DOM elements, triggering a fade-in animation on them, then attaching them to the tree, expecting them to fade in.

For changing documents, however, given that by default animations get their timing from the default document timeline, perhaps we should spec that they are cancelled and even have their timeline updated to reflect the new document timeline? I guess I need to post to the list about this.
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8688850 [details] [diff] [review]
> part 2 - Add Add/RemoveEffect to EffectSet
> 
> >+  // We store a raw (non-owning) pointer here because KeyframeEffectReadOnly
> >+  // keeps a strong reference to the target element where this object is
> >+  // stored. KeyframeEffectReadOnly is responsible for removing itself from
> >+  // this set when it releases its reference to the target element.
> >+  typedef nsTHashtable<nsPtrHashKey<dom::KeyframeEffectReadOnly>> EffectHashSet;
> Just curious, why animations code uses typedef so often.
> Typedeffing (when the new name doesn't hint about ownership model) tends to
> make reviewing harder since one needs to always 
> check what the type is about.
> While I was debugging, it was rather odd to see AnimationSet some times
> meaning a set which has strong refs (PendingAnimationTracker)
> and some times raw refs (AnimationTimeline). (now they both use strong refs)

Fair point. I find it easier to read since the logic tends to fit on one line ('nsTHashtable<nsPtrHashKey<dom::KeyframeEffectReadOnly>>' alone takes up 55 characters out of 80) but that's a fair point about reviewing. I'm happy to drop the typedef here if you prefer. Or rename it to WeakEffectSet?
(In reply to Brian Birtles (:birtles) from comment #10)
> For changing documents, however, given that by default animations get their
> timing from the default document timeline, perhaps we should spec that they
> are cancelled and even have their timeline updated to reflect the new
> document timeline? I guess I need to post to the list about this.

Posted to the list about the more complex issue of how to discard filling animations. Once we work that out, we can discuss how to handle changing documents.

  https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0029.html
  CC: https://lists.w3.org/Archives/Public/www-style/2015Nov/0270.html
(In reply to Olli Pettay [:smaug] from comment #9)
> >+  nsresult rv = aElement->SetProperty(propName, effectSet,
> >+                                      &EffectSet::PropertyDtor, false);
> oh, hmm, how is this all supposed to work when an element is moved to
> another document?
> The last param (false) is about that. Does the spec define that case?

Thinking about this a bit more, I think we actually want to persist the animation when the target element changes document. Trying to do anything else like stop the animation or update its timeline would be particularly odd if we eventually support group effects where child effects may target different elements. So I guess the last parameter should be true here. I'm not sure what the implications are of that, however.
Blocks: 1226118
WeakEffectSet, or perhaps EffectPtrSet?
(Weak means usually weak pointers, not raw pointers)
and sounds like animations should stays alive even after unbind, so Element's traverse/unlink implementation need to traverse/unlink also the properties.
So something similar to what 
NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(FragmentOrElement) does with 
HTMLSVGPropertiesToTraverseAndUnlink.


Can one add animations to Elements which aren't in document? If so, that would be another
reason to traverse the properties.
Attachment #8688851 - Flags: review?(cam) → review+
Comment on attachment 8688853 [details] [diff] [review]
part 4 - Add iterator to EffectSet

Review of attachment 8688853 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.  I was going to suggest making operator* const, but you don't have cbegin/cend methods so you can't iterate over a const EffectSet anyway so it doesn't matter too much.
Attachment #8688853 - Flags: review?(cam) → review+
Comment on attachment 8688855 [details] [diff] [review]
part 5 - Use EffectSet in Element::GetAnimations

Review of attachment 8688855 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeEffect.h
@@ +257,5 @@
>    bool IsCurrent() const;
>    bool IsInEffect() const;
>  
>    void SetAnimation(Animation* aAnimation);
> +  Animation* GetAnimation() { return mAnimation; }

const?
Attachment #8688855 - Flags: review?(cam) → review+
Attachment #8691244 - Flags: review?(bugs)
This is so that when we have code like:

  elem.animate({ opacity: 0 }, 1000)

the resulting animation is kept alive by |elem| based on the following
ownership chain:

  elem --(strong)--> KeyframeEffectReadOnly --(strong)--> Animation

Now, there is an ownership cycle introduced here because KeyframeEffectReadOnly
objects also store owning references to their target elements. This is broken
when the Animation finishes (if it does not fill forwards) or is cancelled
since either event will trigger a call to
KeyframeEffectReadOnly::UpdateTargetRegistration.

If the Animation fills forwards, the resource will not be released until
it is cancelled. For Animations corresponding to CSS Animations / CSS
Transitions this happens when the Element is unbound or when the corresponding
style property is updated causing the animation to be replaced or removed.

For the general case of script-generated animations, however, this cycle won't
be broken until the Element is unbound and all other references to the
Animation or KeyframeEffectReadOnly are dropped.

It's unfortunate that we can't more aggressively prune these objects but it's
what the spec currently says. I've posted to the mailing list[1] about this but
have yet to find a good solution.

[1] https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0029.html
Attachment #8691246 - Flags: review?(bugs)
Comment on attachment 8691246 [details] [diff] [review]
part 8 - Use strong refs to store effects on their target elements

Clearing this review request since try shows this is leaking.
Attachment #8691246 - Flags: review?(bugs)
Comment on attachment 8691244 [details] [diff] [review]
part 6 - Traverse and unlink effect properties

(In reply to Brian Birtles (:birtles) from comment #22)
> Comment on attachment 8691246 [details] [diff] [review]
> part 8 - Use strong refs to store effects on their target elements
> 
> Clearing this review request since try shows this is leaking.

Presumably because I didn't add Traverse/Unlink to EffectSet itself which I probably should do in part 6.
Attachment #8691244 - Flags: review?(bugs)
Attachment #8691245 - Flags: review?(bugs) → review+
Attachment #8691244 - Attachment is obsolete: true
Attachment #8691246 - Attachment is obsolete: true
Attachment #8691668 - Flags: review?(bugs)
Comment on attachment 8691668 [details] [diff] [review]
part 6 - Use strong refs to store effects on their target elements

>+EffectSet::Traverse(nsCycleCollectionTraversalCallback& aCallback)
>+{
>+  for (auto iter = mEffects.Iter(); !iter.Done(); iter.Next()) {
>+    aCallback.NoteXPCOMChild(iter.Get()->GetKey());
>+  }
I'd perhaps use CycleCollectionNoteChild here and not raw aCallback.NoteXPCOMChild.
Passing aCallback.Flags() to its flags param
and "EffectSet::mEffects[]" or some such as name.
That would just help reading cycle collection logs.
But either way. HTMLSVGPropertiesToTraverseAndUnlink uses NoteXPCOMChild, so your code is fine too.


+EffectSet::GetEffectSetPropertyAtoms()
+{
+  static nsIAtom* effectSetPropertyAtoms[] =
+    {
+      nsGkAtoms::animationEffectsProperty,
+      nsGkAtoms::animationEffectsForBeforeProperty,
+      nsGkAtoms::animationEffectsForAfterProperty,
+      nullptr
+    };
+
+  return effectSetPropertyAtoms;
+}
Ah, this works because the method is called first time after nsGkAtoms has been initialized.
HTMLSVGPropertiesToTraverseAndUnlink uses static array outside the method, so it needs to refer to the location of
atoms.
Attachment #8691668 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.