Last Comment Bug 686656 - [css3-animations] when animations interrupted by restyle, implicit 0% or 100% values (from base value) pick up value from animation
: [css3-animations] when animations interrupted by restyle, implicit 0% or 100%...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-14 05:25 PDT by Maxime
Modified: 2011-09-21 09:30 PDT (History)
5 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
ffcss3anim.html (1.23 KB, text/html; charset=UTF-8)
2011-09-14 05:25 PDT, Maxime
no flags Details
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)
2011-09-14 17:26 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
test (2.00 KB, patch)
2011-09-20 15:37 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
test (2.02 KB, patch)
2011-09-20 15:43 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
Details | Diff | Splinter Review

Description Maxime 2011-09-14 05:25:54 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2011-09-14 10:29:59 PDT
Confirming.  David, any idea what's up here?
Comment 2 Boris Zbarsky [:bz] 2011-09-14 10:32:55 PDT
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?
Comment 3 Boris Zbarsky [:bz] 2011-09-14 10:35:59 PDT
And in fact we explicitly skip transition rules when reparenting.  Maybe we need to do that for animations too?
Comment 4 Boris Zbarsky [:bz] 2011-09-14 10:37:54 PDT
If I do that, it seems to fix the bug.
Comment 5 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-14 12:12:18 PDT
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.)
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-14 12:13:14 PDT
(I wrote comment 5 before seeing comments 1 - 4, but submitted it anyway over the collision.)
Comment 7 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-14 12:35:09 PDT
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.
Comment 8 Boris Zbarsky [:bz] 2011-09-14 17:26:48 PDT
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.
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-20 15:37:34 PDT
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 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-20 15:39:04 PDT
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
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-20 15:41:38 PDT
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...
Comment 12 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-09-20 15:43:07 PDT
Created attachment 561319 [details] [diff] [review]
test

with -moz-animation-fill-mode: forwards
Comment 13 Boris Zbarsky [:bz] 2011-09-20 20:45:27 PDT
Comment on attachment 561319 [details] [diff] [review]
test

r=me

Note You need to log in before you can comment on or make changes to this bug.