Closed Bug 1249219 Opened 8 years ago Closed 8 years ago

Animation mutation observers do not support animations on pseudo elements

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

Details

Attachments

(8 files, 17 obsolete files)

5.32 KB, patch
birtles
: review+
Details | Diff | Splinter Review
6.90 KB, patch
birtles
: review+
Details | Diff | Splinter Review
2.13 KB, patch
boris
: review+
Details | Diff | Splinter Review
4.87 KB, patch
boris
: review+
Details | Diff | Splinter Review
12.94 KB, patch
boris
: review+
Details | Diff | Splinter Review
2.90 KB, patch
boris
: review+
Details | Diff | Splinter Review
12.79 KB, patch
boris
: review+
Details | Diff | Splinter Review
4.89 KB, patch
boris
: review+
Details | Diff | Splinter Review
We currently only dispatch animation mutation events for animations running on elements, not pseudo elements. This is enforced in places like nsNodeUtils::GetTargetForAnimation[1].

Before DevTools can treat animations on pseudo elements like they do other animations, we need to fix the mutation observer mechanism to support pseudo elements too.

Even if we don't allow using a pseudo element as the target for listening to mutation events, if we register as an observer on an element with subtree:true, we should get records for animations on its pseudo elements and pseudo elements on its descendants.

[1] https://dxr.mozilla.org/mozilla-central/rev/0629918a09ae87808efdda432d7852371ba37db6/dom/base/nsNodeUtils.cpp#230
Assignee: nobody → boris.chiou
Depends on: 1123523
Hi Brian,

I am thinking how to use mutation observer for CSSPseudoElement. According to the document [1], the API of MutationObserver.observe() is

void observe(
  Node target,
  MutationObserverInit options
);

The target is "Node", so our current implementation for the target of nsDOMMutationObserver is |nsINode|. In order to re-use the API and implement it for CSSPseudoElement, we should make CSSPseudoElement inherit from nsINode. Therefore, if we get the pseudo element target from document.getAnimations() and animation.effect.target, we can use the API of MutationObserver.observe() directly. Does it make sense? So we can use the mSlots in nsINode, instead of implementing a independent one for CSSPseudoElement.

