1254761 - Implement getAnimations({ subtree: true })

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This isn't specced yet, but I met up with Google in February and discussed possible changes to getAnimations().

The ideas we came up with are here: https://docs.google.com/document/d/1lQbgJ0fZYGn1_qboj59G-MOILK33z4kmBdjgtSHF2gA/edit#heading=h.cioj28lsgoa2

In summary something like this:

enum AnimationType {
  "css-animation"
  "css-transition"
  "script"
  "fill",
  "all" // "css-animation", "css-transition", "script"
};

dictionary AnimationFilter {
  (AnimationType or sequence<AnimationType>) type = "all";
  DOMString match = ""; // Matches on id (or animationName / transitionProperty?)
  bool subtree = false;
};

partial interface Animatable {
  sequence<Animation> getAnimations(AnimationFilter filter);
}

partial interface Document {
  // filter.subtree is ignored here
  sequence<Animation> getAnimations(AnimationFilter filter);
}


There's still a lot of detail to be worked out here, but the subtree part seems pretty straightforward and DevTools could use it now.

Also, we're not likely to ship getAnimations() for quite some time (not until the CSS bindings have been specced and have stabilized) so I think we're can go ahead and implement this part for now.

It's not clear if subtree should default to false or true. The argument for true is it means you don't need special behavior for Document.getAnimations(). The argument for false is consistency with the subtree argument with MutationObservers (as well as compatibility, and it's also possibly a bit more intuitive).

Comment 1

[Daisuke Akatsuka (:daisuke)]

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

Comment 2

[Daisuke Akatsuka (:daisuke)]

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 1: Implement getAnimations({ subtree: true })

Please review this.

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8729955 [details] [diff] [review]
Part 1: Implement getAnimations({ subtree: true })

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

Looks good but I want to check the changes to nsDocument::GetAnimations. It should become a *lot* simpler.

::: dom/base/Element.cpp
@@ +3416,1 @@
>  }

Nit: No need for this extra blank line here.

::: dom/base/Element.h
@@ +181,5 @@
>     * (e.g. if we're in the middle of adding it to the document or
>     * removing it from the document).
>     */
>    void UpdateState(bool aNotify);
> +

There are lots of unrelated whitespace changes in this patch. In future, please make these a separate patch (which doesn't need review) since it makes it harder to review when there are unrelated changes.

::: dom/base/nsDocument.cpp
@@ +3171,5 @@
>         node;
>         node = node->GetNextNode(this)) {
>      if (!node->IsElement()) {
>        continue;
>      }

Let's use GetRootElement here instead of GetFirstChild etc.

Comment 5

[Daisuke Akatsuka (:daisuke)]

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

Comment 6

[Daisuke Akatsuka (:daisuke)]

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

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 1: Implement getAnimations({ subtree: true })

* Use GetRootElement
* Remove extra blank line
* Split the patch to remove whitespace

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 2: Removes extra whitespaces

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8730574 [details] [diff] [review]
Part 1: Implement getAnimations({ subtree: true })

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

r=me. Please extend the test with a simple check that the animations are in order for the very last test case, however.

::: dom/animation/CSSPseudoElement.cpp
@@ +49,5 @@
>    return CSSPseudoElementBinding::Wrap(aCx, this, aGivenProto);
>  }
>  
>  void
> +CSSPseudoElement::GetAnimations(const AnimationFilter& filter,

I'm surprised we don't get unused parameter warnings here. Maybe we turned that warning off.

::: dom/animation/test/css-animations/file_element-get-animations.html
@@ +326,5 @@
> +    'should find in only the element even if it has a child');
> +  assert_equals(
> +    div.getAnimations({ subtree:true }).length, 6,
> +    'should find all descendants of the element');
> +}, 'getAnimations with AnimationFilter');

We should probably check that the animations are correctly sorted in tree order (since, in this case, they should all start at the same time).

Feel free to do that as a separate part 3 so we can land this.

Comment 10

[Daisuke Akatsuka (:daisuke)]

