Once KeyframeEffect supports pseudo element as target element, getAnimations() should return these animations.

(In reply to Boris Chiou [:boris] from comment #6) > Created attachment 8720124 [details] [diff] [review] > Part 3: Test Boris, we can now use Element.animate() instead of constructing each Animation object.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7) > Boris, we can now use Element.animate() instead of constructing each > Animation object. It's a good idea, but I think Element.animate() is only for "Element" target, instead of "CSSPseudoElement" target, and CSSPseudoElement.animate() hasn't implemented yet (Bug 1231784). After finishing Bug 1241784, we can revise this part by animate().

Ah, I am sorry. I thought we already have it.

I have to handle Bug 1241784 as soon as possible. :)

Wait for Bug 1244049.

Comment on attachment 8720122 [details] [diff] [review] Part 1: Implement CSSPseudoElement.getAnimations (v2) Review of attachment 8720122 [details] [diff] [review]: ----------------------------------------------------------------- Hi Brian, The naming of CSSPseudoElementType(::NotPseudo, ::after, ::before) is being reviewed in Bug 1244049, but I think you could take a look at the design of CSSPseudoElement.getAnimations() in the meantime. Thanks.

Comment on attachment 8720122 [details] [diff] [review] Part 1: Implement CSSPseudoElement.getAnimations (v2) Review of attachment 8720122 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/CSSPseudoElement.cpp @@ +8,5 @@ > #include "mozilla/dom/CSSPseudoElementBinding.h" > #include "mozilla/dom/Element.h" > +#include "mozilla/dom/KeyframeEffect.h" > +#include "mozilla/AnimationComparator.h" > +#include "mozilla/EffectSet.h" What are EffectSet.h and KeyframeEffect.h needed for? @@ +59,5 @@ > + doc->FlushPendingNotifications(Flush_Style); > + } > + > + Element::GetAnimationsUnsorted(mParentElement, mPseudoType, aRetVal); > + Nit: Drop this blank line. ::: dom/base/Element.h @@ +117,5 @@ > // Make sure we have space for our bits > ASSERT_NODE_FLAGS_SPACE(ELEMENT_TYPE_SPECIFIC_BITS_OFFSET); > > namespace mozilla { > +enum class CSSPseudoElementType: uint8_t; Nit: space before the ':' ?

Comment on attachment 8720124 [details] [diff] [review] Part 3: Test Review of attachment 8720124 [details] [diff] [review]: ----------------------------------------------------------------- Looks good but: * I'd like to see these tests after reworking them to be self-contained * We should make the test check for "::after" and file a bug to fix that * I don't think we can put these tests in web-platform tests yet since they rely on using CSS animations and the mapping from CSS animations to Web Animations hasn't been specced yet * I think we should add some of these tests to dom/animations/test/css-animations/file_document-get-animations.html * We need to test for sorting of animations on pseudo elements from document.getAnimations(), i.e. this code: https://dxr.mozilla.org/mozilla-central/rev/709f559b5406e8555cf84dd09bdc747b076f142c/layout/style/nsAnimationManager.cpp#138 We should trigger CSS animations (and transitions) on an element and ::before and ::after and check that they are returned in the correct order from getAnimations() ::: testing/web-platform/tests/web-animations/keyframe-effect/CSSPseudoElement.html @@ +43,5 @@ > +"use strict"; > + > +test(function(t) { > + var anims = document.getAnimations(); > + assert_equals(anims.length, 2, "# of animations in this document"); I think it's better if we make these tests self-contained. i.e., define the rules as you have, but then generate the target elements in each test. @@ +47,5 @@ > + assert_equals(anims.length, 2, "# of animations in this document"); > + > + var pseudo = anims[1].effect.target; > + assert_equals(pseudo.parentElement, anims[0].effect.target, "parentElement"); > + assert_equals(pseudo.type, ":after", "psuedo type"); This should be ::after. We might need to fix our implementation of CSSPseudoElement to prepend an extra ':' since it seems like the atom defined by nsCSSPseudoElementList.h only has one. See https://dxr.mozilla.org/mozilla-central/rev/709f559b5406e8555cf84dd09bdc747b076f142c/layout/style/StyleRule.cpp#769 Also, s/psuedo/pseudo/ @@ +51,5 @@ > + assert_equals(pseudo.type, ":after", "psuedo type"); > +}, "A pseudo-element could be got from KeyframeEffectReadOnly.target"); > + > +test(function(t) { > + var target = document.getAnimations()[1].effect.target; Again, I think we should make these tests self-contained.

