Closed Bug 1254761 Opened 8 years ago Closed 8 years ago

Implement getAnimations({ subtree: true })

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: birtles, Assigned: daisuke)

References

Details

Attachments

(3 files, 6 obsolete files)

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).
Assignee: nobody → daisuke
Please review this.
Attachment #8729955 - Flags: review?(bbirtles)
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.
Attachment #8729955 - Flags: review?(bbirtles)
* Use GetRootElement
* Remove extra blank line
* Split the patch to remove whitespace
Attachment #8730574 - Flags: review?(bbirtles)
Attachment #8729955 - Attachment is obsolete: true
Attachment #8730575 - Flags: review?(bbirtles)
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.
Attachment #8730574 - Flags: review?(bbirtles) → review+
Attachment #8730575 - Flags: review?(bbirtles) → review+
(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!
(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.
Don't forget, you need DOM peer review for part 1.
Hello Olli,
Please review dom/webidl/Animatable.webidl
Attachment #8731094 - Flags: review?(bugs)
Attachment #8730574 - Attachment is obsolete: true
Hi Brian,
I splited the test to this patch.
Please review.
Attachment #8731095 - Flags: review?(bbirtles)
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
Attachment #8731095 - Flags: review?(bbirtles)
There is some spec bug for this? I'd prefer to not land features which aren't being tracked to be spec'ed.
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.
Attachment #8731094 - Flags: review?(bugs) → review+
(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
Status: NEW → ASSIGNED
Revised the tests.
Attachment #8731570 - Flags: review?(bbirtles)
Attachment #8731095 - Attachment is obsolete: true
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?
Attachment #8731570 - Flags: review?(bbirtles) → review+
Attachment #8731094 - Attachment is obsolete: true
Attachment #8730575 - Attachment is obsolete: true
Thanks, Brian!
I added a new test.
Attachment #8731570 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 1206420
You need to log in before you can comment on or make changes to this bug.