1241784 - Implement CSSPseudoElement.animate()

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Boris Chiou [:boris]]

According to our spec, (CSS)PseudoElement implements Animatable;

Bug 1174575 only implemented basic attributes for (CSS)PseudoElement, so we have to implement animate() and getAnimations() [1] in (CSS)PseudoElement here.

[1] https://w3c.github.io/web-animations/#animatable

Comment 1

[Boris Chiou [:boris]]

Attachment: Part 1: Add a helper function for Element.animate()

Therefore, CSSPseudoElement.animate() can also use it.

Comment 2

[Boris Chiou [:boris]]

Attachment: Part 2: Implement CSSPseudoElement.animate()

Comment 3

[Boris Chiou [:boris]]

Attachment: Part 1: Add a helper function for Element.animate() (v2)

Therefore, CSSPseudoElement.animate() can also use it.

Comment 4

[Boris Chiou [:boris]]

Attachment: Part 3: Test

Comment 5

[Boris Chiou [:boris]]

Comment on attachment 8721653 [details] [diff] [review]
Part 3: Test

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

::: testing/web-platform/tests/web-animations/animatable/animate.html
@@ +153,5 @@
> +  div.className = '';
> +  var sheetToBeRemoved = doc.getElementById('pseudoTest');
> +  var sheetParent = sheetToBeRemoved.parentNode;
> +  sheetParent.removeChild(sheetToBeRemoved);
> +}

I am not sure if adding/removing a css rule for pseudo element dynamically is a good way in web-platform-tests. (Maybe we can hack a solution to achieve this in the future.)
I don't have a good idea to get a pseudo element by other ways, so use these two functions to achieve the purpose.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8721652 [details] [diff] [review]
Part 1: Add a helper function for Element.animate() (v2)

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

::: dom/base/Element.cpp
@@ +3322,5 @@
> +{
> +  if (aTarget.IsNull()) {
> +    aError.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

There are a number of error code paths here that we don't need. As best possible, we shouldn't add code paths we can't test and especially when we can simply avoid creating error cases altogether.

As far as I understand, there are only two call sites for this: Element::Animate and CSSPseudoElement::Animate, both of which we control, both of which will never need a null target, and both of which should never pass an uninitialized target.

So, we should:

1. Make the argument type of aTarget just ElementOrCSSPseudoElement&.
2. Assert that aTarget is either an element a CSS pseudo element by putting a MOZ_ASSERT_UNREACHABLE in your else block below.
3. Wrap aTarget in Nullable when we pass it to the constructor.

::: dom/base/Element.h
@@ +833,5 @@
> +          JS::Handle<JSObject*> aFrames,
> +          const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
> +          ErrorResult& aError);
> +  static already_AddRefed<Animation>
> +  Animate(const Nullable<ElementOrCSSPseudoElement>& aTarget,

This needs a comment pointing out that it's a helper method that factors out the common functionality needed by Element::Animate and CSSPseudoElement::Animate.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8721653 [details] [diff] [review]
Part 3: Test

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

::: testing/web-platform/tests/web-animations/animatable/animate.html
@@ +143,5 @@
> +    '.before::before { content: ""; animation: anim 10s; }';
> +  doc.body.appendChild(style);
> +  div.className = 'before';
> +  return doc.getAnimations()[0].effect.target;
> +}

Why not use the CSSOM like this:

https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/layout/style/test/test_animations_event_order.html#161

@@ +145,5 @@
> +  div.className = 'before';
> +  return doc.getAnimations()[0].effect.target;
> +}
> +
> +function removeStyleOnPseudoElement(div, doc) {

Instead of making each test call this cleanup function, register it automatically in createStyleOnPseudoElement:

https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/testing/web-platform/tests/web-animations/testcommon.js#44

@@ +179,5 @@
> +  assert_class_string(anim.effect, 'KeyframeEffect',
> +                      'Returned Animation has a KeyframeEffect');
> +  removeStyleOnPseudoElement(div);
> +}, 'CSSPseudoElement.animate() creates an Animation object with a ' +
> +   'KeyframeEffect');

I don't think we need to repeat all these tests. It's a maintenance burden to try and keep two copies in sync.

