1197620 - [animation] sub-animation doesnt work after re-activation the parent animation

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Robert Utasi [:hunboy]]

Attachment: modal subanimation scale testcase

When two animation is defined to the parent and to the child, after reactivating the child animation is ignored.

Testcase:

Repro: open the modal by "click me" button and close it, reopen it.

A.R.: scale animation is broken at the second (third, etc.) trying. Only opacity animation works on the parent.

E.R.: modal scale animates all the time when you open the modal.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are you aware of this having worked in the past (i.e., you know it's a regression), or did you just discover it and don't know whether it worked before?

Comment 2

[Robert Utasi [:hunboy]]

David, I've finished with the mozregression, but failed. It's not a regression, never worked while animation is implemented.

Cross browser info:

Chromium works properly.
IE11 engine is permanently broken, needs resize the window for reflow, to start the algorthyhm at all. Anyway works properly when starts at all.

Comment 3

[Robert Utasi [:hunboy]]

Attachment: simplified testcase

Reduced testcase added, the animation is broken on the child element at second (third, etc.) try by clicking the checkbox.

Comment 4

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

I think hiro observed something similar in bug 1166500 comment 9.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: A mochitest for this issue

I think this issue is the same as 1) described in bug 11665500 #0. 
The problem is that animation keeps running when the display property of its parent element is set to 'none'.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I finally found out the reason why CheckAnimationRule is called when 'display:none' is set to the parent element in test_animations.html [1].

[1] http://hg.mozilla.org/mozilla-central/file/c46370eea81a/layout/style/test/test_animations.html#l2009

The reason is that GetPropertyValue() is called in the test. GetPropertyValue eventually calls CheckAnimationRule. So CheckAnimationRule is not normally called when 'display:none' is set to an ancestor element of animation.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

We should force to stop animations inside 'display:none' subtree in somewhere, and force to start (calling CheckAnimationRule) when the display property of ancestor elements is updated from 'none'.

A couple of questions and thoughts:

a) I think ElementRestyler::RestyleSelf() is a good place to call such process because there are old and new style context, so we can detect the changes of the display property. Is there any other more proper place to call such process?

b) RestyleManager::RestyleElement seems not to be called for elements inside 'display:none' subtree. Is this right? If so we can not use IsInDisplayNoneSubtree() at that time.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Shouldn't we destroy AnimationCollection when 'display:none' is set? For example:

 var animation = element.getAnimtions()[0];
 animation.oncancel = function() {
   element.style.display = ''; // a new animation is created here.
   // the previous animation instance is not valid any more. 
 };
 element.style.display = 'none';

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proof of concept

This is a proof of concept what I commented in comment#7.

This patch does not solve the issue mentioned in comment#8 yet. Actually I have been trying to solve the issue but it is harder than I thought.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Update mochitests

Comment 11

[Hiroyuki Ikezoe (:hiro)]

dbaron, could you please give some feedback on comment#7?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> We should force to stop animations inside 'display:none' subtree in
> somewhere, and force to start (calling CheckAnimationRule) when the display
> property of ancestor elements is updated from 'none'.
> 
> A couple of questions and thoughts:
> 
> a) I think ElementRestyler::RestyleSelf() is a good place to call such
> process because there are old and new style context, so we can detect the
> changes of the display property. Is there any other more proper place to
> call such process?
> 
> b) RestyleManager::RestyleElement seems not to be called for elements inside
> 'display:none' subtree. Is this right? If so we can not use
> IsInDisplayNoneSubtree() at that time.

I think that's not a great place because of the problem you point out in (b) -- that this won't catch descendants of elements that change to display:none, since for those elements we destroy the frames without any restyling.  Additionally, to maintain frame tree consistency, we never set style contexts on frames that would indicate that they're becoming display:none, so it's hard to tell at frame destruction time if it's about to be recreated (in which case we don't want to stop animations) or if it's becoming display:none.

The trick is to try to handle this without doing a lot of extra work.

One possibility, although it's a little bit of a hack, is that whenever we destroy a frame *during restyle processing* **and** that frame has animations, we stick the relevant content node in in a hashtable (probably separate tables for element/::before::after), somewhat like the tables in ReframingStyleContexts, except used less intensively.  Then, during RestyleManager::EndProcessingRestyles, we go through those elements, and for any that still don't have a frame, we stop their animations.

