Closed
Bug 1249219
Opened 9 years ago
Closed 9 years ago
Animation mutation observers do not support animations on pseudo elements
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: birtles, Assigned: boris)
References
Details
Attachments
(8 files, 17 obsolete files)
(deleted),
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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
Reporter | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → boris.chiou
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
(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.
Assignee | ||
Comment 3•9 years ago
|
||
(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)
Reporter | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Reporter | ||
Comment 6•9 years ago
|
||
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?
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Comment hidden (typo) |
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment hidden (obsolete) |
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cam)
Comment 12•9 years ago
|
||
important |
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)
Assignee | ||
Comment 13•9 years ago
|
||
Got it. Thanks.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 16•9 years ago
|
||
Use Pair<Element*, CSSPseudoElementType> as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)
Assignee | ||
Updated•9 years ago
|
Attachment #8730564 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
Merge AnimationAdded, AnimationChanged, and AnimationRemoved into one function,
AnimationMutated.
Assignee | ||
Updated•9 years ago
|
Attachment #8731116 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8731117 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8731142 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8731139 -
Flags: review?(bbirtles)
Assignee | ||
Updated•9 years ago
|
Attachment #8731140 -
Flags: review?(bbirtles)
Assignee | ||
Updated•9 years ago
|
Attachment #8731141 -
Flags: review?(bbirtles)
Assignee | ||
Comment 21•9 years ago
|
||
Add tests in test_animation_observers.html, so we can test elements and pseudo
elements together by setting subtree.
Attachment #8731156 -
Flags: review?(bbirtles)
Assignee | ||
Updated•9 years ago
|
Attachment #8731151 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Reporter | ||
Comment 23•9 years ago
|
||
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)
Reporter | ||
Comment 24•9 years ago
|
||
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)
Reporter | ||
Comment 25•9 years ago
|
||
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)
Reporter | ||
Comment 26•9 years ago
|
||
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)
Assignee | ||
Comment 27•9 years ago
|
||
(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
Reporter | ||
Comment 28•9 years ago
|
||
(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.
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Assignee | ||
Comment 30•9 years ago
|
||
(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(). :)
Assignee | ||
Comment 31•9 years ago
|
||
(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.)
Assignee | ||
Comment 32•9 years ago
|
||
(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
Reporter | ||
Comment 33•9 years ago
|
||
(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
Assignee | ||
Comment 34•9 years ago
|
||
NonOwningAnimationTarget is a struct made of two members:
1. mozilla::dom::Element*
2. mozilla::CSSPseudoElementType
Attachment #8731632 -
Flags: review?(bbirtles)
Assignee | ||
Updated•9 years ago
|
Attachment #8731139 -
Attachment is obsolete: true
Attachment #8731140 -
Attachment is obsolete: true
Assignee | ||
Comment 35•9 years ago
|
||
We use NonOwningAnimationTarget as the hash key.
Attachment #8731633 -
Flags: review?(bbirtles)
Assignee | ||
Comment 36•9 years ago
|
||
Attachment #8731634 -
Flags: review?(bbirtles)
Assignee | ||
Comment 37•9 years ago
|
||
Use NonOwningAnimationTarget as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)
Attachment #8731636 -
Flags: review?(bbirtles)
Assignee | ||
Comment 38•9 years ago
|
||
Remove the duplicated code in nsNodeUtils.
Attachment #8731637 -
Flags: review?(bbirtles)
Assignee | ||
Comment 39•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8731141 -
Attachment is obsolete: true
Attachment #8731141 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8731156 -
Attachment is obsolete: true
Reporter | ||
Comment 40•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Attachment #8731633 -
Flags: review?(bbirtles) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8731634 -
Flags: review?(bbirtles) → review+
Reporter | ||
Comment 41•9 years ago
|
||
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+
Reporter | ||
Comment 42•9 years ago
|
||
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 43•9 years ago
|
||
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)
Assignee | ||
Comment 44•9 years ago
|
||
(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.
Assignee | ||
Comment 45•9 years ago
|
||
(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
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8731632 -
Attachment is obsolete: true
Assignee | ||
Comment 47•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Attachment #8731976 -
Attachment is obsolete: true
Assignee | ||
Comment 48•9 years ago
|
||
Use NonOwningAnimationTarget as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)
Updated: address reviewer's comments.
Attachment #8731986 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8731636 -
Attachment is obsolete: true
Assignee | ||
Comment 49•9 years ago
|
||
Remove the duplicated code in nsNodeUtils.
Updated: address reviewer's comments.
Attachment #8731997 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8731637 -
Attachment is obsolete: true
Assignee | ||
Comment 50•9 years ago
|
||
Add tests in test_animation_observers.html, so we can test elements and pseudo
elements together by setting subtree.
Assignee | ||
Comment 51•9 years ago
|
||
(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)
Assignee | ||
Comment 52•9 years ago
|
||
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)
Comment 53•9 years ago
|
||
(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)
Updated•9 years ago
|
Attachment #8731639 -
Flags: review+
Comment 54•9 years ago
|
||
Comment on attachment 8732007 [details] [diff] [review]
Part 7: Test (v2)
That's fine.
Attachment #8732007 -
Flags: feedback?(cam) → feedback+
Assignee | ||
Comment 55•9 years ago
|
||
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)
Reporter | ||
Comment 56•9 years ago
|
||
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+
Assignee | ||
Comment 57•9 years ago
|
||
Add tests in test_animation_observers.html, so we can test elements and pseudo
elements together by setting subtree.
Attachment #8732030 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8732007 -
Attachment is obsolete: true
Assignee | ||
Comment 58•9 years ago
|
||
Comment 59•9 years ago
|
||
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
Assignee | ||
Comment 60•9 years ago
|
||
(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.
Assignee | ||
Comment 61•9 years ago
|
||
Attachment #8732492 -
Flags: review?(pbrosset)
Assignee | ||
Comment 62•9 years ago
|
||
(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 63•9 years ago
|
||
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+
Assignee | ||
Comment 64•9 years ago
|
||
(In reply to Patrick Brosset [:pbro] from comment #63)
> Can you just have the exact same comment in the 2 locations?
Sure!
Assignee | ||
Comment 65•9 years ago
|
||
Attachment #8732732 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8732492 -
Attachment is obsolete: true
Assignee | ||
Comment 66•9 years ago
|
||
Use NonOwningAnimationTarget as the returned type of
1. KeyframeEffectReadOnly::GetTarget()
2. nsNodeUtils::GetTargetForAnimation(...)
Rebased.
Attachment #8732735 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8731986 -
Attachment is obsolete: true
Assignee | ||
Comment 67•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Attachment #8731639 -
Attachment is obsolete: true
Assignee | ||
Comment 68•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 69•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8186c484a64d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6919c17077a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1b7305c0cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f9fc0d49d49
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3e647f539b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2047b0726ff5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5d40fcf830
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c90d5d3f78d
Keywords: checkin-needed
Comment 70•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8186c484a64d
https://hg.mozilla.org/mozilla-central/rev/c6919c17077a
https://hg.mozilla.org/mozilla-central/rev/ac1b7305c0cd
https://hg.mozilla.org/mozilla-central/rev/8f9fc0d49d49
https://hg.mozilla.org/mozilla-central/rev/d3e647f539b9
https://hg.mozilla.org/mozilla-central/rev/2047b0726ff5
https://hg.mozilla.org/mozilla-central/rev/cc5d40fcf830
https://hg.mozilla.org/mozilla-central/rev/0c90d5d3f78d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•