Closed Bug 1197620 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox43 --- affected
firefox44 --- affected
firefox45 --- verified

People

(Reporter: hunboy, Assigned: hiro)

References

Details

Attachments

(5 files, 7 obsolete files)

1.37 KB, text/html
Details
361 bytes, text/html
Details
4.28 KB, patch
birtles
: review+
hiro
: checkin+
Details | Diff | Splinter Review
12.36 KB, patch
hiro
: review+
hiro
: checkin+
Details | Diff | Splinter Review
5.87 KB, patch
hiro
: review+
cbook
: checkin+
Details | Diff | Splinter Review
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.
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?
Component: Layout → CSS Parsing and Computation
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.
Attached file simplified testcase
Reduced testcase added, the animation is broken on the child element at second (third, etc.) try by clicking the checkbox.
I think hiro observed something similar in bug 1166500 comment 9.
Attached patch A mochitest for this issue (obsolete) — Splinter Review
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'.
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.
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.
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';
Attached patch Proof of concept (obsolete) — Splinter Review
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.
Attached patch Update mochitests (obsolete) — Splinter Review
Attachment #8651660 - Attachment is obsolete: true
dbaron, could you please give some feedback on comment#7?
Flags: needinfo?(dbaron)
(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.
Blocks: 962594
Flags: needinfo?(dbaron)
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.
Attachment #8653986 - Attachment is obsolete: true
Attachment #8653987 - Attachment is obsolete: true
Attachment #8655803 - Flags: review?(dbaron)
Attached patch Part 2: Test cases (obsolete) — Splinter Review
Test cases in this patch cover 'display:none' and '.hidden = true' for animation element and its parent element.
Attachment #8655804 - Flags: review?(dbaron)
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
Attachment #8655803 - Flags: review?(dbaron) → review+
(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)
Flags: needinfo?(cam)
Flags: needinfo?(bbirtles)
Comment on attachment 8655804 [details] [diff] [review]
Part 2: Test cases

># HG changeset patch
># User Hiroyuki Ikezoe <hiikezoe@mozilla-japan.org>
># 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.
Attachment #8655804 - Flags: review?(dbaron) → review?(bbirtles)
(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.
(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.
Flags: needinfo?(cam)
Flags: needinfo?(bbirtles)
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.
Attachment #8655804 - Flags: review?(bbirtles)
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!
Attachment #8655804 - Attachment is obsolete: true
Attachment #8659696 - Flags: review?(bbirtles)
Comment on attachment 8659696 [details] [diff] [review]
Part 2: Test cases v2

Assuming this fails without part 1, looks great. Thanks.
Attachment #8659696 - Flags: review?(bbirtles) → review+
Carrying over review+.
Addressed comment#15 and comment#18.
Attachment #8655803 - Attachment is obsolete: true
Attachment #8660209 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f18147a55e5
All of oranges are known orange.

Thank you both!
Keywords: checkin-needed
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 ?
Flags: needinfo?(hiikezoe)
Keywords: checkin-needed
Just rebased.

Thanks!
Assignee: nobody → hiikezoe
Attachment #8660209 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(hiikezoe)
Attachment #8661094 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/467631a7f944
https://hg.mozilla.org/mozilla-central/rev/7abc5253e8cf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
QA Whiteboard: [good first verify]
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 .
QA Whiteboard: [good first verify] → [good first verify] [bugday-20151104]
Confirmed it's still broken in 45.0a1 (2015-11-03) Win 7.
Status: RESOLVED → REOPENED
Flags: needinfo?(hiikezoe)
Resolution: FIXED → ---
Ah, we should also check finished animations there[1].

https://hg.mozilla.org/mozilla-central/rev/467631a7f944#l4.12
Flags: needinfo?(hiikezoe)
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.
Flags: needinfo?(bbirtles)
(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.
Flags: needinfo?(bbirtles)
(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!
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.
Attachment #8683384 - Flags: review?(bbirtles)
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?
Attachment #8683384 - Flags: review?(bbirtles) → review+
(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.
Addressed all comment #36.
nsFrame::HasCSSAnimations in nsFrame.cpp has been moved since the previous position was inside #ifdef DEBUG block.
Attachment #8683913 - Flags: review?(bbirtles)
Attachment #8683384 - Attachment is obsolete: true
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.
Attachment #8683913 - Flags: review?(bbirtles) → review+
Attachment #8659696 - Flags: checkin+
Attachment #8661094 - Flags: checkin+
Attachment #8683913 - Flags: checkin?
Attachment #8683913 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/4ba6798c125b
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Verified fixed FF 45.0a1 (2015-11-10) Win 7.
Status: RESOLVED → VERIFIED
See Also: → 1182856
You need to log in before you can comment on or make changes to this bug.