Either we should refactor the first set of tests to do everything on both an Element and a CSSPseudoElement, or just work out what we really need to test with regards to pseudo elements (assuming that most implementations will re-use mostly the same code paths for either case.) In this case, I think probably the first two tests are enough.

Comment 8

[Boris Chiou [:boris]]

Attachment: Part 1: Add a helper function for Element.animate() (v3)

Therefore, CSSPseudoElement.animate() can also use it.

Updated: address birtles' comments

Comment 9

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #7)
> Why not use the CSSOM like this:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> af6356a3e8c56036b74ba097395356d9c6e6c5a3/layout/style/test/
> test_animations_event_order.html#161

Thanks. I should follow the same rules as the test cases in Bug 1234403.


> I don't think we need to repeat all these tests. It's a maintenance burden
> to try and keep two copies in sync.
> 
> Either we should refactor the first set of tests to do everything on both an
> Element and a CSSPseudoElement, or just work out what we really need to test
> with regards to pseudo elements (assuming that most implementations will
> re-use mostly the same code paths for either case.) In this case, I think
> probably the first two tests are enough.

I agree. Just test the basic functions for CSSPseudoElement::Animate() because we indeed use the same code paths for both Element::Animate() and CSSPseudoElement::Animate().

Thanks.

Comment 10

[Boris Chiou [:boris]]

Attachment: Part 1: Add a helper function for Element.animate() (v4)

Therefore, CSSPseudoElement.animate() can also use it.

Comment 11

[Boris Chiou [:boris]]

Attachment: Part 2: Implement CSSPseudoElement.animate(). (v2, carry birtles' r+)

Rebased.

Comment 12

[Boris Chiou [:boris]]

Attachment: Part 3: Test (v2)

Comment 13

[Boris Chiou [:boris]]

Attachment: Part 3: Test (v3)

Comment 14

[Boris Chiou [:boris]]

Attachment: Part 3: Test (v4)

Comment 15

[Boris Chiou [:boris]]

Attachment: Part 3: Test (v5)

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8721924 [details] [diff] [review]
Part 1: Add a helper function for Element.animate() (v4)

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

r=birtles with comments addressed.

::: dom/base/Element.cpp
@@ +3323,5 @@
> +{
> +  // We don't support operator= and copy constructor for
> +  // ElementOrCSSPseudoElement, so we have to set it as Element or
> +  // CSSPseudoElement manually.
> +  Nullable<ElementOrCSSPseudoElement> targetWrapper;

In that case I take back forget my comments about passing a bare ElementOrCSSPseudoElement. This is too messy. I still think we shouldn't add code paths we can't test, however.

We should just make the start of this method:

  MOZ_ASSERT(!aTarget.IsNull() &&
             (aTarget.Value().IsElement() ||
              aTarget.Value().IsCSSPseudoElement()),
             "aTarget should be initialized");

  RefPtr<Element> referenceElement;
  if (aTarget.Value().IsElement()) {
    referenceElement = &aTarget.Value().GetAsElement();
  } else {
    referenceElement = aTarget.Value().GetAsCSSPseudoElement().ParentElement();
  }

  nsCOMPtr<nsIGlobalObject> ownerGlobal = referenceElement->GetOwnerGlobal();
  ...

::: dom/base/Element.h
@@ +831,5 @@
> +  Animate(JSContext* aContext,
> +          JS::Handle<JSObject*> aFrames,
> +          const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
> +          ErrorResult& aError);
> +  // A helper method that factors out the common functionality needed by

Nit: Add a blank line before this

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8723423 [details] [diff] [review]
Part 3: Test (v5)

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

::: testing/web-platform/tests/web-animations/animatable/animate.html
@@ +128,5 @@
> +// Tests on CSSPseudoElement
> +
> +test(function(t) {
> +  createStyle(t, {'@keyframes anim': '',
> +                  '.before::before': 'animation: anim 10s;'});

We normally put spaces on either side of braces in Javascript.[1]

I know this is web-platform-tests so Gecko coding styles don't necessarily apply, but I think it makes it easier to read.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects

@@ +131,5 @@
> +  createStyle(t, {'@keyframes anim': '',
> +                  '.before::before': 'animation: anim 10s;'});
> +  var div = createDiv(t);
> +  div.classList.add('before');
> +  // Check if addAnimationOnPseudoElement adds an animation on a

What is addAnimationOnPseudoElement?

@@ +138,5 @@
> +                'Add an animation on a pseudo element successfully');
> +  assert_class_string(document.getAnimations()[0].effect.target,
> +                      'CSSPseudoElement',
> +                      'The returned target is a CSSPseudoElement');
> +}, 'Create a CSSPseudoElement successfully')

This doesn't seem to belong as a separate test in animatable/animate.html, right?

In the long-term, hopefully there will be a way to create a CSSPseudoElement directly. So perhaps we should add a helper method to testcommon.js that calls createDiv and createStyle, fetches the pseudo element, asserts that it exists and has the right type, cancels the generated animation and then returns the object?

e.g.

function createPseudo(type) {
  // createDiv
  // createStyle (uses type)
  // assert_equals(document.getAnimations().length, 1)
  // assert_class_string(...)
  // (In future might be able to use div.getAnimations({subtree: true}))
  // anim = document.getAnimations()[0]
  // anim.cancel();
  // return anim.effect.target;
}

Then when there is a more direct way of creating CSSPseudoElement objects we can just update/drop that method?

What do you think?

::: testing/web-platform/tests/web-animations/testcommon.js
@@ +46,5 @@
>    });
>    return div;
>  }
>  
> +// Creates a style sheet and appends it to the document head

