Closed
Bug 1254761
Opened 7 years ago
Closed 7 years ago
Implement getAnimations({ subtree: true })
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: birtles, Assigned: daisuke)
References
Details
Attachments
(3 files, 6 obsolete files)
8.33 KB,
patch
|
Details | Diff | Splinter Review | |
3.36 KB,
patch
|
Details | Diff | Splinter Review | |
7.44 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → daisuke
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf408665bf3c
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6debabcaa6d5
Reporter | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd85bb13a23e
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a30b063a71c6
Assignee | ||
Comment 7•7 years ago
|
||
* Use GetRootElement * Remove extra blank line * Split the patch to remove whitespace
Attachment #8730574 -
Flags: review?(bbirtles)
Assignee | ||
Updated•7 years ago
|
Attachment #8729955 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8730575 -
Flags: review?(bbirtles)
Reporter | ||
Comment 9•7 years ago
|
||
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+
Reporter | ||
Updated•7 years ago
|
Attachment #8730575 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 10•7 years ago
|
||
(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!
Reporter | ||
Comment 11•7 years ago
|
||
(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.
Reporter | ||
Comment 12•7 years ago
|
||
Don't forget, you need DOM peer review for part 1.
Assignee | ||
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3bdf00c01ee
Assignee | ||
Comment 14•7 years ago
|
||
Hello Olli, Please review dom/webidl/Animatable.webidl
Attachment #8731094 -
Flags: review?(bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8730574 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
Hi Brian, I splited the test to this patch. Please review.
Attachment #8731095 -
Flags: review?(bbirtles)
Reporter | ||
Comment 16•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
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•7 years ago
|
||
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+
Reporter | ||
Comment 19•7 years ago
|
||
(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
Assignee | ||
Updated•7 years ago
|
Attachment #8731095 -
Attachment is obsolete: true
Reporter | ||
Comment 21•7 years ago
|
||
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+
Assignee | ||
Comment 22•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8731094 -
Attachment is obsolete: true
Assignee | ||
Comment 23•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8730575 -
Attachment is obsolete: true
Assignee | ||
Comment 24•7 years ago
|
||
Thanks, Brian! I added a new test.
Assignee | ||
Updated•7 years ago
|
Attachment #8731570 -
Attachment is obsolete: true
Assignee | ||
Comment 25•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2af835f0ba11
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 26•7 years ago
|
||
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
Keywords: checkin-needed
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b2bc470bfbc3 https://hg.mozilla.org/mozilla-central/rev/e5233a84bd57 https://hg.mozilla.org/mozilla-central/rev/22615a6fd646
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•