1.23 KB, text/html; charset=UTF-8
Make sure to skip animation rules as well as transition rules when we're reparenting a style context when not processing an animation restyle.
5.43 KB, patch
|Details | Diff | Splinter Review|
2.02 KB, patch
|Details | Diff | Splinter Review|
Created attachment 560138 [details] ffcss3anim.html User Agent: Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110913 Firefox/9.0a1 Build ID: 20110913030846 Steps to reproduce: I was trying to animate the border-color property between a base value (specified out of the @keyframes ruleset) and another value in a keyframe. Actual results: After a Repaint event, the starting value of the animated property is no longer the base value, but the value which was active before the Repaint event. See the attached example: Resizing the window triggers a Repaint and "blocks" the starting value of the animation. For the first box, for example, the starting value becomes "black", and the animation therefore goes between "black" and "black". Expected results: Animation should continue properly, like it was just after the page load. The specification does indeed say that you must include rules for at least the times 0% (or from) and 100% (or to), but doesn't indicate clearly if the starting value can not be defined elsewhere (as in, NOT in the keyframe rule). This works in webkit.
Confirming. David, any idea what's up here?
If there were transitions involved, I would think that the problem is including the transition rules when reparenting or something. But that shouldn't matter for animations, should it?
And in fact we explicitly skip transition rules when reparenting. Maybe we need to do that for animations too?
If I do that, it seems to fix the bug.
So what's supposed to prevent this is that nsAnimationManager::CheckAnimationRule only calls BuildAnimations when !mPresContext->IsProcessingAnimationStyleChange(). (BuildAnimations is where we use the base value if there's nothing specified for 0% or 100%.) I think the problem is that that's not sufficient for the cases where !mPresContext->IsProcessingRestyles(). Some untested work-in-progress is at: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/96b7d4e5eda0/animation-with-restyle but I'm pretty busy right now; may not have time to finish it for a bit. (Boris, I'm curious if you think this makes sense -- my assumption is that with the async stuff we've done lately that reframes that result from processing restyles will actually lead to the new frames being created after mPresContext->IsProcessingRestyles() is no longer true -- but I could be wrong there, in which case this fix would actually be sufficient.)
(I wrote comment 5 before seeing comments 1 - 4, but submitted it anyway over the collision.)
OK, bz and I just discussed this; the reframing for restyles is sync, so bz's fix should be all that necessary. But we should have a test that triggers both the reframing and non-reframing cases.
Created attachment 560286 [details] [diff] [review] Make sure to skip animation rules as well as transition rules when we're reparenting a style context when not processing an animation restyle.
Created attachment 561317 [details] [diff] [review] test Without your patch, I get 1 failure: failed | bug 686656 test 1 at 625ms - got 100px, expected 50px and with your patch the test passes.
Comment on attachment 560286 [details] [diff] [review] Make sure to skip animation rules as well as transition rules when we're reparenting a style context when not processing an animation restyle. r=dbaron
Also, I'd like to add some assertions, but they fire too much; need to investigate: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/2f99515c87d4/unapplied.assert-check-animation-rule Also, I realize the comment in the test is a bit misleading -- the 100% case doesn't actually matter since the animation has ended by the point we test. I should probably add 'forwards' to the animation lines in both cases; then the 1000ms test should also fail without the patch. I should check that...
Created attachment 561319 [details] [diff] [review] test with -moz-animation-fill-mode: forwards
Comment on attachment 561319 [details] [diff] [review] test r=me