Rebased.

(In reply to Brian Birtles (:birtles) from comment #14) > * I don't think we can put these tests in web-platform tests yet since they > rely on using CSS animations and the mapping from CSS animations to Web > Animations hasn't been specced yet > * I think we should add some of these tests to > dom/animations/test/css-animations/file_document-get-animations.html OK. I will move all the tests into CSS animations. 1. Add some tests into file_document-get-animations.html. 2. Create a new test for pseudo elements. Thanks.

(In reply to Brian Birtles (:birtles) from comment #14) > We should trigger CSS animations (and transitions) on an element and > ::before and ::after and check that they are returned in the correct order > from getAnimations() Do we have any simple way to add a CSS transition on a pseudo element in the mochitests? I try this: e.g. .before::before { width: 0px; transition: all 10s; } .before2::before { width: 100px; } test(function(t){ var div = addDiv(t); div.classList.add('before'); getComputedStyle(div, '::before').width; div.classList.add('before2'); var anims = document.getAnimations(); // the length of anims is 0 }, '...'); But it doesn't work.

Comment on attachment 8721341 [details] [diff] [review] Part 3: Test for the animation order returned by document.getAnimations() Review of attachment 8721341 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-animations/file_document-get-animations.html @@ +269,5 @@ > + assert_equals(anims[id].effect.target.parentElement, div, > + 'the parentElement of this target'); > + assert_equals(anims[id].effect.target.type, orderTest[i], > + 'the type of this target'); > + } I think we can make this test a lot simpler and just focus on sort order. e.g. assert_equals(anims[0].effect.target, div, 'animation on regular element is returned first'); assert_equals(anims[1].effect.target.type, '::before', 'animation on ::before pseudo element is returned second'); assert_equals(anims[2].effect.target.type, '::after', 'animation on ::after pseudo element is returned last'); I think that would be enough? You might also like to test that children of the element are also sorted correctly (i.e. *after* the pseudos). If you want to make this more self-contained, you can do something like: https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/layout/style/test/test_animations_event_order.html#138

Comment on attachment 8721342 [details] [diff] [review] Part 4: Test for the support of CSSPseudoElement returned by effect.target Review of attachment 8721342 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-animations/file_effect-target.html @@ +35,5 @@ > + 'The target is a CSSPseudoElement object'); > + assert_class_string(anims[2].effect.target, 'HTMLDivElement', > + 'The target is a HTMLDivElement'); > +}, 'The targets returned by animation effect could be CSSPseudoElement or ' + > + 'Element'); I'm not really sure what this is testing. Perhaps just drop the div2 part and the check for HTMLDivElement and make this a test about checking we get back a CSSPseudoElement object? The first test in this file checks the type in the case of an Element target since it does object equality testing.