I haven't yet thought of anything simpler, although there may well be something.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Stop all animations in destroyed frames

dbaron, thank you for the kindly guide!

This patch adds AnimationsInDestroyedFrame class to stop all animation in destroyed frames.
The AnimationsInDestroyedFrame is very similar to ReframingStyleContexts but it uses arrays instead of hashtables because the class does not need nsStyleContext at all in case of destroying animations.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Test cases

Test cases in this patch cover 'display:none' and '.hidden = true' for animation element and its parent element.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8655803 [details] [diff] [review]
Part 1: Stop all animations in destroyed frames

~RestyleManager should assert that mAnimationsInDestroyedFrame is null,
with assertion text like "leaving dangling pointers from
AnimationsInDestroyedFrame.

(And the same for mReframingStyleContexts, actually.)

>+  MOZ_ASSERT(!mRestyleManager->mReframingStyleContexts,
>+             "shouldn't construct recursively");

This should be mAnimationsInDestroyedFrame rather than
mReframingStyleContexts.

StopAllAnimationsInArray should be renamed to
StopAnimationsWithoutFrame.  (The InArray isn't needed, and it only
stops the animations whose elements don't have a frame rather than all
animations.)  Or it could have the same name as the public method, i.e.,
StopAnimationsForElementsWithoutFrames.

Likewise StopAllAnimations should be renamed to
StopAnimationsForElementsWithoutFrames.