(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8730574 [details] [diff] [review]
> Part 1: Implement getAnimations({ subtree: true })
> 
> Review of attachment 8730574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. Please extend the test with a simple check that the animations are in
> order for the very last test case, however.

OK! I'll split the test. 

> ::: dom/animation/CSSPseudoElement.cpp
> @@ +49,5 @@
> >    return CSSPseudoElementBinding::Wrap(aCx, this, aGivenProto);
> >  }
> >  
> >  void
> > +CSSPseudoElement::GetAnimations(const AnimationFilter& filter,
> 
> I'm surprised we don't get unused parameter warnings here. Maybe we turned
> that warning off.

So, what should we do for this??

> ::: dom/animation/test/css-animations/file_element-get-animations.html
> @@ +326,5 @@
> > +    'should find in only the element even if it has a child');
> > +  assert_equals(
> > +    div.getAnimations({ subtree:true }).length, 6,
> > +    'should find all descendants of the element');
> > +}, 'getAnimations with AnimationFilter');
> 
> We should probably check that the animations are correctly sorted in tree
> order (since, in this case, they should all start at the same time).
> 
> Feel free to do that as a separate part 3 so we can land this.

OK!

Comment 11

[Brian Birtles (:birtles)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #10)
> > ::: dom/animation/CSSPseudoElement.cpp
> > @@ +49,5 @@
> > >    return CSSPseudoElementBinding::Wrap(aCx, this, aGivenProto);
> > >  }
> > >  
> > >  void
> > > +CSSPseudoElement::GetAnimations(const AnimationFilter& filter,
> > 
> > I'm surprised we don't get unused parameter warnings here. Maybe we turned
> > that warning off.
> 
> So, what should we do for this??

Nothing. It's fine.

Comment 12

[Brian Birtles (:birtles)]

Don't forget, you need DOM peer review for part 1.

Comment 13

[Daisuke Akatsuka (:daisuke)]

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

Comment 14

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 1: Implement getAnimations({ subtree: true })

Hello Olli,
Please review dom/webidl/Animatable.webidl

Comment 15

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 3: Add tests for AnimationFilter

Hi Brian,
I splited the test to this patch.
Please review.

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8731095 [details] [diff] [review]
Part 3: Add tests for AnimationFilter

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

::: dom/animation/test/css-animations/file_element-get-animations.html
@@ +311,5 @@
> +  target.style.animation = 'anim1 100s';
> +
> +  var animations = target.getAnimations({ subtree: false });
> +  assert_equals(animations.length, 1,
> +                'should find in only the element');

'Should find only the element'

@@ +313,5 @@
> +  var animations = target.getAnimations({ subtree: false });
> +  assert_equals(animations.length, 1,
> +                'should find in only the element');
> +  assert_equals(animations[0].effect.target, target,
> +                'effect target shuld be the element');

s/shuld/should/

@@ +324,5 @@
> +  target.style.animation = 'anim1 100s';
> +
> +  var animations = target.getAnimations({ subtree: true });
> +  assert_equals(animations.length, 3,
> +                'should find Pseudo element animations too');

'getAnimations({ subtree: true }) should return animations on pseudo-elements'

@@ +326,5 @@
> +  var animations = target.getAnimations({ subtree: true });
> +  assert_equals(animations.length, 3,
> +                'should find Pseudo element animations too');
> +  assert_equals(animations[0].effect.target, target,
> +                '1st effect target shuld be target');

'The animation targeting the parent element should be returned first'

@@ +328,5 @@
> +                'should find Pseudo element animations too');
> +  assert_equals(animations[0].effect.target, target,
> +                '1st effect target shuld be target');
> +  assert_equals(animations[1].effect.target.type, '::before',
> +                '2nd effect target shuld be ::before');

'The animation targeting the ::before pseudo-element should be returned second'

@@ +330,5 @@
> +                '1st effect target shuld be target');
> +  assert_equals(animations[1].effect.target.type, '::before',
> +                '2nd effect target shuld be ::before');
> +  assert_equals(animations[1].effect.target.parentElement, target,
> +                'parent of 2nd effect target shuld be target');

I'm not sure this check is needed.