Comment on attachment 8721651 [details] [diff] [review] Part 5: Test for CSSPseudoElement.getAnimations (v3) Review of attachment 8721651 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-animations/file_pseudoElement-get-animations.html @@ +25,5 @@ > + var div = addDiv(t, { class: 'with-before-animation' }); > + var pseudoTarget = document.getAnimations()[0].effect.target; > + assert_class_string(pseudoTarget.getAnimations()[0], 'CSSAnimation', > + 'Interface of returned animation is CSSAnimation'); > +}, 'getAnimations returns CSSAnimation objects for CSS Animations'); Why is this important to test? I think a better test would just be that we get back the animation we expected, e.g. something like assert_equals(pseudoTarget.getAnimations().length, 1, 'Expected number of animations are returned'); assert_equals(pseudoTarget.getAnimations()[0].animationName, 'anim1', 'CSS animation name matches'); @@ +47,5 @@ > + assert_equals(anims[2].id, 'anim3', > + 'The animation id of the animation created by '+ > + 'Animation constructor'); > +}, 'getAnimations for CSSAnimations targetting to the same CSSPseudoElement ' + > + 'object follows animation-name order'); This test is confusing because the test description doesn't match what it does. I think you should break it into two parts: 1. Test that animations are returned in animation-name order 2. Test that when another animation is via script, it appears at the end of the list. e.g. test(function(t) { var div = addDiv(t, { class: 'with-after-animation' }); // Sanity checks assert_equals(document.getAnimations().length, 2, 'Got expected number of animations on document'); var pseudoElement = document.getAnimations()[0].effect.target; assert_class_string(pseudoElement, 'CSSPseudoElement', 'Got pseudo-element target'); // Check animations return from the pseudo element are in correct order assert_equals(pseudoElement.getAnimations().length, 2, 'Got expected number of animations on pseudo-element'); assert_equals(pseudoElement.getAnimations()[0].animationName, 'anim1', 'First animation returned from pseudo element is first ' + 'animation in animation-name list'); assert_equals(pseudoElement.getAnimations()[1].animationName, 'anim2', 'Second animation returned from pseudo element is second ' + 'animation in animation-name list'); }, 'getAnimations for CSSAnimations targetting to the same CSSPseudoElement ' + 'object follows animation-name order'); test(function(t) { var div = addDiv(t, { class: 'with-after-animation' }); // Get animations on document var docAnimations = document.getAnimations(); assert_equals(docAnimations.length, 2, 'Got expected number of animations on document'); // Create additional animation on the pseudo-element from script var pseudoElement = document.getAnimations()[0].effect.target; var effect = new KeyframeEffectReadOnly(pseudoTarget, { background: ["blue", "red"] }, 3000); var newAnimation = new Animation(effect, document.timeline); newAnimation.id = 'anim3'; newAnimation.play(); // Check order--the script-generated animation should appear later var animations = pseudoTarget.getAnimations(); assert_equals(anims.length, 3, 'Got expected number of animations after adding animation ' + 'from script'); assert_equals(anims[0], docAnims[0], 'First animation is the first existing animation returned ' + 'on the document'); assert_equals(anims[1], docAnims[1], 'Second animation is the second existing animation returned ' + 'on the document'); assert_equals(anims[2].id, 'anim3', 'Animation added by script appears last'); }, 'getAnimations for returns script-generated animations in the expected ' + 'order'); @@ +64,5 @@ > + assert_equals(anims[0].effect.target, pseudoTarget, > + 'The same CSSPseudoElement object'); > + assert_equals(anims[1].effect.target, pseudoTarget, > + 'The same CSSPseudoElement object'); > +}, 'A pseudo-element could be used to get animations that target itself'); I don't understand what this is testing. It seems like it is already covered above? ::: dom/animation/test/mochitest.ini @@ +48,5 @@ > support-files = css-animations/file_element-get-animations.html > [css-animations/test_keyframeeffect-getframes.html] > support-files = css-animations/file_keyframeeffect-getframes.html > +[css-animations/test_pseudoElement-get-animations.html] > +skip-if = buildapp == 'mulet' Why does this fail on mulet?

