686656 - [css3-animations] when animations interrupted by restyle, implicit 0% or 100% values (from base value) pick up value from animation

Status: RESOLVED FIXED

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

Description

[Maxime]

Attachment: 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 [:bzbarsky]]

Confirming.  David, any idea what's up here?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

And in fact we explicitly skip transition rules when reparenting.  Maybe we need to do that for animations too?

Comment 4

[Boris Zbarsky [:bzbarsky]]

If I do that, it seems to fix the bug.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

(I wrote comment 5 before seeing comments 1 - 4, but submitted it anyway over the collision.)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 [:bzbarsky]]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: test

with -moz-animation-fill-mode: forwards

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 561319 [details] [diff] [review]
test

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1ed711ae0b35
http://hg.mozilla.org/integration/mozilla-inbound/rev/c18a3ab59221

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1ed711ae0b35
https://hg.mozilla.org/mozilla-central/rev/c18a3ab59221