[1] https://developer.mozilla.org/en/docs/Web/API/MutationObserver
Flags: needinfo?(bbirtles)
(In reply to Boris Chiou [:boris]  from comment #1)
> Hi Brian,
> 
> I am thinking how to use mutation observer for CSSPseudoElement. According
> to the document [1], the API of MutationObserver.observe() is
> 
> void observe(
>   Node target,
>   MutationObserverInit options
> );
> 
> The target is "Node", so our current implementation for the target of
> nsDOMMutationObserver is |nsINode|. In order to re-use the API and implement
> it for CSSPseudoElement, we should make CSSPseudoElement inherit from
> nsINode. Therefore, if we get the pseudo element target from
> document.getAnimations() and animation.effect.target, we can use the API of
> MutationObserver.observe() directly. Does it make sense? So we can use the
> mSlots in nsINode, instead of implementing a independent one for
> CSSPseudoElement.
> 
> [1] https://developer.mozilla.org/en/docs/Web/API/MutationObserver

Or just as comment 0 said, we don't make CSSPseudoElement inherit from nsINode, and only for element with subtree:true. So the input is still a dom::Element (the parentElement of this CSSPseudoElement). I'm not sure which one is more acceptable.
(In reply to Boris Chiou [:boris]  from comment #2)
> Or just as comment 0 said, we don't make CSSPseudoElement inherit from
> nsINode, and only for element with subtree:true. So the input is still a
> dom::Element (the parentElement of this CSSPseudoElement). I'm not sure
> which one is more acceptable.

If so, we have to implement mSlots and some mutation observer related functions (e.g. AddAnimationObserver()) in CSSPseudoElement. I'd try this way first. (So remove the ni.) Thanks.
Flags: needinfo?(bbirtles)
Hi Boris, this bug isn't necessarily about being to use CSSPseudoElement with MutationObserver.observe(). It's more about allowing DevTools, which uses the chrome-only animation observer mechanism, to get notifications when they observe an Element that has pseudo elements.

I think we need to fix bug 1254418 first. That bug probably doesn't have much to do with CSSPseudoElement and this bug might not involve CSSPseudoElement either. Instead, that's just about making Element.getAnimations() return the right thing when called on a generated-content element. Depending on how that bug works out, this bug will also likely about observing generated-content elements, as opposed to CSSPseudoElements.

In the long-term, we might be able to make the deep-tree walker return CSSPseudoElements for generated content elements, but I think there is probably more things to add to CSSPseudoElement before we can do that.
(In reply to Brian Birtles (:birtles) from comment #4)
> Hi Boris, this bug isn't necessarily about being to use CSSPseudoElement
> with MutationObserver.observe(). It's more about allowing DevTools, which
> uses the chrome-only animation observer mechanism, to get notifications when
> they observe an Element that has pseudo elements.

I see. Thanks.

> 
> I think we need to fix bug 1254418 first. That bug probably doesn't have
> much to do with CSSPseudoElement and this bug might not involve
> CSSPseudoElement either. Instead, that's just about making
> Element.getAnimations() return the right thing when called on a
> generated-content element. Depending on how that bug works out, this bug
> will also likely about observing generated-content elements, as opposed to
> CSSPseudoElements.

OK, it makes sense. Let me take a look at bug 1254418 first.
Just thinking about this a bit more. Alternatively, we could try and make the deep tree walker return CSSPseudoElement objects. I think, to do this, we'd need to:

* Make CSSPseudoElement inherit from nsINode
* For all methods we currently use on the generated-content Element, make CSSPseudoElement look up the generated-content element and forward the calls to it?
* Get the deep tree walker to detect the special case where a generated-content Element can be represented by a CSSPseudoElement object (only ::before and ::after currently) and create/fetch a CSSPseudoElement in this case?

I haven't looked into this to see how involved it is, but this might be nicer?
(In reply to Brian Birtles (:birtles) from comment #6)
> Just thinking about this a bit more. Alternatively, we could try and make
> the deep tree walker return CSSPseudoElement objects. I think, to do this,
> we'd need to:

I think this idea is for bug 1254418, right?

> 
> * Make CSSPseudoElement inherit from nsINode

Yes, this will make things simpler if we want to let animation mutation observer support CSSPseudoElement.

> * For all methods we currently use on the generated-content Element, make
> CSSPseudoElement look up the generated-content element and forward the calls
> to it?

This sounds good. Actually, I'm not familiar with those methods we used on generated-content Element. I should check this.

> * Get the deep tree walker to detect the special case where a
> generated-content Element can be represented by a CSSPseudoElement object
> (only ::before and ::after currently) and create/fetch a CSSPseudoElement in
> this case?
> 
> I haven't looked into this to see how involved it is, but this might be
> nicer?

Yes, building the relationship between generated-content Element and CSSPseudoElement is a good idea in the deep tree walker, let me trace the code more for this.

In conclusion, the idea is:
1. Check the generated-content element, and then create/fetch a CSSPseudoElement for it.
2. So we can use the returned Element or CSSPseudoElement to call GetAnimation() in deep walker. (As Patrick's method in the mail thread)
In bug 1254418, we want to support Element.getAnimations() for generated-content element, but the idea here supports another way for DevTools to use the deep walker to call the correct Element/CSSPseudoElement.getAnimations(). This makes sense and we don't have to change the implementation of Element.getAnimations.
(In reply to Boris Chiou [:boris]  from comment #7)
> (In reply to Brian Birtles (:birtles) from comment #6)
> > * Make CSSPseudoElement inherit from nsINode
> 
> Yes, this will make things simpler if we want to let animation mutation
> observer support CSSPseudoElement.

Making CSSPseudoElement inherit from nsINode may be not enough because we change the type between nsIDOMNode and nsINode frequently in inDeepTreeWalker, and the APIs of inDeepTreeWalker use nsIDOMNode as the node type, so I think things become complicated if we want to try this idea.
Status: NEW → ASSIGNED
Hi, Cameron,

I'm looking at a test case about animation observer [1] (which is changing the duration of a finished animation), and I want to make sure if the nsAutoAnimationMutationBatch is correct.
In that test case [1], I found we call nsAutoAnimationMutationBatch::AnimationAdded() first, and then call nsAutoAnimationMutationBatch::AnimationChanged(). The "entry->mState" is "eState_Added", and "the entry->mChanged" is true [2] in this case because we add the animation first and then call KeyframeEffect::NotifySpecifiedTimingUpdated.
However, nsAutoAnimationMutationBatch::Done() adds/changes/removes this animation to a list per an entry [3], so we will lose the "AnimationChanged" record. Is it correct in nsAutoAnimationMutationBatch? Thanks.

[1] https://dxr.mozilla.org/mozilla-central/rev/5e14887312d4523ab59c3f6c6c94a679cf42b496/dom/animation/test/chrome/test_animation_observers.html#1498
[2] https://dxr.mozilla.org/mozilla-central/rev/5e14887312d4523ab59c3f6c6c94a679cf42b496/dom/base/nsDOMMutationObserver.h#837
[3] https://dxr.mozilla.org/mozilla-central/rev/5e14887312d4523ab59c3f6c6c94a679cf42b496/dom/base/nsDOMMutationObserver.cpp#1140
Flags: needinfo?(cam)
Yes, this behaviour was intentional.  The author using the MutationObserver shouldn't need to care about the change to the animation if they are already handling the AnimationAdded record, since to the author it just looks like the animation has been added with the values it has after the modification.

Similarly, if you change an animation and then immediately remove it then you can coalesce those two notifications into just an AnimationRemoved (since script shouldn't need to care about the change if it will just handle the removal).
Flags: needinfo?(cam)
Got it. Thanks.
Use Pair<Element*, CSSPseudoElementType> as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)
Attachment #8730564 - Attachment is obsolete: true
Merge AnimationAdded, AnimationChanged, and AnimationRemoved into one function,
AnimationMutated.
Attachment #8731116 - Attachment is obsolete: true
We use the parent element of a pseudo element as the subject to be notified.

Usage:
We record animations targeting to pseudo elements by setting subtree attribute
to true. For example:
MutationObserver(Element*, { subtree: ture });
So all the animations targeting to the pseudo elements whose parentElement is
the first argument will be recorded.
Attachment #8731117 - Attachment is obsolete: true
Attachment #8731142 - Attachment is obsolete: true
Attachment #8731139 - Flags: review?(bbirtles)
Attachment #8731140 - Flags: review?(bbirtles)
Attachment #8731141 - Flags: review?(bbirtles)
Attached patch Part 4: Test (v3) (obsolete) — Splinter Review
Add tests in test_animation_observers.html, so we can test elements and pseudo
elements together by setting subtree.
Attachment #8731156 - Flags: review?(bbirtles)
Attachment #8731151 - Attachment is obsolete: true
(In reply to Boris Chiou [:boris]  from comment #21)
> Created attachment 8731156 [details] [diff] [review]
> Part 4: Test (v3)
> 
> Add tests in test_animation_observers.html, so we can test elements and
> pseudo
> elements together by setting subtree.

For this patch (test case), I know we want to standardize animation tests by testharness.js where possible, but I'd like to reuse some tests in test_animation_observer.html (, so we can test normal elements and pseudo elements together). If you want, I can add a new test file to do some basic tests by testharness.js.
Comment on attachment 8731139 [details] [diff] [review]
Part 1: Use Element, CSSPseudoElementType pair as the returned value of GetTargetForAnimation

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

Sorry for the delay!

::: dom/animation/KeyframeEffect.h
@@ +207,5 @@
>  
>    void GetTarget(Nullable<OwningElementOrCSSPseudoElement>& aRv) const;
> +  // This GetTarget() can get an |Element, CSSPseudoElement| pair without
> +  // creating a CSSPseudoElement object.
> +  Pair<Element*, CSSPseudoElementType> GetTarget() const

I think Pair<> is ok if it's not used too widely, but we are going to be using this type here, in nsNodeUtils and the DOMMutationObserver too so I think we should just make our own type: AnimationTarget.

It can be a simple struct with mElement and mPseudoType members. We should default mElement to nullptr and mPseudoType to CSSPseudoElementType::NotPseudo. We can probably use {} initializer syntax to make it easy to create without needing a constructor.

We'll need to add a comment explaining that mElement represents the parent element of a pseudo-element not the generated content element.

Also, for the case where mTarget is nullptr, I think it would be better to return a Maybe<> object instead.

i.e. Maybe<AnimationTarget> GetTarget() const
Attachment #8731139 - Flags: review?(bbirtles)
Comment on attachment 8731140 [details] [diff] [review]
Part 2: Merge nsNodeUtils::AnimationAdded/Changed/Removed

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

This seems fine to me, with comments addressed. In particular, I think we should restore AnimationAdded/AnimationChanged/AnimationRemoved and make them call AnimationMutated (and revert the changes to Animation.cpp/nsAnimationManager.cpp/KeyframeEffect.cpp).

::: dom/base/nsNodeUtils.cpp
@@ +257,2 @@
>  
> +  Element* ele = target.first();

'elem' is a more common abbreviation.

::: dom/base/nsNodeUtils.h
@@ +145,5 @@
>  
> +  /**
> +   * Get target for a specific animation
> +   * @param aAnimation The animation whose target is what we want.
> +   */

Nit: We should push this change (the added comment) to part 1.

@@ -146,5 @@
>    static mozilla::Pair<mozilla::dom::Element*, mozilla::CSSPseudoElementType>
>      GetTargetForAnimation(const mozilla::dom::Animation* aAnimation);
> -  static void AnimationAdded(mozilla::dom::Animation* aAnimation);
> -  static void AnimationChanged(mozilla::dom::Animation* aAnimation);
> -  static void AnimationRemoved(mozilla::dom::Animation* aAnimation);

Can we keep these methods? It makes the call sites shorter and easier to read.

Can we just make AnimationMutated a private method that these methods wrap?

@@ +155,5 @@
> +   * @param aAnimation The mutated animation.
> +   * @param aMutatedType The mutation type of this animation. It could be
> +   *                     Added, Changed, or Removed.
> +   */
> +  enum class AnimationMutatedType {

s/Mutated/Mutation/
Attachment #8731140 - Flags: review?(bbirtles)
Comment on attachment 8731141 [details] [diff] [review]
Part 3: Support pseudo elements in Animation Mutation Observer

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

This seems reasonable to me but I should defer to Cameron on this one.

::: dom/base/nsDOMMutationObserver.cpp
@@ +400,5 @@
>    }
>  
> +  // Record animations targeting to a pseudo element only when subtree is true.
> +  if ((animationTarget.second() == CSSPseudoElementType::before ||
> +       animationTarget.second() == CSSPseudoElementType::after) &&

Could we just check that the pseudoType is *not* NoPseudo?
Attachment #8731141 - Flags: review?(bbirtles) → review?(cam)
Comment on attachment 8731156 [details] [diff] [review]
Part 4: Test (v3)

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

This looks good but could be a little more simple?

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1417,5 @@
> +  //   childA   childB  childC
> +  //         (::before) (::after)
> +  //         (::after)
> +  //              |
> +  //            childD(::before)

This seems more complicated that it needs to be. Wouldn't the following graph cover every possibility?

    div
 (::before)
 (::after)
  /     \
childA childB

@@ +1536,4 @@
>  addAsyncAnimTest("change_duration_and_currenttime",
>                   { observe: div, subtree: true }, function*() {
> +  // Change pseudo element and its parent element in the same time
> +  // to make sure we got different records.

Rather than making this test more complex, could be write a similar test for pseudo elements?

Alternative, I wonder if we even need to test this? Perhaps the ordering test above is enough? What do you think?
Attachment #8731156 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #23)
> Sorry for the delay!

Never mind. Thanks for your review.

> I think Pair<> is ok if it's not used too widely, but we are going to be
> using this type here, in nsNodeUtils and the DOMMutationObserver too so I
> think we should just make our own type: AnimationTarget.
> 
> It can be a simple struct with mElement and mPseudoType members. We should
> default mElement to nullptr and mPseudoType to
> CSSPseudoElementType::NotPseudo. We can probably use {} initializer syntax
> to make it easy to create without needing a constructor.

About AnimationTarget struct, I am thinking that could we reuse the struct, PseudoElementHashKey [1].
For example:
1. Rename PseudoElementHashKey to AnimationTarget, and move it to a specific header file (e.g. Animation.h)
2. We can use this struct for all the functions who want to use this struct. (e.g. PseudoElementHash, nsNodeUtils, KeyframeEffect)
3. Use this struct to replace Pair<Element*, CSSPseudoElementType> in other places, e.g. EffectCompositor::GetAnimationElementAndPseudoForFrame() [2].

Does it make sense? Thanks.

[1] https://dxr.mozilla.org/mozilla-central/rev/341344bdec8f10bf50646cd6ef2355361435cbf6/dom/animation/PseudoElementHashEntry.h#18
[2] https://dxr.mozilla.org/mozilla-central/rev/341344bdec8f10bf50646cd6ef2355361435cbf6/dom/animation/EffectCompositor.h#188
(In reply to Boris Chiou [:boris]  from comment #27)
> About AnimationTarget struct, I am thinking that could we reuse the struct,
> PseudoElementHashKey [1].
> For example:
> 1. Rename PseudoElementHashKey to AnimationTarget, and move it to a specific
> header file (e.g. Animation.h)
> 2. We can use this struct for all the functions who want to use this struct.
> (e.g. PseudoElementHash, nsNodeUtils, KeyframeEffect)
> 3. Use this struct to replace Pair<Element*, CSSPseudoElementType> in other
> places, e.g. EffectCompositor::GetAnimationElementAndPseudoForFrame() [2].
> 
> Does it make sense? Thanks.

Yeah, that sounds good. Let's call it NonOwningAnimationTarget so that it's obvious that the pointer member does not hold a reference.

Regarding what header file to use, we could either add it to KeyframeEffect.h, or put it in a separate file: NonOwningAnimationTarget.h. I guess a separate file is better here.

Thanks for taking care of this.

Also, I forgot to explain the reason *why* a type is better than Pair. It's simply that with Pair<>, the meaning of first() and second() is not obvious if the type is used in many different places.
(In reply to Brian Birtles (:birtles) from comment #24)
> > -  static void AnimationAdded(mozilla::dom::Animation* aAnimation);
> > -  static void AnimationChanged(mozilla::dom::Animation* aAnimation);
> > -  static void AnimationRemoved(mozilla::dom::Animation* aAnimation);
> 
> Can we keep these methods? It makes the call sites shorter and easier to
> read.
> 
> Can we just make AnimationMutated a private method that these methods wrap?

Sure. I can restore them and add a private method to reduce the redundant code. Thanks for your suggestion.
(In reply to Brian Birtles (:birtles) from comment #28)
> Also, I forgot to explain the reason *why* a type is better than Pair. It's
> simply that with Pair<>, the meaning of first() and second() is not obvious
> if the type is used in many different places.

I agree. I also don't like to use first() and second(). :)
(In reply to Brian Birtles (:birtles) from comment #26)
> This seems more complicated that it needs to be. Wouldn't the following
> graph cover every possibility?
> 
>     div
>  (::before)
>  (::after)
>   /     \
> childA childB

I would like to add one more animation to childA or childB to make sure we get records for animations on pseudo elements on its descendants.

> 
> @@ +1536,4 @@
> >  addAsyncAnimTest("change_duration_and_currenttime",
> >                   { observe: div, subtree: true }, function*() {
> > +  // Change pseudo element and its parent element in the same time
> > +  // to make sure we got different records.
> 
> Rather than making this test more complex, could be write a similar test for
> pseudo elements?
> 
> Alternative, I wonder if we even need to test this? Perhaps the ordering
> test above is enough? What do you think?

I'd like to a new similar test for pseudo elements because I want to make sure we record animations for either elements or pseudo elements while changing timing properties. (e.g. AnimationChanged() in KeyframeEffect::NotifySpecifiedTimingUpdated works in both case because I revise the code in part 3.)
(In reply to Brian Birtles (:birtles) from comment #23)
> It can be a simple struct with mElement and mPseudoType members. We should
> default mElement to nullptr and mPseudoType to
> CSSPseudoElementType::NotPseudo. We can probably use {} initializer syntax
> to make it easy to create without needing a constructor.

I'd like to add the struct like this:
struct NonOwningAnimationTarget {
  // comment for mElement
  dom::Element* mElement = nullptr;
  CSSPseudoElementType mPseudoType = CSSPseudoElementType::NotPseudo;

  NonOwningAnimationTarget(dom::Element* aElement, CSSPseudoElementType aType): mElement(aElement), mPseudoType(aType) { }
}

I still have to add a constructor of two arguments because we use member initialization. It cannot support this aggregate initialization [1], {element, pseudoType}, unless we define a constructor for it.

[1] http://en.cppreference.com/w/cpp/language/aggregate_initialization
(In reply to Boris Chiou [:boris]  from comment #32)
> (In reply to Brian Birtles (:birtles) from comment #23)
> > It can be a simple struct with mElement and mPseudoType members. We should
> > default mElement to nullptr and mPseudoType to
> > CSSPseudoElementType::NotPseudo. We can probably use {} initializer syntax
> > to make it easy to create without needing a constructor.
> 
> I'd like to add the struct like this:
> struct NonOwningAnimationTarget {
>   // comment for mElement
>   dom::Element* mElement = nullptr;
>   CSSPseudoElementType mPseudoType = CSSPseudoElementType::NotPseudo;
> 
>   NonOwningAnimationTarget(dom::Element* aElement, CSSPseudoElementType
> aType): mElement(aElement), mPseudoType(aType) { }
> }
> 
> I still have to add a constructor of two arguments because we use member
> initialization. It cannot support this aggregate initialization [1],
> {element, pseudoType}, unless we define a constructor for it.

Very interesting! You're right! I suppose we need to do that then. I think that even once we add a constructor we can still use the { } notation to create an object using the new uniform C++ initialization[1] so we won't have to update all the users of PseudoElementHashKey.

[1] https://en.wikipedia.org/wiki/C%2B%2B11#Uniform_initialization
NonOwningAnimationTarget is a struct made of two members:
1. mozilla::dom::Element*
2. mozilla::CSSPseudoElementType
Attachment #8731632 - Flags: review?(bbirtles)
Attachment #8731139 - Attachment is obsolete: true
Attachment #8731140 - Attachment is obsolete: true
We use NonOwningAnimationTarget as the hash key.
Attachment #8731633 - Flags: review?(bbirtles)
Use NonOwningAnimationTarget as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)
Attachment #8731636 - Flags: review?(bbirtles)
Remove the duplicated code in nsNodeUtils.
Attachment #8731637 - Flags: review?(bbirtles)
We use the parent element of a pseudo element as the subject to be notified.

Usage:
We record animations targeting to pseudo elements by setting subtree attribute
to true.

MutationObserver(Node, { subtree: ture });

So all the animations targeting to the pseudo elements whose parentElement is
the first argument will be recorded.
Attachment #8731639 - Flags: review?(cam)
Attachment #8731141 - Attachment is obsolete: true
Attachment #8731141 - Flags: review?(cam)
Attachment #8731156 - Attachment is obsolete: true
Comment on attachment 8731632 [details] [diff] [review]
Part 1: Define NonOwningAnimationTarget

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

r=me with comments addressed.

::: dom/animation/NonOwningAnimationTarget.h
@@ +18,5 @@
> +struct NonOwningAnimationTarget
> +{
> +  // mElement represents the parent element of a pseudo-element, not the
> +  // generated content element.
> +  dom::Element* mElement = nullptr;

Perhaps we should annotate this MOZ_NON_OWNING_REF.

@@ +22,5 @@
> +  dom::Element* mElement = nullptr;
> +  CSSPseudoElementType mPseudoType = CSSPseudoElementType::NotPseudo;
> +
> +  NonOwningAnimationTarget(dom::Element* aElement, CSSPseudoElementType aType)
> +    : mElement(aElement), mPseudoType(aType) { }

Should we put the constructor at the top for consistency?
Attachment #8731632 - Flags: review?(bbirtles) → review+
Attachment #8731633 - Flags: review?(bbirtles) → review+
Attachment #8731634 - Flags: review?(bbirtles) → review+
Comment on attachment 8731636 [details] [diff] [review]
Part 4: Use NonOwningAnimationTarget as the returned value of some animation target getters

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

::: dom/animation/Animation.cpp
@@ +10,5 @@
>  #include "mozilla/dom/AnimationPlaybackEvent.h"
>  #include "mozilla/AutoRestore.h"
>  #include "mozilla/AsyncEventDispatcher.h" // For AsyncEventDispatcher
>  #include "mozilla/Maybe.h" // For Maybe
> +#include "mozilla/NonOwningAnimationTarget.h" // For NonOwningAnimationTarget

Really minor nit: I think we don't need to say "// For ..." when the type we need matches the filename?

@@ +60,5 @@
>                                             MOZ_GUARD_OBJECT_NOTIFIER_PARAM) {
>        MOZ_GUARD_OBJECT_NOTIFIER_INIT;
> +      Maybe<NonOwningAnimationTarget> target =
> +        nsNodeUtils::GetTargetForAnimation(&aAnimation);
> +      if (target.isNothing()) {

Maybe<> has an operator bool[1] which returns true when isSome() is true. That means you can just write:

if (!target) {
  return;
}

[1] https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/mfbt/Maybe.h#154

@@ +1108,5 @@
>      return;
>    }
>  
> +  Maybe<NonOwningAnimationTarget> target = mEffect->GetTarget();
> +  if (target.isNothing()) {

Likewise, here too and many other places in this file:

  if (!target) {
     ...

::: dom/animation/KeyframeEffect.h
@@ +14,5 @@
>  #include "mozilla/AnimationPerformanceWarning.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ComputedTimingFunction.h" // ComputedTimingFunction
>  #include "mozilla/LayerAnimationInfo.h"     // LayerAnimations::kRecords
> +#include "mozilla/NonOwningAnimationTarget.h" // NonOwningAnimationTarget

Minor nit: No need for comment here I think

@@ +209,5 @@
>    void GetTarget(Nullable<OwningElementOrCSSPseudoElement>& aRv) const;
> +  Maybe<NonOwningAnimationTarget> GetTarget() const
> +  {
> +    return mTarget ? Some(NonOwningAnimationTarget(mTarget, mPseudoType))
> +                   : Nothing();

I wonder if this would be slightly faster if we did:

  Maybe<NonOwningAnimationTarget> result;
  if (mTarget) {
    result.emplace(mTarget, mPseudoType);
  }
  return result;

The reason is this would benefit from return-value optimization. It probably doesn't make any difference though so please use whichever you think is more clear.

::: dom/base/nsDOMMutationObserver.cpp
@@ +388,5 @@
>      return;
>    }
>  
> +  Maybe<NonOwningAnimationTarget> animationTarget = effect->GetTarget();
> +  if (animationTarget.isNothing()) {

Here too, we can just write "if (!animationTarget)"

::: dom/base/nsNodeUtils.cpp
@@ +248,5 @@
>  void
>  nsNodeUtils::AnimationAdded(Animation* aAnimation)
>  {
> +  Maybe<NonOwningAnimationTarget> target = GetTargetForAnimation(aAnimation);
> +  if (target.isNothing()) {

if (!target) {

@@ +262,5 @@
>  void
>  nsNodeUtils::AnimationChanged(Animation* aAnimation)
>  {
> +  Maybe<NonOwningAnimationTarget> target = GetTargetForAnimation(aAnimation);
> +  if (target.isNothing()) {

Likewise here, "if (!target)" should be enough

@@ +277,5 @@
>  void
>  nsNodeUtils::AnimationRemoved(Animation* aAnimation)
>  {
> +  Maybe<NonOwningAnimationTarget> target = GetTargetForAnimation(aAnimation);
> +  if (target.isNothing()) {

Here too.

::: dom/base/nsNodeUtils.h
@@ +7,5 @@
>  #ifndef nsNodeUtils_h___
>  #define nsNodeUtils_h___
>  
> +#include "mozilla/Maybe.h"       // for Maybe<...>
> +#include "mozilla/NonOwningAnimationTarget.h" // for NonOwningAnimationTarget

Minor nit: Likewise here, I don't think we need the "// for ..." comments

@@ +144,5 @@
>      }
>    }
>  
> +  /**
> +   * Get target for a specific animation

Nit: This comment might be a little more clear as, "Utility function to get the target (pseudo-)element associated with an animation."
Attachment #8731636 - Flags: review?(bbirtles) → review+
Comment on attachment 8731637 [details] [diff] [review]
Part 5: Add a wrapper of AnimationAdded/Changed/Removed

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

::: dom/base/nsNodeUtils.cpp
@@ +249,5 @@
> +nsNodeUtils::AnimationMutated(Animation* aAnimation,
> +                              AnimationMutationType aMutatedType)
> +{
> +  Maybe<NonOwningAnimationTarget> target = GetTargetForAnimation(aAnimation);
> +  if (target.isNothing()) {

if (!target) {

@@ +265,5 @@
> +        IMPL_ANIMATION_NOTIFICATION(AnimationChanged, elem, (aAnimation));
> +        break;
> +      case AnimationMutationType::Removed:
> +        IMPL_ANIMATION_NOTIFICATION(AnimationRemoved, elem, (aAnimation));
> +        break;

I think we need a default: branch here with a MOZ_ASSERT_UNREACHABLE("unexpected mutation type"); call.

::: dom/base/nsNodeUtils.h
@@ +315,5 @@
> +   * Notify the observers of the target of an animation
> +   * @param aAnimation The mutated animation.
> +   * @param aMutationType The mutation type of this animation. It could be
> +   *                      Added, Changed, or Removed.
> +   */

Minor nit: This comment should go directly before AnimationMutated so that it can be automatically associated with that function.
Attachment #8731637 - Flags: review?(bbirtles) → review+
Comment on attachment 8731639 [details] [diff] [review]
Part 6: Support pseudo elements in Animation Mutation Observer (v2)

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

I think we should expose the pseudo type on the MutationRecord as an extra [ChromeOnly] dictionary member.  So, maybe:

  DOMString pseudo;

which is "", "::before" or "::after".  This should make it easier for devtools.

(Eventually when we have an object in the DOM to represent pseudo-elements we could use that for the target.)

The rest of the patch looks good.

Are there tests for this?
Attachment #8731639 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) (busy Mar 14–18) from comment #43)
> I think we should expose the pseudo type on the MutationRecord as an extra
> [ChromeOnly] dictionary member.  So, maybe:
> 
>   DOMString pseudo;
> 
> which is "", "::before" or "::after".  This should make it easier for
> devtools.

OK. Got it.

> 
> The rest of the patch looks good.
> 
> Are there tests for this?

Oh, yes. I am working on the test. Thanks.
(In reply to Brian Birtles (:birtles) from comment #41)
> > +      if (target.isNothing()) {
> 
> Maybe<> has an operator bool[1] which returns true when isSome() is true.
> That means you can just write:
> 
> if (!target) {
>   return;
> }

Got it. I will update all the bool checks for Maybe. Thanks.

> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 3e04659fdf6aef792f7cf9840189c6c38d08d1e8/mfbt/Maybe.h#154
Attachment #8731632 - Attachment is obsolete: true
NonOwningAnimationTarget is a struct made of two members:
1. mozilla::dom::Element*
2. mozilla::CSSPseudoElementType

Updated: Add MOZ_NON_OWNING_REF and move the position of the constructor.
Attachment #8731980 - Flags: review+
Attachment #8731976 - Attachment is obsolete: true
Use NonOwningAnimationTarget as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)

Updated: address reviewer's comments.
Attachment #8731986 - Flags: review+
Attachment #8731636 - Attachment is obsolete: true
Remove the duplicated code in nsNodeUtils.

Updated: address reviewer's comments.
Attachment #8731997 - Flags: review+
Attachment #8731637 - Attachment is obsolete: true
Attached patch Part 7: Test (v2) (obsolete) — Splinter Review
Add tests in test_animation_observers.html, so we can test elements and pseudo
elements together by setting subtree.
(In reply to Cameron McCormack (:heycam) (busy Mar 14–18) from comment #43)
> I think we should expose the pseudo type on the MutationRecord as an extra
> [ChromeOnly] dictionary member.  So, maybe:
> 
>   DOMString pseudo;
> 
> which is "", "::before" or "::after".  This should make it easier for
> devtools.

Sorry. Do you mean add a DOMString in interface MutationRecord [1]? A record may have some animations, and those animations could be animations targeting to element *and* animations targeting to pseudo elements while we get removal mutation records [2].
I saw it while writing the test case for pseudo elements. Therefore, I think the best way to make sure the pseudo types of these animations is to use "animation.effect.target.type". You can also check attachment 8732007 [details] [diff] [review].

[1] https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/dom/webidl/MutationObserver.webidl#11
[2] https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/dom/animation/test/chrome/test_animation_observers.html#1461
Flags: needinfo?(cam)
Comment on attachment 8732007 [details] [diff] [review]
Part 7: Test (v2)

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

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1504,5 @@
> +  //       the animations of its parent element.
> +  divAnimations = [ ...divAnimations,
> +                    ...divBeforeAnimations,
> +                    ...divAfterAnimations ];
> +  childBAnimations = [ ...childBAnimations, ...childBPseudoAnimations ];

The animation list in a record could be made of animations targeting to elements and pseudo elements.
Attachment #8732007 - Flags: feedback?(cam)
(In reply to Boris Chiou [:boris]  from comment #51)
> Sorry. Do you mean add a DOMString in interface MutationRecord [1]? A record
> may have some animations, and those animations could be animations targeting
> to element *and* animations targeting to pseudo elements while we get
> removal mutation records [2].

Hmm, that is a good point.  So we would either need to generate separate records, or not include the pseudo information.

> I saw it while writing the test case for pseudo elements. Therefore, I think
> the best way to make sure the pseudo types of these animations is to use
> "animation.effect.target.type".

I think that's OK, then.  Thanks!
Flags: needinfo?(cam)
Comment on attachment 8732007 [details] [diff] [review]
Part 7: Test (v2)

That's fine.
Attachment #8732007 - Flags: feedback?(cam) → feedback+
Comment on attachment 8732007 [details] [diff] [review]
Part 7: Test (v2)

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

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1614,5 @@
> +  pAnim.finish();
> +  yield await_frame();
> +  assert_records([{ added: [], changed: [], removed: [anim] }],
> +                 "records after animation is finished");
> +});

The purpose of this test is:
If there are animations targeting to a specific element whose pseudo element also has an animation, the records we got exclude animations targeting to pseudo elements if subtree is false. This test make sure the check of Subtree() for pseudo elements in nsAnimationReceiver::RecordAnimationMutation in part 6 works.
Attachment #8732007 - Flags: review?(bbirtles)
Comment on attachment 8732007 [details] [diff] [review]
Part 7: Test (v2)

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

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1600,5 @@
>    yield await_frame();
>    assert_records([], "records after animation is added");
>  });
>  
> +addAsyncAnimTest("exclude_animations_targeting_to_pseudo_elements",

Really minor nit: Drop the "to_" from here
Attachment #8732007 - Flags: review?(bbirtles) → review+
Add tests in test_animation_observers.html, so we can test elements and pseudo
elements together by setting subtree.
Attachment #8732030 - Flags: review+
Attachment #8732007 - Attachment is obsolete: true
Awesome to see the progress on this bug! Thanks.

Once this lands, the animation-inspector tool's backend will know when animations are added/changed/removed in the page (because it listens for all mutations currently, with subTree:true).
Looking at the current code [1], if a new animation is added to a pseudo-element, the backend will send an event to the timeline (the UI of the tool) to display the new animation. However the timeline isn't ready yet to display those correctly I think.
I think this should be a pretty easy thing to fix. It might only be that DomNodePreview [2] only knows how to display elements for now, not pseudos.

Thing is, I don't think we have any tests in devtools that use animated pseudo-elements yet, so I'm worried that landing this as is will break the animation-inspector in some cases without us noticing.

I'd be glad to work on the devtools part for this, but this should be done in a separate bug, and shouldn't block this from landing, because there's been a lot of work here already and it's R+.
So, why don't we just add a check at the start of onAnimationMutation [1] that bails out if the mutation target is a pseudo-element?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/animation.js#635-669
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/shared/dom-node-preview.js
(In reply to Patrick Brosset [:pbro] from comment #59)
> Awesome to see the progress on this bug! Thanks.

Thanks, Patrick. I'm glad to help you guys.

> I'd be glad to work on the devtools part for this, but this should be done
> in a separate bug, and shouldn't block this from landing, because there's
> been a lot of work here already and it's R+.
> So, why don't we just add a check at the start of onAnimationMutation [1]
> that bails out if the mutation target is a pseudo-element?

Sure. I can add a patch in this bug to check at the start of the for loop of addedAnimations and removedAnimations to avoid the potential impact on the inspector.
(In reply to Boris Chiou [:boris]  from comment #61)
> Created attachment 8732492 [details] [diff] [review]
> Part 8: Avoid adding animations on pseudo elements for Inspector temporary

I'm not sure if this patch is enough to bails out if the mutation target is a pseudo-element, so I need your eyes for this. Thanks.
Comment on attachment 8732492 [details] [diff] [review]
Part 8: Avoid adding animations on pseudo elements for Inspector temporary

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

This looks good to me. Thanks.
Can you just have the exact same comment in the 2 locations?
Attachment #8732492 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbro] from comment #63)
> Can you just have the exact same comment in the 2 locations?

Sure!
Attachment #8732492 - Attachment is obsolete: true
Use NonOwningAnimationTarget as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)

Rebased.
Attachment #8732735 - Flags: review+
Attachment #8731986 - Attachment is obsolete: true
We use the parent element of a pseudo element as the subject to be notified.

Usage:
We record animations targeting to pseudo elements by setting subtree attribute
to true.

MutationObserver(Node, { subtree: ture });

So all the animations targeting to the pseudo elements whose parentElement is
the first argument will be recorded.

Rebased.
Attachment #8732740 - Flags: review+
Attachment #8731639 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.