(In reply to Boris Chiou [:boris] from comment #19) > (In reply to Brian Birtles (:birtles) from comment #14) > > We should trigger CSS animations (and transitions) on an element and > > ::before and ::after and check that they are returned in the correct order > > from getAnimations() > > Do we have any simple way to add a CSS transition on a pseudo element in the > mochitests? I try this: > e.g. > .before::before { > width: 0px; > transition: all 10s; > } > .before2::before { > width: 100px; > } > > test(function(t){ > var div = addDiv(t); > div.classList.add('before'); > getComputedStyle(div, '::before').width; > div.classList.add('before2'); > > var anims = document.getAnimations(); // the length of anims is 0 > > }, '...'); > > But it doesn't work. I'm pretty sure you can debug that one! You might want to add one or more of the following: content: " "; display: block; height: 100px; background: blue; /* for debugging */

Thanks for your hints, Brian. I checked the link from Comment 25 and the hint from Comment 28, and they are useful. I also added some tests for css-transitions. Thanks.

Comment on attachment 8721876 [details] [diff] [review] Part 3: Test for the animation order returned by document.getAnimations() (v2) Review of attachment 8721876 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed. I think you could make the tests much easier to read by using descriptive names (i.e. not div[0], div[1]), re-using addDiv to automatically clean up, and using more informative test descriptions. ::: dom/animation/test/css-animations/file_document-get-animations.html @@ +252,5 @@ > + // ::before, > + // ::after > + // | > + // div[1] > + var divs = [addDiv(t), document.createElement('div')]; Why not call addDiv twice and then you don't need to remember to call remove() at the end? Or, better still, write something like: var parent = addDiv(t); var child = addDiv(t); parent.appendChild(child); [parent, child].forEach((div, i) => { ... Then use 'parent' for your id? @@ +264,5 @@ > + assert_equals(anims.length, 4, > + 'CSS animations on both pseudo-elements and elements ' + > + 'are returned'); > + assert_equals(anims[0].effect.target, divs[0], > + 'the target of the 1st animation'); These descriptions could be more helpful. e.g. 'The animation targeting the parent element comes first' 'The animation targeting the :before element comes second' 'The animation targeting the :after element comes third' 'The animation targeting the child element comes last' @@ +272,5 @@ > + 'the type of the 3rd animation'); > + assert_equals(anims[3].effect.target, divs[1], > + 'the target of the 4th animation'); > + > + divs[1].remove(); This can be removed if we use addDiv above. ::: dom/animation/test/css-transitions/file_document-get-animations.html @@ +30,5 @@ > assert_equals(document.getAnimations().length, 0, > 'getAnimations returns no running CSS Transitions'); > }, 'getAnimations for CSS Transitions'); > > +test(function(t) { My feedback for the transitions test is basically the same as for the animations test.

(In reply to Brian Birtles (:birtles) from comment #32) > Why not call addDiv twice and then you don't need to remember to call > remove() at the end? > > Or, better still, write something like: > > var parent = addDiv(t); > var child = addDiv(t); > > parent.appendChild(child); > > [parent, child].forEach((div, i) => { > ... > > Then use 'parent' for your id? Oops. addDiv() uses document.appendChild() for this div and I thought the 2nd div should be the child of the 1st div, not the child of document. Maybe I should revise addDiv() for this case. > These descriptions could be more helpful. e.g. > 'The animation targeting the parent element comes first' > 'The animation targeting the :before element comes second' > 'The animation targeting the :after element comes third' > 'The animation targeting the child element comes last' OK. Thanks.

(In reply to Boris Chiou [:boris] from comment #33) > (In reply to Brian Birtles (:birtles) from comment #32) > > Why not call addDiv twice and then you don't need to remember to call > Oops. addDiv() uses document.appendChild() for this div and I thought the > 2nd div should be the child of the 1st div, not the child of document. Maybe > I should revise addDiv() for this case. OK. I think parent.appendChild(child) will override the parentNode of this div, so everything will be OK by using addDiv twice. Thanks.

Updated: address birtles' comments.

Comment on attachment 8721877 [details] [diff] [review] Part 4: Test for the CSSPseudoElement objects returned by effect.target (v2) Review of attachment 8721877 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-animations/file_effect-target.html @@ +16,5 @@ > 'Animation.target is the animatable div'); > +}, 'Returned CSS animations have the correct effect targets'); > + > +// Make sure we got the same CSSPseudoElement object while calling effect.target > +// many times. (Element.SetProperty/GetProperty work in this case.) I think we should drop this comment and make the test self-documenting. @@ +23,5 @@ > + document.head.appendChild(extraStyle); > + var sheet = extraStyle.sheet; > + sheet.insertRule('.after::after { animation: anim 10s, anim 100s; }', 0); > + > + var div = addDiv(t, { class: 'after' }); I think it would be enough just to test at this point that we get back two animations with the same target. e.g. var animations = document.getAnimations(); assert_equals(animations.length, 2, 'Got animations running on ::after pseudo element'); assert_equals(animations[0].target, animations[1].target, 'Both animations return the same target object'); That's probably enough. If you want to test that adding a third animation also uses the same target, then make that a separate step. @@ +42,5 @@ > + assert_equals(anims[2].effect.target, pseudoTarget, > + 'The same CSSPseudoElement object'); > + > + extraStyle.remove(); > +}, 'effect.target should return the same CSSPseudoElement object'); 'effect.target should return the same CSSPseudoElement object each time ::: dom/animation/test/css-transitions/file_effect-target.html @@ +30,5 @@ > + var div = addDiv(t, { class: 'init' }); > + flushComputedStyle(div); > + div.classList.add('change'); > + > + var pseudoTarget = document.getAnimations()[0].effect.target; Same here, just test the animations you have without complicating it further with script-generated tests. Then, if you want to test script-generated animations don't change the target that is returned, make it a separate step. @@ +38,5 @@ > + var newAnim = new Animation(effect, document.timeline); > + newAnim.play(); > + > + var anims = document.getAnimations(); > + assert_equals(anims.length, 3, 'The # of animations'); s/#/number/ @@ +44,5 @@ > + 'The same CSSPseudoElement object'); > + assert_equals(anims[1].effect.target, pseudoTarget, > + 'The same CSSPseudoElement object'); > + assert_equals(anims[2].effect.target, pseudoTarget, > + 'The same CSSPseudoElement object'); These test descriptions are not very useful.