@@ +332,5 @@
> +                '2nd effect target shuld be ::before');
> +  assert_equals(animations[1].effect.target.parentElement, target,
> +                'parent of 2nd effect target shuld be target');
> +  assert_equals(animations[2].effect.target.type, '::after',
> +                '3rd effect target shuld be ::after');

'The animation targeting the ::after pesudo-element should be returned last'

@@ +334,5 @@
> +                'parent of 2nd effect target shuld be target');
> +  assert_equals(animations[2].effect.target.type, '::after',
> +                '3rd effect target shuld be ::after');
> +  assert_equals(animations[2].effect.target.parentElement, target,
> +                'parent of 3rd effect target shuld be target');

I don't think this check is needed.

@@ +365,5 @@
> +                '#child2::before': 'animation: anim1 10s;',
> +                '#grandchild1::after': 'animation: anim1 10s;',
> +                '#grandchild1::before': 'animation: anim1 10s;',
> +                '#grandchild2::after': 'animation: anim1 10s;',
> +                '#grandchild2::before': 'animation: anim1 10s;' });

This seems much more complicated than it needs to be. We should be aiming for minimal test cases that cover the different permutations.

e.g. See parts 1a, 1b, 1c here:
https://dxr.mozilla.org/mozilla-central/rev/5e14887312d4523ab59c3f6c6c94a679cf42b496/layout/style/test/test_animations_event_order.html#86

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

There is some spec bug for this? I'd prefer to not land features which aren't being tracked to be spec'ed.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8731094 [details] [diff] [review]
Part 1: Implement getAnimations({ subtree: true })

So this is conditional review. If there is a spec bug filed and other browser vendors agree that we'll have this kind of API, then r+.
But make sure that spec bug is filed before this code lands.

Comment 19

[Brian Birtles (:birtles)]

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8731094 [details] [diff] [review]
> Part 1: Implement getAnimations({ subtree: true })
> 
> So this is conditional review. If there is a spec bug filed and other
> browser vendors agree that we'll have this kind of API, then r+.
> But make sure that spec bug is filed before this code lands.

Spec bug filed: https://github.com/w3c/web-animations/issues/146

This was based on discussion with Google in February where we wrote an initial spec for this feature.[1]

We're not planning on shipping this for a long time yet (we need to spec all the CSS integration first) so there is still time to change this if other vendors raise issues.

[1] https://docs.google.com/document/d/1lQbgJ0fZYGn1_qboj59G-MOILK33z4kmBdjgtSHF2gA/edit?pref=2&pli=1#heading=h.dok8i6c7jp14

Comment 20

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 3: Add tests for AnimationFilter

Revised the tests.

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8731570 [details] [diff] [review]
Part 3: Add tests for AnimationFilter

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

r=me with a test that we iterate over pseudo-elements and children in the correct order.

::: dom/animation/test/css-animations/file_element-get-animations.html
@@ +353,5 @@
> +  assert_equals(animations.length, 1,
> +                'Should find only the element even if it has a child');
> +  assert_equals(animations[0].effect.target, parent,
> +                'Effect target shuld be the element');
> +}, 'Test AnimationFilter{ subtree: false } with element that has a child');

We need a test that checks the order of pseudo-elements vs children, right?

e.g. when you have

   parent
   ::before
   ::after
     |
   child

We need to check we get parent, ::before, ::after, child -- I think that is the correct order?

We just need to extend this test to check with subtree: true I think?

Comment 22

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 1: Implement getAnimations({ subtree: true })

Comment 23

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 2: Removes extra whitespaces

Comment 24

[Daisuke Akatsuka (:daisuke)]

Attachment: Part 3: Add tests for AnimationFilter

Thanks, Brian!
I added a new test.

Comment 25

[Daisuke Akatsuka (:daisuke)]

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

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2bc470bfbc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5233a84bd57
https://hg.mozilla.org/integration/mozilla-inbound/rev/22615a6fd646

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2bc470bfbc3
https://hg.mozilla.org/mozilla-central/rev/e5233a84bd57
https://hg.mozilla.org/mozilla-central/rev/22615a6fd646