"Creates a style element with the specified rules, appends it to the document head and
removes the created element during test cleanup."

You should probably also give an example of the format of |rules| too.

@@ +69,4 @@
>  // Removes element
>  function removeElement(element) {
>    element.parentNode.removeChild(element);
>  }

Nit: createDiv calls this removeElement() function. It was something the UniPro guys added which I opposed at the time.

Now that every user agent that is likely to implement this supports remove() we should probably drop 'removeElement' and update createDiv. But we can do that later if you want to keep this patch simple.

Comment 18

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #16)
> In that case I take back forget my comments about passing a bare
> ElementOrCSSPseudoElement. This is too messy. I still think we shouldn't add
> code paths we can't test, however.
> 
> We should just make the start of this method:
> 
>   MOZ_ASSERT(!aTarget.IsNull() &&
>              (aTarget.Value().IsElement() ||
>               aTarget.Value().IsCSSPseudoElement()),
>              "aTarget should be initialized");
> 
>   RefPtr<Element> referenceElement;
>   if (aTarget.Value().IsElement()) {
>     referenceElement = &aTarget.Value().GetAsElement();
>   } else {
>     referenceElement =
> aTarget.Value().GetAsCSSPseudoElement().ParentElement();
>   }
> 
>   nsCOMPtr<nsIGlobalObject> ownerGlobal = referenceElement->GetOwnerGlobal();
>   ...

Thanks for your example. I will use Nullable<ElementOrCSSPseudoElement> as the function argument directly and add the assertion.

Comment 19

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #17)
> What is addAnimationOnPseudoElement?

Oops, I forgot to remove this comment.

> In the long-term, hopefully there will be a way to create a CSSPseudoElement
> directly. So perhaps we should add a helper method to testcommon.js that
> calls createDiv and createStyle, fetches the pseudo element, asserts that it
> exists and has the right type, cancels the generated animation and then
> returns the object?
> 
> e.g.
> 
> function createPseudo(type) {
>   //...
> }
> 
> Then when there is a more direct way of creating CSSPseudoElement objects we
> can just update/drop that method?
> 
> What do you think?

I agree. We could move this part into testcommon.js

> Nit: createDiv calls this removeElement() function. It was something the
> UniPro guys added which I opposed at the time.
> 
> Now that every user agent that is likely to implement this supports remove()
> we should probably drop 'removeElement' and update createDiv. But we can do
> that later if you want to keep this patch simple.

OK. I will update a new individual patch (part 4) to remove removeElement() and use Element.remove() directly.

Comment 20

[Boris Chiou [:boris]]