Comment on attachment 8721879 [details] [diff] [review] Part 5: Test for CSSPseudoElement.getAnimations (v4) Review of attachment 8721879 [details] [diff] [review]: ----------------------------------------------------------------- There's no test that covers the ordering between transitions and animations. I'd probably just drop the second half of the transitions test file (the one for script-generated animations) and extend the last test in the animations test file to check we get transitions, animations, script-generated animations. ::: dom/animation/test/css-animations/file_pseudoElement-get-animations.html @@ +42,5 @@ > + 'animation in animation-name list'); > + assert_equals(pseudoTarget.getAnimations()[1].animationName, 'anim2', > + 'Second animation returned from pseudo element is the second ' + > + 'animation in animation-name list'); > +}, 'getAnimations for CSSAnimations targetting to the same CSSPseudoElement ' + 'targeting the same' @@ +75,5 @@ > + 'Second animation is the second existing animation returned ' + > + 'on the document'); > + assert_equals(anims[2].id, 'anim3', > + 'Animation added by script appears last'); > +}, 'getAnimations for returns script-generated animations in the expected ' + 'getAnimations returns' ::: dom/animation/test/css-transitions/file_pseudoElement-get-animations.html @@ +64,5 @@ > + assert_equals(anims.length, 4, > + 'Got expected number of animations after adding animation ' + > + 'from script'); > + assert_equals(anims[0], docAnims[0], > + 'First animation is the first existing animation returned ' + s/animation/transition/ here and below @@ +70,5 @@ > + assert_equals(anims[1], docAnims[1], > + 'Second animation is the second existing animation returned ' + > + 'on the document'); > + assert_equals(anims[2], docAnims[2], > + 'Third animation is the second existing animation returned ' + s/second/third/ @@ +74,5 @@ > + 'Third animation is the second existing animation returned ' + > + 'on the document'); > + assert_equals(anims[3].id, 'anim4', > + 'Animation added by script appears last'); > +}, 'getAnimations for returns script-generated animations in the expected ' + 'getAnimations returns'

Updated: 1. Add addStyle function in testcommon.js 2. Split tests.

