1323121 - CSS animation plays only once when showing/hiding parent element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Axel]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021

Steps to reproduce:

See this pen for a reduced test case of the issue, and a couple of ineffective workarounds I've tried: http://codepen.io/anon/pen/BQOMvP

The test case involves two elements with a simple opacity-based reveal animation. The characteristics of the animation don't seem to matter (the example makes use of the `backwards` fill mode).

JavaScript is then used to show and hide these two elements via the `hidden` HTML attribute. The attribute is set on the first element itself, but for the second element it is set on a parent `div`.


Actual results:

- The first time the elements are shown, the reveal animation works as expected on both elements.
- The second time, however, the animation doesn't work on the second element (the one wrapped in a `div`): the element is shown instantaneously with an opacity of 1.


Expected results:

When showing a previously hidden element, any CSS animation declared on a child of this element should be played again whether or not it has been played once before. This is the behaviour in Chrome, and it was the behaviour in Firefox until a few versions ago (can't remember which unfortunately).

Comment 1

[ovidiu boca[:Ovidiu]]

I can reproduce this on Mac OS X 10.10 with FF Nightly 53.0a1(2016-12-15) and FF 50 release. 
In my case if I didn't changed the page focus everything worked as expected, but when I changed the page focus, beatwin first time and second time, the bug was there.

Comment 2

[ovidiu boca[:Ovidiu]]

Tested on FF Nighlty 53.0a1, Aurora 52.0a2, Beta 51.0b7 and 50 release and all are affected.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

birtles, any thought?

Comment 4

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

Hiro knows how we handle display:none elements much better than I. Redirecting.

I notice it works correctly in Firefox 48 but not Firefox 50 so there is a regression somewhere in that range.

Comment 5

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

Alice, would you be able to help with the regression range for this?

So far we have:
48: OK
50: NG

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I guess this change, replacing "HasCSSAnimations() || HasCSSTransitions()" with "EffectSet::GetEffectSet(this)" was an overkill.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0041bc955a8d#l5.30

Comment 7

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

Marking this as P1 since we should fix this regression in the current release.

Comment 8

[YF (Yang)]

I have confirmed this regression window, it is mentioned in comment 6.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Reduced example, easy to reproduce the problem related to focus (in comment 1), but hard to reproduce this issue itself

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> I guess this change, replacing "HasCSSAnimations() || HasCSSTransitions()"
> with "EffectSet::GetEffectSet(this)" was an overkill.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0041bc955a8d#l5.30

I am convinced that the line should be "HasCSSAnimations() || HasCSSTransitions() || EffectSet::GetEffectSet(this)" because animations (script animations, CSS animations and transitions) are removed from EffectSet when they reached to its end.

But what I don't still understand is why this problem does happen only when ancestor element gets hidden/visible and why this problem is related to focus.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)

> But what I don't still understand is why this problem does happen only when
> ancestor element gets hidden/visible 

Ah, I missed that we destroy animations at different place when the target element get hidden[1].

[1] https://hg.mozilla.org/mozilla-central/file/863c2b61bd27/layout/style/nsAnimationManager.cpp#l405

And we did also destroy animations in descendant hidden elements in bug 1197620.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1323121 - Destroy animations on hidden elements even if the animation has been already finished.

We have added a test case in bug 1197620 that finished animation with
fill:forwards on hidden elements restarts when the element gets visible, but
it did not catch this bug. We should have added a case without fill:forwards.

Review commit: https://reviewboard.mozilla.org/r/99402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99402/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Note that the new test case in attachment 8819762 [details] fails without fix.

Comment 14

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

Thanks for the quick fix! Can you explain what the problem is and why this patch fixes it?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

The problem is that we remove finished animation (CSS animation) from EffectSet before we reach nsFrame::DestroyFrom() that I referred in comment 6.  So if an CSS animation(transition) finished and the target element gets hidden *after* the finish, we did not destroy CSSAnimationCollection (CSSTransitionCollection) that is associated with the element at all.  So, when the target element gets visible again, the collection is still there (the same animation name). As the result, animations in the test case in comment 0 did not run at all after the first run.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Also side note about script animation case:
In case of script animation, we just clear isRunningOnCompositor flag when the target element gets hidden, it just needs when the script animation is *running* on the compositor, i.e. the animation is in EffectSet.  (After the animation finished we don't need to clear the flag I think. If it's cleared and *if* we notify it to the devtools the lightning bolt will disappear soon after animations finish)

Comment 17

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

Thanks, that's exactly the sort of information I was looking for.

Comment 18

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

Comment on attachment 8819762 [details]
Bug 1323121 - Destroy animations on hidden elements even if the animation has been already finished.

https://reviewboard.mozilla.org/r/99402/#review99724

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/962332e7c58e
Destroy animations on hidden elements even if the animation has been already finished. r=birtles

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/962332e7c58e

Comment 21

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

Can we nominate this for aurora/beta too? Thanks!

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8819762 [details]
Bug 1323121 - Destroy animations on hidden elements even if the animation has been already finished.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1268385
[User impact if declined]: Finished CSS Animations never re-start even if an ancestor element changes to visible from hidden.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  I don't think so
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change just returned the condition that had unintentionally filtered finished CSS animations/transitions out in bug 1268385.
[String changes made/needed]: None

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8819762 [details]
Bug 1323121 - Destroy animations on hidden elements even if the animation has been already finished.

Fix a regression related to CSS animation. Beta51+ & Aurora52+. Should be in 51 beta 10.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18ca74ed9f75

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ead5b2370bb

Comment 26

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Hiroyuki Ikezoe's assessment on manual testing needs (see Comment 22) and the fact that this fix has automated coverage.