CSS animation plays only once when showing/hiding parent element

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: axelboc, Assigned: hiro)

Tracking

({regression, testcase})

49 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

Attachments

(2 attachments)

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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
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.
Component: Untriaged → CSS Parsing and Computation
Tested on FF Nighlty 53.0a1, Aurora 52.0a2, Beta 51.0b7 and 50 release and all are affected.
birtles, any thought?
Flags: needinfo?(bbirtles)
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.
Flags: needinfo?(bbirtles) → needinfo?(hiikezoe)
Alice, would you be able to help with the regression range for this?

So far we have:
48: OK
50: NG
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
Flags: needinfo?(hiikezoe)
Marking this as P1 since we should fix this regression in the current release.
Priority: -- → P1
I have confirmed this regression window, it is mentioned in comment 6.
Has Regression Range: --- → yes
Has STR: --- → yes
Version: 50 Branch → 49 Branch
Blocks: 1268385
(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.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
(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.
Note that the new test case in attachment 8819762 [details] fails without fix.
Thanks for the quick fix! Can you explain what the problem is and why this patch fixes it?
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.
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)
Thanks, that's exactly the sort of information I was looking for.
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
Attachment #8819762 - Flags: review?(bbirtles) → review+
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/962332e7c58e
Destroy animations on hidden elements even if the animation has been already finished. r=birtles
https://hg.mozilla.org/mozilla-central/rev/962332e7c58e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Can we nominate this for aurora/beta too? Thanks!
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
Attachment #8819762 - Flags: approval-mozilla-beta?
Attachment #8819762 - Flags: approval-mozilla-aurora?
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.
Attachment #8819762 - Flags: approval-mozilla-beta?
Attachment #8819762 - Flags: approval-mozilla-beta+
Attachment #8819762 - Flags: approval-mozilla-aurora?
Attachment #8819762 - Flags: approval-mozilla-aurora+
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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.