Comment on attachment 8722336 [details] [diff] [review] Part 4: Test for the CSSPseudoElement objects returned by effect.target (v4) Review of attachment 8722336 [details] [diff] [review]: ----------------------------------------------------------------- I added a helper function, addStyle(), to create the style dynamically. If it is accepted, I'd like to swap the order of part 3 and part 4, so file_document_get_animations.html can also use this helper function.

(In reply to Brian Birtles (:birtles) from comment #37) > There's no test that covers the ordering between transitions and animations. > I'd probably just drop the second half of the transitions test file (the one > for script-generated animations) and extend the last test in the animations > test file to check we get transitions, animations, script-generated > animations. This sounds great. I could do this and update this patch. Thanks.

Updated: 1. Extend the last test in css-animations/file_pseudoElement-get-animations.html 2. Remove the redundant tests.

Comment on attachment 8722368 [details] [diff] [review] Part 5: Test for CSSPseudoElement.getAnimations (v5) Review of attachment 8722368 [details] [diff] [review]: ----------------------------------------------------------------- Could you please take a look at this patch again? Thanks. ::: dom/animation/test/css-animations/file_pseudoElement-get-animations.html @@ +62,5 @@ > + '4rd animation is the 2nd animation in animation-name list'); > + assert_equals(anims[4].id, 'scripted-anim', > + 'Animation added by script appears last'); > +}, 'getAnimations returns css transitions/animations, and script-generated ' + > + 'animations in the expected order'); I remove the original 2nd test case and extend this test case for checking the order of css animations/css transitions/scripted animations.

