Closed Bug 1323121 Opened 5 years ago Closed 5 years ago
CSS animation plays only once when showing/hiding parent element
Reduced example, easy to reproduce the problem related to focus (in comment 1), but hard to reproduce this issue itself
1.41 KB, text/html
Bug 1323121 - Destroy animations on hidden elements even if the animation has been already finished.
58 bytes, text/x-review-board-request
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
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.
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
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
(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.  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.
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/962332e7c58e Destroy animations on hidden elements even if the animation has been already finished. r=birtles
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
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.
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.
You need to log in before you can comment on or make changes to this bug.