>+  for (auto& content : aArray) {

Please use for (nsIContent* content : aArray) (at least assuming it
works).

>+  // AnimationsInDestoryedFrame is used for stopping all animations in
>+  // destroyed frames.

Destoryed -> Destroyed
(although I also suggest renaming below)

And "for stopping all animations in destroyed frames" -> "to stop
animations on elements that have no frame at the end of the restyling
process"

>+  // It lives only during restyling process.

"lives only" -> "only lives"
"restyling process" -> "the restyling process"


>+        if (!mContents.Contains(aContent)) {
>+          mContents.AppendElement(aContent);
>+        }
(three times, essentially)

Doing a containment check on an array before appending to it is
very expensive, since it makes the algorithm as a whole O(N^2).

You should either switch the arrays to being hashtables (which are what
ReframingStyleContexts uses) or you should make it ok to have the same
element twice, or you should make it an assertion (debug-only) that the
element is not already in the array.

TODO: COME BACK TO THIS TO SUGGEST WHICH.

>+    nsTArray<nsRefPtr<nsIContent>> mContents;
>+    nsTArray<nsRefPtr<nsIContent>> mBeforeContents;
>+    nsTArray<nsRefPtr<nsIContent>> mAfterContents;

I don't think what you're doing here will work correctly for the before
and after animations, since the content node will get recreated during a
reframe, and you will either (a) incorectly stop animations that you
shouldn't stop or (b) fail to stop animations that you should stop,
since you're looking them up by the wrong element.

You should:
 (1) document that the Put method takes the content node for the
 generated content for animations on ::before and ::after, rather
 than the content node for the real element.

 (2) document that mBeforeContents and mAfterContents hold the real
 element rather than the content node for the generated content (which
 might change during a reframe)

 (3) change Put to call GetParent() for the before and after cases (but
 not the element case)

 (4) Document nsAnimationManager::StopAnimationsForElement the same as (2).

Please rename the class AnimationsInDestroyedFrame to
AnimationsWithDestroyedFrame, and the member the same way (In->With).

>+  // Create a AnimationsInDestoryedFrame during restyling process to
>+  // stop all animations in destroyed frames.

Destoryed -> Destroyed

And "to stop all animations in destroyed frames" -> "to stop
animations on elements that have no frame at the end of the restyling
process"

>+  if (nsLayoutUtils::HasCurrentAnimations(static_cast<nsIFrame*>(this))) {

No need for the static_cast; passing this directly will work.

>+    // If there are animations still running during restyling process,
>+    // stop all those animations.

Replace this with:  If no new frame for this element is created by the
end of the restyling process, stop animations for this frame.

>+    // AnimationsInDestoryedFrame lives only during restyling process.

Destoryed -> Destroyed

>+  nsAutoAnimationMutationBatch mb(aElement->OwnerDoc());

You should ask heycam and/or birtles whether you're better off having
a single one of these around all of the animations that are stopped
rather than having one for all of them.  If so, you should move this
to the caller's caller, and document that this method expects its caller
to have an nsAutoAnimationMutationBatch.  (You should also double-check
with them that it's *safe* to do what this does here.)


r=dbaron with those things fixed

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #15)
> Comment on attachment 8655803 [details] [diff] [review]
> Part 1: Stop all animations in destroyed frames
> 
...
> >+  nsAutoAnimationMutationBatch mb(aElement->OwnerDoc());
> 
> You should ask heycam and/or birtles whether you're better off having
> a single one of these around all of the animations that are stopped
> rather than having one for all of them.  If so, you should move this
> to the caller's caller, and document that this method expects its caller
> to have an nsAutoAnimationMutationBatch.  (You should also double-check
> with them that it's *safe* to do what this does here.)

(or I can ask directly)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8655804 [details] [diff] [review]
Part 2: Test cases

># HG changeset patch
># User Hiroyuki Ikezoe <[e-mail]>
># Parent  73f18361a11df0ca491e437bac78c2733f9af5a2
>Bug 1197620 - Part 2 tests that aniamtion stop playing when its element is not displayed.
>
>This tests cover that the element is 'display:none' and '.hidden = true' cases,
>and its parent element is 'display:none' and '.hidden = true' cases.


I'm going to punt this review to birtles (sorry, should have done that sooner).

But before I do, two comments:

(1) aniamtion -> animation in the commit message

(2) It's not worth testing 'display: none' vs. the HTML 'hidden' attribute separately.  The hidden attribute just sets display:none.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #15)
> Comment on attachment 8655803 [details] [diff] [review]
> Part 1: Stop all animations in destroyed frames
...
> >+        if (!mContents.Contains(aContent)) {
> >+          mContents.AppendElement(aContent);
> >+        }
> (three times, essentially)
> 
> Doing a containment check on an array before appending to it is
> very expensive, since it makes the algorithm as a whole O(N^2).
> 
> You should either switch the arrays to being hashtables (which are what
> ReframingStyleContexts uses) or you should make it ok to have the same
> element twice, or you should make it an assertion (debug-only) that the
> element is not already in the array.
> 
> TODO: COME BACK TO THIS TO SUGGEST WHICH.

Oops, I forgot to come back to this TODO.

I think it should be very rare (although I don't think it's impossible) to add an element twice.  So I think it's better to just keep the arrays and not do a Contains check, but just add a comment to the code pointing out that it might need to deal with elements that have already had their animations stopped.  I think the current code handles that case fine.

Comment 19

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

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #15)
> >+  nsAutoAnimationMutationBatch mb(aElement->OwnerDoc());
> 
> You should ask heycam and/or birtles whether you're better off having
> a single one of these around all of the animations that are stopped
> rather than having one for all of them.  If so, you should move this
> to the caller's caller, and document that this method expects its caller
> to have an nsAutoAnimationMutationBatch.  (You should also double-check
> with them that it's *safe* to do what this does here.)

This should be fine. In this case we're visiting each element only once so moving the nsAutoAnimationMutationBatch isn't going to help (in fact, it would make us do more work since we'd then have to decide the order for visiting all the elements with batched records, and that involves potentially expensive sorting--but only in the case where we actually have observers, i.e. the animations inspector panel is open).

With regards to safety, I think this is ok. It will mean we end up keeping a ref to all the removed animations and their targets but that should be fine: we can still destroy the collection while keeping the animations alive.

Comment 20

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

Comment on attachment 8655804 [details] [diff] [review]
Part 2: Test cases

>Bug 1197620 - Part 2 tests that aniamtion stop playing when its element is not displayed.

As dbaron points out, s/aniamtion/animation/

>This tests cover that the element is 'display:none' and '.hidden = true' cases,
>and its parent element is 'display:none' and '.hidden = true' cases.

This comment needs to be update to remove the reference to .hidden.

>diff --git a/dom/animation/test/mozilla/file_hide_and_show.html b/dom/animation/test/mozilla/file_hide_and_show.html
>new file mode 100644
...
>+function stop_animation_test(test, animation, stopFunc, description) {

I know we have a lot of these functions_names_with_underscores but I'm pretty sure the coding style says javascript functions should use camelCase.[1]
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

Do you think making the function start with a verb would be more clear? e.g. stopAndTest?

We should possibly use the aTest naming too but that seems to be pretty loosely followed in tests so I think it's ok to leave this as-is.

>+  animation.ready.then(function() {

this should use test.step_func

Also, why do we need to wait on ready?

>+    stopFunc();
>+    return animation.ready;
>+  }).then(test.step_func(function() {
>+    assert_equals(animation.playState, 'idle', description);

I think you just need to flush styles here, right? I'm not sure waiting on ready is the right thing.

Calling animation.playState on a CSSAnimation or CSSTransition should flush styles anyway, so we shouldn't need to do anything right?

>+function restart_animation_test(test, animationElement,
>+                                stopFunc, restartFunc,
>+                                description) {

restartAnimationTest. Or, perhaps, stopRestartAndTest?

>+  var animation = animationElement.getAnimations()[0];
>+
>+  animation.oncancel = test.step_func(function() {
>+    restartFunc();
>+
>+    // FIXME: The original animation object should be used here.
>+    var animation = animationElement.getAnimations()[0];

I don't understand this comment. A new animation will be generated, right?

>+    animation.ready.then(test.step_func(function() {
>+      assert_equals(animation.playState, 'running',
>+        'Animation should start again when its parent element gets shown');

Could we just test for 'pending' and not waiting on ready?

Instead of testing playState, we could just test that getAnimations() returns
an empty array after stopFunc() but returns a non-empty array after restartFunc().

>+      test.done();
>+    }));
>+  });
>+
>+  animation.ready.then(function() {

Again, why do we need to wait on ready here?

>+    stopFunc();
>+  });
>+}
>+
>+async_test(function(t) {
>+  var div = addDiv(test, { style: 'animation: move 100s infinite' });
>+  var animation = div.getAnimations()[0];
>+
>+  stop_animation_test(t, animation, function() {
>+    div.style.display = 'none';
>+  }, 'Animation should stop playing when the element style display is set ' +
>+     'to "none"');
>+}, 'Animation stops playing when the element style display is set to "none"');

I find factoring out a separate stop_animations_test method here makes the test a little harder to follow. Could we just write this test as:

test(function(t) {
  var div = addDiv(test, { style: 'animation: move 100s infinite' });
  var animation = div.getAnimations()[0];

  div.style.display = 'none';
  assert_equals(animation.playState, 'idle',
                'Animation should stop playing when the element style display is set ' +
                'to "none"');
});

Or even:

test(function(t) {
  var div = addDiv(test, { style: 'animation: move 100s infinite' });
  assert_equals(div.getAnimations().length, 1, 'display:initial element has animations'); 

  div.style.display = 'none';
  assert_equals(div.getAnimations().length, 0, 'display:none element has no animations'); 
});

?

Likewise for other uses of stopAnimationsTest.

>+async_test(function(t) {
>+  var div = addDiv(test, { style: 'animation: move 100s infinite' });
>+  var animation = div.getAnimations()[0];
>+
>+  stop_animation_test(t, animation, function() {
>+    div.hidden = true;
>+  }, 'Animation should stop playing when the element gets hidden');
>+}, 'Animation stops playing when the element gets hidden');

As dbaron points out, we don't need this test.

>+async_test(function(t) {
>+  var parentElement = addDiv(t);
>+  var div = document.createElement('div');

We can just addDiv again right? i.e.

  var div = addDiv(t);

Likewise for the other uses of createElement layer.

>+async_test(function(t) {
>+  var parentElement = addDiv(t);
>+  var div = document.createElement('div');
>+
>+  parentElement.appendChild(div);
>+  div.style.animation = 'move 100s infinite';
>+
>+  var animation = div.getAnimations()[0];
>+
>+  stop_animation_test(t, animation, function() {
>+    parentElement.hidden = true;
>+  }, 'Animation should stop playing when its parent element gets hidden');
>+}, 'Animation stops playing when its parent element gets hidden');

We don't need this test.

>+async_test(function(t) {
>+  var div = addDiv(t, { style: 'animation: move 100s infinite' });
>+  var animation = div.getAnimations()[0];
>+
>+  restart_animation_test(t, div, function() {
>+    div.style.display = 'none';
>+  }, function() {
>+    div.style.display = '';
>+  }, 'Animation should start again when the element style display is set ' +
>+     'to "none"');
>+}, 'Animation starts playing when the element gets shown from ' +
>+   '"display:none" state');

Depending on how much we simplify restart_animation_test, it might be more clear to just
write the whole test code here.

>+async_test(function(t) {
>+  var div = addDiv(t, { style: 'animation: move 100s infinite' });
>+  var animation = div.getAnimations()[0];
>+
>+  restart_animation_test(t, div, function() {
>+    div.hidden = true;
>+  }, function() {
>+    div.hidden = false;
>+  }, 'Animation should start again when the element gets shown');
>+}, 'Animation starts playing when the element gets shown from hidden state');

We don't need this test.

>+async_test(function(t) {
>+  var parentElement = addDiv(t);
>+  var div = document.createElement('div');
>+
>+  parentElement.appendChild(div);
>+  div.style.animation = 'move 100s infinite';
>+
>+  var animation = div.getAnimations()[0];
>+
>+  restart_animation_test(t, div, function() {
>+    parentElement.style.display = 'none';
>+  }, function() {
>+    parentElement.style.display = '';
>+  }, 'Animation should start again when its parent element gets shown');
>+}, 'Animation starts playing when its parent element gets shown from ' +
>+   '"display:none" state');

Nit: s/gets shown/is shown/

>+async_test(function(t) {
>+  var parentElement = addDiv(t);
>+  var div = document.createElement('div');
>+
>+  parentElement.appendChild(div);
>+  div.style.animation = 'move 100s infinite';
>+
>+  restart_animation_test(t, div, function() {
>+    parentElement.hidden = true;
>+  }, function() {
>+    parentElement.hidden = false;
>+  }, 'Animation should start again when its parent element gets shown');
>+}, 'Animation starts playing when its parent element gets shown from ' +
>+   'hidden state');

We don't need this test.


Apart from that, looks good but I'd like to have one more look after those changes.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Test cases v2

Thank you, Brian! I never thought we can test without checking playState.

Now tests get much simpler.

(In reply to Brian Birtles (:birtles) from comment #20)
> Comment on attachment 8655804 [details] [diff] [review]
> test(function(t) {
>   var div = addDiv(test, { style: 'animation: move 100s infinite' });
>   assert_equals(div.getAnimations().length, 1, 'display:initial element has
> animations'); 
> 
>   div.style.display = 'none';
>   assert_equals(div.getAnimations().length, 0, 'display:none element has no
> animations'); 
> });

All of tests could be rewritten as this pattern.
And tests for 'hidden' were removed. I did not know that 'hidden' sets display:none. Thank you, David!

Comment 22

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

Comment on attachment 8659696 [details] [diff] [review]
Part 2: Test cases v2

Assuming this fails without part 1, looks great. Thanks.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Stop all animations on elements that have no frame v2

Carrying over review+.
Addressed comment#15 and comment#18.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f18147a55e5
All of oranges are known orange.

Thank you both!

Comment 25

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

applying StopAnimationInDestroyedFrames.patch
patching file layout/base/RestyleManager.cpp
Hunk #1 FAILED at 84
1 out of 3 hunks FAILED -- saving rejects to file layout/base/RestyleManager.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh StopAnimationInDestroyedFrames.patch

maybe it need rebase ?

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Stop all animations on elements that have no frame v3

Just rebased.

Thanks!

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/467631a7f944
https://hg.mozilla.org/integration/mozilla-inbound/rev/7abc5253e8cf

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/467631a7f944
https://hg.mozilla.org/mozilla-central/rev/7abc5253e8cf

Comment 29

[Ashickur Rahman]

What I have understand from the comment 0 is, when I first active the animation both parent and child animation is working. But from the second try it is not working. 

If I have understand right, I have successfully reproduce the bug in Firefox 39.0; Build ID: 20150629114836; User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:39.0) Gecko/20100101 Firefox/39.0 . 

But the fix is not working for me in: Firefox: 43.0b1; Build ID : 20151103023037; User Agent : Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0 .

Comment 30

[Paul Silaghi, QA [:pauly]]

Confirmed it's still broken in 45.0a1 (2015-11-03) Win 7.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Ah, we should also check finished animations there[1].

https://hg.mozilla.org/mozilla-central/rev/467631a7f944#l4.12

Comment 32

[Hiroyuki Ikezoe (:hiro)]

From CSS Animation spec:
 Setting the display property to none will terminate any *running* animation applied to the element and its descendants.

And
 If an element has a display of none, updating display to a value other than none will start all animations applied to the element by the animation-name property

So I think using HasCurrentAnimations is correct. We should restart animations which are no longer running if the target element style is changed from display:none?

There is one more thing that I am wondering about. It's Animation.playState. The playState should stay 'finished' or 'idle' when the target element style changes to display:none?

For reference on current trunk the playState changes to 'idle' in case of the target element is set to display:none, the playState stays to 'finished' in case of parent of the target element is set to display:none. The latter case is actually this bug's case.

Brian, what do you think about the playState behavior?

If we take the 'idle' case I will post an additional patch to check that the target frame has any kind of animations instead of current animations in this bug. 
If we take the 'finished' case I will open a new bug for it.

Comment 33

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> From CSS Animation spec:
>  Setting the display property to none will terminate any *running* animation
> applied to the element and its descendants.
> 
> And
>  If an element has a display of none, updating display to a value other than
> none will start all animations applied to the element by the animation-name
> property
> 
> So I think using HasCurrentAnimations is correct. We should restart
> animations which are no longer running if the target element style is
> changed from display:none?

I guess what the spec should say is that *all* animations are terminated, and that updating display -> !none (re)starts all animations. I can fix that if you agree that's what we want.

That appears to be what Chrome and Edge do.

Right, so HasCurrentAnimations won't return true for finished animations. We really need to just get all CSS animations.

> There is one more thing that I am wondering about. It's Animation.playState.
> The playState should stay 'finished' or 'idle' when the target element style
> changes to display:none?

It should go to 'idle' because it has been cancelled. I tested Chrome Canary and it goes to idle there.

> If we take the 'idle' case I will post an additional patch to check that the
> target frame has any kind of animations instead of current animations in
> this bug. 

Thanks, please do that.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #33)

> > If we take the 'idle' case I will post an additional patch to check that the
> > target frame has any kind of animations instead of current animations in
> > this bug. 
> 
> Thanks, please do that.

Thank you! I will post the patch with some tests here and ask you to review it!

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Terminate all animations if correcponding element style is changed to display:none

This is it.

In the tests, there is no assertion to check that div.getAnimations().length equals to zero after display:none is set. As you know, finished animations are not included in div.getAnimations(). We should use Document.getAnimations once bug 1212720 is fixed. *BUT* Document.getAnimations will return animations on display:none elements?

I believe that:
 Document.getAnimations returns finished animations
 Document.getAnimations does not returns animations on elements which are not attached to the document

And I believe Document.getAnimations does not returns animations on display:none elements. 
But in the test cases it's enough to check that a finished animation restart when display style is set to !none.

Comment 36

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

Comment on attachment 8683384 [details] [diff] [review]
Part 3: Terminate all animations if correcponding element style is changed to display:none

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

::: dom/animation/test/mozilla/file_hide_and_show.html
@@ +70,5 @@
>  }, 'Animation starts playing when its parent element is shown from ' +
>     '"display:none" state');
>  
> +test(function(t) {
> +  var div = addDiv(t, { style: 'animation: move 100s 1' });

Why do we need the '1' here? Won't the iteration-count become 1 anyway?

@@ +84,5 @@
> +
> +  div.style.display = '';
> +  assert_equals(div.getAnimations().length, 1,
> +                'Element which is no longer display:none has animations ' +
> +                'again');

I think this test might be more clear if we set a forwards-fill on the animation (since forwards-filling animations are still not current)?

Then we could test something like:

  1. finish()
     Test: getAnimations().length() === 1
  2. display -> 'none'
     Test: getAnimations().length() === 0
  3. display -> ''
     Test: getAnimations().length() === 1

Also, it would be good to also test that the newly-generated animation (after setting display = '' again) has a different object identity that the original animation (i.e. div.getAnimations()[0] !== animation).

@@ +107,5 @@
> +  assert_equals(div.getAnimations().length, 1,
> +                'Element which is no longer in display:none subtree has ' +
> +                'animations again');
> +}, 'Animation which has already finished starts playing when its parent ' +
> +   'element is shown from "display:none" state');

Depending on what you decide to do for the previous test, this test will probably need updating too.

::: layout/generic/nsFrame.cpp
@@ +10148,5 @@
>    DR_state = nullptr;
>  }
>  
> +bool
> +nsFrame::HasAnimations()

I would prefer to call this HasCSSAnimations() since we are really only interested in CSS animations in this case, not transitions or (soon to come) script-generated animations.

I'll rename this in bug 1190235 unless you want to do it here.

I don't really know if this is better on nsLayoutUtils or on nsFrame but I get the impression these sort of utility methods are often added to nsLayoutUtils? What do you think?

Comment 37

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #36)
> Comment on attachment 8683384 [details] [diff] [review]
> Part 3: Terminate all animations if correcponding element style is changed
> to display:none
> 
> Review of attachment 8683384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/test/mozilla/file_hide_and_show.html
> @@ +70,5 @@
> >  }, 'Animation starts playing when its parent element is shown from ' +
> >     '"display:none" state');
> >  
> > +test(function(t) {
> > +  var div = addDiv(t, { style: 'animation: move 100s 1' });
> 
> Why do we need the '1' here? Won't the iteration-count become 1 anyway?

I did not know that the iteration-count is 1 by default.

> @@ +84,5 @@
> > +
> > +  div.style.display = '';
> > +  assert_equals(div.getAnimations().length, 1,
> > +                'Element which is no longer display:none has animations ' +
> > +                'again');
> 
> I think this test might be more clear if we set a forwards-fill on the
> animation (since forwards-filling animations are still not current)?
> 
> Then we could test something like:
> 
>   1. finish()
>      Test: getAnimations().length() === 1
>   2. display -> 'none'
>      Test: getAnimations().length() === 0
>   3. display -> ''
>      Test: getAnimations().length() === 1

That's a really nice idea. I will do it. I soon forget the fact that forwards-fill animations are still in the list of getAnimations() even after the animations has been finished.

> Also, it would be good to also test that the newly-generated animation
> (after setting display = '' again) has a different object identity that the
> original animation (i.e. div.getAnimations()[0] !== animation).

Thanks, I will do it.

> ::: layout/generic/nsFrame.cpp
> @@ +10148,5 @@
> >    DR_state = nullptr;
> >  }
> >  
> > +bool
> > +nsFrame::HasAnimations()
> 
> I would prefer to call this HasCSSAnimations() since we are really only
> interested in CSS animations in this case, not transitions or (soon to come)
> script-generated animations.

That makes sense. I will rename to HasCSSAnimations.

> I'll rename this in bug 1190235 unless you want to do it here.
> 
> I don't really know if this is better on nsLayoutUtils or on nsFrame but I
> get the impression these sort of utility methods are often added to
> nsLayoutUtils? What do you think?

I think this method should be moved to nsLayoutUtils when it is called from other places. Until that time I'd like to make it a private method of nsFrame.

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Terminate *all* animations if corresponding element style is changed to display:none

Addressed all comment #36.
nsFrame::HasCSSAnimations in nsFrame.cpp has been moved since the previous position was inside #ifdef DEBUG block.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8683913 [details] [diff] [review]
Part 3: Terminate *all* animations if corresponding element style is changed to display:none

Carrying over review+
I am sorry for review request.

Comment 40

[Hiroyuki Ikezoe (:hiro)]

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

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba6798c125b

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ba6798c125b

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ba6798c125b

Comment 44

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 45.0a1 (2015-11-10) Win 7.