Comment on attachment 8722336 [details] [diff] [review] Part 4: Test for the CSSPseudoElement objects returned by effect.target (v4) Review of attachment 8722336 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed. ::: dom/animation/test/css-animations/file_effect-target.html @@ +13,5 @@ > div.style.animation = 'anim 100s'; > var animation = div.getAnimations()[0]; > assert_equals(animation.effect.target, div, > 'Animation.target is the animatable div'); > +}, 'Returned CSS animations have the correct effect targets'); Nit: s/targets/target/ @@ +38,5 @@ > + var anims = document.getAnimations(); > + assert_equals(anims.length, 2, > + 'Got animations running on ::after pseudo element'); > + assert_equals(anims[0].effect.target, newAnim.effect.target, > + 'Both animations return the same target object'); Maybe we should also test that newAnim.effect.target === pseudoTarget? Up to you. Also, see my comment on the transitions test but we should either check that newAnim !== anims[0] or else just compare the targets of anims[0].effect and anims[1].effect. @@ +40,5 @@ > + 'Got animations running on ::after pseudo element'); > + assert_equals(anims[0].effect.target, newAnim.effect.target, > + 'Both animations return the same target object'); > +}, 'effect.target from the script-generated animation should return the same' + > + 'CSSPseudoElement object as that from the CSS generated animation'); Space missing between 'same' and 'CSSPseudoElement'. ::: dom/animation/test/css-transitions/file_effect-target.html @@ +19,5 @@ > }, 'Returned CSS transitions have the correct Animation.target'); > > +test(function(t) { > + addStyle(t, { '.init::after': 'content: ""; width: 0px; height: 0px; ' + > + 'transition: all 10s;', Indentation seems off here. @@ +48,5 @@ > + > + var anims = document.getAnimations(); > + assert_equals(anims.length, 2, > + 'Got animations running on ::after pseudo element'); > + assert_equals(anims[0].effect.target, newAnim.effect.target, We should probably check that anims[0] !== newAnim or else just compare the targets of anims[0] and anims[1] otherwise if getAnimations() returns newAnim first we'll end up comparing newAnim to newAnim. @@ +52,5 @@ > + assert_equals(anims[0].effect.target, newAnim.effect.target, > + 'Both the transition and the scripted-generated animation ' + > + 'return the same target object'); > +}, 'effect.target from the script-generated animation should return the same' + > + 'CSSPseudoElement object as that from the CSS generated transition'); Missing a space between 'same' and 'CSSPseudoElement' here. ::: dom/animation/test/testcommon.js @@ +32,5 @@ > /** > + * Appends a style div to the document head. > + * > + * @param t The testharness.js Test object. If provided, this will be used > + * to register a cleanup callback to remove the div when the test "remove the style element" @@ +36,5 @@ > + * to register a cleanup callback to remove the div when the test > + * finishes. > + * > + * @param rules A dictionary object with selector names and rules to set on > + * the CSSSytleSheet s/CSSSytleSheet/style sheet./ @@ +41,5 @@ > + */ > +function addStyle(t, rules) { > + var extraStyle = document.createElement('style'); > + document.head.appendChild(extraStyle); > + if (t && typeof t.add_cleanup === 'function') { Put this part at the end of the function like we do in addDiv? @@ +43,5 @@ > + var extraStyle = document.createElement('style'); > + document.head.appendChild(extraStyle); > + if (t && typeof t.add_cleanup === 'function') { > + t.add_cleanup(function() { > + extraStyle.parentNode.removeChild(extraStyle); extraStyle.remove(); @@ +46,5 @@ > + t.add_cleanup(function() { > + extraStyle.parentNode.removeChild(extraStyle); > + }); > + } > + if (rules) { Nit: Add a blank line before this otherwise it looks like an else block.

Comment on attachment 8722336 [details] [diff] [review] Part 4: Test for the CSSPseudoElement objects returned by effect.target (v4) Review of attachment 8722336 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-animations/file_effect-target.html @@ +38,5 @@ > + var anims = document.getAnimations(); > + assert_equals(anims.length, 2, > + 'Got animations running on ::after pseudo element'); > + assert_equals(anims[0].effect.target, newAnim.effect.target, > + 'Both animations return the same target object'); OK. Adding 'newAnim.effect.target === pseudoTarget' is OK to me, and I should also add the check of 'newAnim! = anims[0]'. Thanks.

Updated: address birtles' comments.

Rebased: Reuse addStyle in part 3.

Comment on attachment 8722368 [details] [diff] [review] Part 5: Test for CSSPseudoElement.getAnimations (v5) Review of attachment 8722368 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed ::: dom/animation/test/css-animations/file_pseudoElement-get-animations.html @@ +29,5 @@ > + assert_equals(pseudoTarget.getAnimations().length, 1, > + 'Expected number of animations are returned'); > + assert_equals(pseudoTarget.getAnimations()[0].animationName, 'anim1', > + 'CSS animation name matches'); > +}, 'getAnimations returns CSSAnimation objects we expected'); 'getAnimations returns expected CSSAnimation objects' @@ +33,5 @@ > +}, 'getAnimations returns CSSAnimation objects we expected'); > + > +test(function(t) { > + var div = addDiv(t, { class: 'after-with-mix-anims-trans' }); > + // Triggle transitions Trigger transitions @@ +49,5 @@ > + > + // Check order - the script-generated animation should appear later > + var anims = pseudoTarget.getAnimations(); > + assert_equals(anims.length, 5, > + 'Got expected number of animations/trnasition running on ' + transitions ::: dom/animation/test/css-animations/test_pseudoElement-get-animations.html @@ +11,5 @@ > + function() { > + window.open("file_pseudoElement-get-animations.html"); > + }); > +</script> > +</html> This has an trailing </html> but no opening <html>. Just drop the trailing </html>.

Updated: Fix typos.

https://hg.mozilla.org/integration/mozilla-inbound/rev/069f272ec9f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/df84913f9ff2 https://hg.mozilla.org/integration/mozilla-inbound/rev/d49c6a43b80a https://hg.mozilla.org/integration/mozilla-inbound/rev/a40b8d9d7e46 https://hg.mozilla.org/integration/mozilla-inbound/rev/2899d594244e

https://hg.mozilla.org/mozilla-central/rev/069f272ec9f1 https://hg.mozilla.org/mozilla-central/rev/df84913f9ff2 https://hg.mozilla.org/mozilla-central/rev/d49c6a43b80a https://hg.mozilla.org/mozilla-central/rev/a40b8d9d7e46 https://hg.mozilla.org/mozilla-central/rev/2899d594244e