Closed Bug 1241784 Opened 4 years ago Closed 4 years ago

Implement CSSPseudoElement.animate()

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: boris, Assigned: boris)

References

()

Details

Attachments

(4 files, 13 obsolete files)

6.24 KB, patch
boris
: review+
Details | Diff | Splinter Review
1.14 KB, patch
boris
: review+
Details | Diff | Splinter Review
3.66 KB, patch
boris
: review+
Details | Diff | Splinter Review
1.61 KB, patch
boris
: review+
Details | Diff | Splinter Review
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
Depends on: 1096773
Depends on: 1234403
Assignee: nobody → boris.chiou
Therefore, CSSPseudoElement.animate() can also use it.
Attachment #8720199 - Attachment is obsolete: true
Attached patch Part 3: Test (obsolete) — Splinter Review
Attachment #8721652 - Flags: review?(bbirtles)
Attachment #8720200 - Flags: review?(bbirtles)
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.
Attachment #8721653 - Flags: review?(bbirtles)
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.
Attachment #8721652 - Flags: review?(bbirtles)
Attachment #8720200 - Flags: review?(bbirtles) → review+
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.
Attachment #8721653 - Flags: review?(bbirtles)
Attachment #8721652 - Attachment is obsolete: true
(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.
Therefore, CSSPseudoElement.animate() can also use it.
Attachment #8721902 - Attachment is obsolete: true
Attachment #8720200 - Attachment is obsolete: true
Attachment #8721653 - Attachment is obsolete: true
Attachment #8721926 - Attachment is obsolete: true
Attachment #8722381 - Attachment is obsolete: true
Attachment #8721924 - Flags: review?(bbirtles)
Attached patch Part 3: Test (v5) (obsolete) — Splinter Review
Attachment #8723422 - Attachment is obsolete: true
Attachment #8723423 - Flags: review?(bbirtles)
Status: NEW → ASSIGNED
Summary: Implement Animatable interface for (CSS)PseudoElement → Implement CSSPseudoElement.animate()
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
Attachment #8721924 - Flags: review?(bbirtles) → review+
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.
Attachment #8723423 - Flags: review?(bbirtles)
(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.
(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.
Therefore, CSSPseudoElement.animate() can also use it.

Updated: address Brian's comments.
Attachment #8726632 - Flags: review+
Attachment #8721924 - Attachment is obsolete: true
Attachment #8721925 - Attachment is obsolete: true
Attached patch Part 3: Test (v6) (obsolete) — Splinter Review
Attachment #8723423 - Attachment is obsolete: true
Attachment #8726634 - Flags: review?(bbirtles)
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().
Attachment #8726635 - Flags: review?(bbirtles)
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?
Attachment #8726634 - Flags: review?(bbirtles) → review+
Attachment #8726635 - Flags: review?(bbirtles) → review+
(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.
(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).
(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.
Updated: address birtles' comments.
Attachment #8727241 - Flags: review+
Attachment #8726634 - Attachment is obsolete: true
Attachment #8726635 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.