Closed Bug 1234403 Opened 4 years ago Closed 4 years ago

Document/CSSPseudoElement.getAnimations() should return animations associated with pseudo elements

Categories

(Core :: DOM: Animation, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: hiro, Assigned: boris)

References

Details

Attachments

(5 files, 19 obsolete files)

6.75 KB, patch
boris
: review+
Details | Diff | Splinter Review
1.80 KB, patch
boris
: review+
Details | Diff | Splinter Review
6.57 KB, patch
boris
: review+
Details | Diff | Splinter Review
5.61 KB, patch
boris
: review+
Details | Diff | Splinter Review
8.11 KB, patch
boris
: review+
Details | Diff | Splinter Review
Once KeyframeEffect supports pseudo element as target element, getAnimations() should return these animations.
Blocks: 1206420
Assignee: nobody → boris.chiou
Attached patch [WIP] Implement getAnimations (obsolete) — Splinter Review
Attached patch [WIP] Part 2: Test (v2) (obsolete) — Splinter Review
Attachment #8713089 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Depends on: 1244049
Summary: Document/Element.getAnimations() should return animations associated with pseudo elements → Document/CSSPsuedoElement.getAnimations() should return animations associated with pseudo elements
Attachment #8712072 - Attachment is obsolete: true
Attachment #8713112 - Attachment is obsolete: true
Attached patch Part 3: Test (obsolete) — Splinter Review
(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. :)
Blocks: 1241784
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.
Attachment #8720122 - Flags: review?(bbirtles)
Attachment #8720123 - Flags: review?(bbirtles)
Attachment #8720124 - Flags: review?(bbirtles)
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 ':' ?
Attachment #8720122 - Flags: review?(bbirtles) → review+
Attachment #8720123 - Flags: review?(bbirtles) → review+
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.
Attachment #8720124 - Flags: review?(bbirtles)
Summary: Document/CSSPsuedoElement.getAnimations() should return animations associated with pseudo elements → Document/CSSPseudoElement.getAnimations() should return animations associated with pseudo elements
Depends on: 1249230
Attachment #8720123 - Attachment is obsolete: true
Attachment #8720122 - Attachment is obsolete: true
Attachment #8720733 - Attachment is obsolete: true
Attachment #8720734 - Flags: review+
(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.
Attachment #8720124 - Attachment is obsolete: true
Attachment #8721341 - Flags: review?(bbirtles)
Attachment #8721342 - Flags: review?(bbirtles)
Attachment #8721343 - Flags: review?(bbirtles)
Attachment #8721343 - Attachment is obsolete: true
Attachment #8721343 - Flags: review?(bbirtles)
Attachment #8721651 - Flags: review?(bbirtles)
Attachment #8721351 - Attachment is obsolete: true
Attachment #8721351 - Flags: review?(bbirtles)
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
Attachment #8721341 - Flags: review?(bbirtles)
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.
Attachment #8721342 - Flags: review?(bbirtles)
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?
Attachment #8721651 - Flags: review?(bbirtles)
(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.
Attachment #8721341 - Attachment is obsolete: true
Attachment #8721876 - Flags: review?(bbirtles)
Attachment #8721342 - Attachment is obsolete: true
Attachment #8721877 - Flags: review?(bbirtles)
Attachment #8721651 - Attachment is obsolete: true
Attachment #8721879 - Flags: review?(bbirtles)
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.
Attachment #8721876 - Flags: review?(bbirtles) → review+
(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.
Attachment #8721900 - Flags: review+
Attachment #8721876 - Attachment is obsolete: true
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.
Attachment #8721877 - Flags: review?(bbirtles)
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'
Attachment #8721879 - Flags: review?(bbirtles) → review+
Attachment #8721877 - Attachment is obsolete: true
Updated:
1. Add addStyle function in testcommon.js
2. Split tests.
Attachment #8722334 - Attachment is obsolete: true
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.
Attachment #8722336 - Flags: review?(bbirtles)
(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.
Attachment #8721879 - Attachment is obsolete: true
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.
Attachment #8722368 - Flags: review?(bbirtles)
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.
Attachment #8722336 - Flags: review?(bbirtles) → review+
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.
Attachment #8722336 - Attachment is obsolete: true
Attachment #8721900 - Attachment is obsolete: true
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>.
Attachment #8722368 - Flags: review?(bbirtles) → review+
Attachment #8722368 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.