Attachment: Part 1: Add a helper function for Element.animate(). (v5, carry birtles' r+)

Therefore, CSSPseudoElement.animate() can also use it.

Updated: address Brian's comments.

Comment 21

[Boris Chiou [:boris]]

Attachment: Part 2: Implement CSSPseudoElement.animate(). (v3, carry birtles' r+)

Rebased.

Comment 22

[Boris Chiou [:boris]]

Attachment: Part 3: Test (v6)

Comment 23

[Boris Chiou [:boris]]

Attachment: Part 4: Remove removeElement from testcommon.js

Comment 24

[Boris Chiou [:boris]]

Comment on attachment 8726635 [details] [diff] [review]
Part 4: Remove removeElement from testcommon.js

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

Looks like only createDiv() uses removeElement().

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8726634 [details] [diff] [review]
Part 3: Test (v6)

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

r=me with that possible tweak to createPseudo if you agree it makes sense.

::: testing/web-platform/tests/web-animations/testcommon.js
@@ +81,5 @@
> +  createStyle(test, { '@keyframes anim': '',
> +                      ['.pseudo::' + type]: 'animation: anim 10s;' });
> +  var div = createDiv(test);
> +  div.classList.add('pseudo');
> +  assert_equals(document.getAnimations().length, 1);

Should this just get the last animation in the list and verify that it has the right target? Then you could call createPseudo in a test that has already generated an animation?

Comment 26

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #25)
> > +  assert_equals(document.getAnimations().length, 1);
> 
> Should this just get the last animation in the list and verify that it has
> the right target? Then you could call createPseudo in a test that has
> already generated an animation?

OK. I agree. We can remove this assert (no need to check we have '1' animation) and revise the second one to:

var anims = document.getAnimations();
assert_class_string(anims[anims.length-1].effect.target, 'CSSPseudoElement');

So we just check the last animation and make sure it is a CSSPseudoElement target.

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Chiou [:boris]  from comment #26)
> (In reply to Brian Birtles (:birtles) from comment #25)
> > > +  assert_equals(document.getAnimations().length, 1);
> > 
> > Should this just get the last animation in the list and verify that it has
> > the right target? Then you could call createPseudo in a test that has
> > already generated an animation?
> 
> OK. I agree. We can remove this assert (no need to check we have '1'
> animation) and revise the second one to:
> 
> var anims = document.getAnimations();
> assert_class_string(anims[anims.length-1].effect.target, 'CSSPseudoElement');
> 
> So we just check the last animation and make sure it is a CSSPseudoElement
> target.

Sounds good. We could also assert that:
* anims.length >= 1
* effect.target.parentElement equals |div|
* effect.target.type equals |type| (although this would be redundant since if we passed the above test there couldn't possibly be another animation targeting |div|)

The only reason that I'm suggesting these assertions is that if we start using this in a complex test, we might find that the newly added animation is *not* the last one in getAnimations(). If that's the case, it would be good to know that (rather than have the test fail mysteriously, or pass for the wrong reason).

Comment 28

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #27)
> Sounds good. We could also assert that:
> * anims.length >= 1
> * effect.target.parentElement equals |div|
> * effect.target.type equals |type| (although this would be redundant since
> if we passed the above test there couldn't possibly be another animation
> targeting |div|)

Cool. Thanks. Let's use these three assertions.

> 
> The only reason that I'm suggesting these assertions is that if we start
> using this in a complex test, we might find that the newly added animation
> is *not* the last one in getAnimations(). If that's the case, it would be
> good to know that (rather than have the test fail mysteriously, or pass for
> the wrong reason).

Got it. It makes sense.

Comment 29

[Boris Chiou [:boris]]

Attachment: Part 3: Test. (v7, carry birtles' r+)

Updated: address birtles' comments.

Comment 30

[Boris Chiou [:boris]]

Attachment: Part 4: Remove removeElement from testcommon.js. (v1, carry birtles' r+)

Rebased.

Comment 31

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c31ffcf7874b

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bb7c63a1154
https://hg.mozilla.org/integration/mozilla-inbound/rev/e289c18ace28
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a3f5ba34b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c532283a04f

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9bb7c63a1154
https://hg.mozilla.org/mozilla-central/rev/e289c18ace28
https://hg.mozilla.org/mozilla-central/rev/e1a3f5ba34b3
https://hg.mozilla.org/mozilla-central/rev/2c532283a04f