Bug 26869 – Add fill modes for CSS Animations (don't reset state after the animation finishes)

• Component: CSS
• Status: RESOLVED FIXED

[Andy Matuschak]

The following animation seems simple:
- box 1 slides on-screen from the left
- then box 2 slides on-screen from the right
- (they stay there)

But because the styles of animated objects are reset after the animation finishes, this is not easily solved without Javascript or other ugliness.

Core Animation has fill modes which specify this kind of behavior: http://developer.apple.com/documentation/graphicsimaging/Reference/CAMediaTiming_protocol/Introduction/Introduction.html#//apple_ref/doc/constant_group/Fill_Modes

Something like this would really help build nice sequential animations.

[Simon Fraser (smfr)]

*** Bug 32465 has been marked as a duplicate of this bug. ***

[Dean Jackson]

See http://lists.w3.org/Archives/Public/www-style/2010Mar/0010.html

[Dean Jackson]

Created attachment 49843 [details]
Patch and testcases for fill-mode

[WebKit Review Bot]

Attachment 49843 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/animation/Animation.h:62:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/animation/Animation.h:63:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/animation/Animation.h:102:  More than one command on the same line  [whitespace/newline] [4]
WebCore/page/animation/AnimationBase.cpp:1031:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/animation/AnimationBase.cpp:1036:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/css/CSSParser.cpp:2578:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/animation/KeyframeAnimation.cpp:116:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/animation/KeyframeAnimation.cpp:128:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/animation/KeyframeAnimation.cpp:129:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/animation/KeyframeAnimation.cpp:241:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/animation/KeyframeAnimation.cpp:247:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/css/CSSStyleSelector.cpp:5665:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/page/animation/AnimationBase.h:74:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:75:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:89:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/animation/Animation.cpp:114:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/animation/Animation.cpp:120:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 17 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 49848 [details]
updated patch (fixing some style errors)

I fixed some style errors. I left the rest because they follow the style of the existing file and would require more substantial changes for no real gain.

[WebKit Review Bot]

Attachment 49848 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/animation/Animation.h:102:  More than one command on the same line  [whitespace/newline] [4]
WebCore/page/animation/AnimationBase.cpp:1036:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/page/animation/AnimationBase.h:74:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:75:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:89:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/animation/Animation.cpp:114:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/animation/Animation.cpp:120:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 7 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 49857 [details]
added convenience methods for testing fill mode

I left the style errors in after re-reading the thread on webkit-dev about not touching code outside the domain of the patch for style fixes.

[WebKit Review Bot]

Attachment 49857 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/animation/Animation.h:102:  More than one command on the same line  [whitespace/newline] [4]
WebCore/page/animation/AnimationBase.h:74:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:75:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:89:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/animation/Animation.cpp:114:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/animation/Animation.cpp:120:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 6 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 49857 [details]
added convenience methods for testing fill mode

> diff --git a/LayoutTests/animations/fill-mode-transform.html b/LayoutTests/animations/fill-mode-transform.html

> +      setTimeout("snapshot('a', 0)", 100);
> +      setTimeout("snapshot('a', 1)", 500);
> +      setTimeout("snapshot('a', 2)", 1350);
> +      setTimeout("snapshot('b', 3)", 100);
> +      setTimeout("snapshot('b', 4)", 500);
> +      setTimeout("snapshot('b', 5)", 1350);
> +      setTimeout("snapshot('c', 6)", 100);
> +      setTimeout("snapshot('c', 7)", 500);
> +      setTimeout("snapshot('c', 8)", 1350);
> +      setTimeout("snapshot('d', 9)", 100);
> +      setTimeout("snapshot('d', 10)", 500);
> +      setTimeout("snapshot('d', 11)", 1350);

A test like this will fail on the bots sporadically. We shouldn't add any new animation tests that rely on setTimeout.

> diff --git a/WebCore/css/CSSComputedStyleDeclaration.cpp b/WebCore/css/CSSComputedStyleDeclaration.cpp
> index 8039e35..407450c 100644
> --- a/WebCore/css/CSSComputedStyleDeclaration.cpp
> +++ b/WebCore/css/CSSComputedStyleDeclaration.cpp
> @@ -146,6 +146,7 @@ static const int computedProperties[] = {
>      CSSPropertyWebkitAnimationDelay,
>      CSSPropertyWebkitAnimationDirection,
>      CSSPropertyWebkitAnimationDuration,
> +    CSSPropertyWebkitAnimationFillMode,
>      CSSPropertyWebkitAnimationIterationCount,
>      CSSPropertyWebkitAnimationName,
>      CSSPropertyWebkitAnimationPlayState,
> @@ -1198,6 +1199,24 @@ PassRefPtr<CSSValue> CSSComputedStyleDeclaration::getPropertyCSSValue(int proper
>          }
>          case CSSPropertyWebkitAnimationDuration:
>              return getDurationValue(style->animations());
> +        case CSSPropertyWebkitAnimationFillMode: {
> +            RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
> +            const AnimationList* t = style->animations();
> +            if (t) {
> +                for (size_t i = 0; i < t->size(); ++i) {
> +                    if (t->animation(i)->fillMode() == Animation::AnimationFillModeNone)
> +                        list->append(CSSPrimitiveValue::createIdentifier(CSSValueNone));
> +                    else if (t->animation(i)->fillMode() == Animation::AnimationFillModeForwards)
> +                        list->append(CSSPrimitiveValue::createIdentifier(CSSValueForwards));
> +                    else if (t->animation(i)->fillMode() == Animation::AnimationFillModeBackwards)
> +                        list->append(CSSPrimitiveValue::createIdentifier(CSSValueBackwards));
> +                    else
> +                        list->append(CSSPrimitiveValue::createIdentifier(CSSValueBoth));

Use a switch()?

> diff --git a/WebCore/css/CSSStyleSelector.cpp b/WebCore/css/CSSStyleSelector.cpp

> +    CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +    switch (primitiveValue->getIdent()) {
> +

Extra blank line here.

> diff --git a/WebCore/page/animation/AnimationBase.cpp b/WebCore/page/animation/AnimationBase.cpp

>                  }
> +
>              } else {

Extra line here.

> diff --git a/WebCore/page/animation/KeyframeAnimation.cpp b/WebCore/page/animation/KeyframeAnimation.cpp

> @@ -233,12 +235,12 @@ void KeyframeAnimation::endAnimation()
>  #if USE(ACCELERATED_COMPOSITING)
>      if (m_object->hasLayer()) {
>          RenderLayer* layer = toRenderBoxModelObject(m_object)->layer();
> -        if (layer->isComposited())
> +        if (layer->isComposited() && !m_animation->fillsForwards())
>              layer->backing()->animationFinished(m_keyframes.animationName());

Why are we calling endAnimation() if it fills fowrards?

> diff --git a/WebCore/platform/animation/Animation.h b/WebCore/platform/animation/Animation.h

> @@ -119,6 +129,7 @@ private:
>      int m_iterationCount;
>      double m_delay;
>      double m_duration;
> +    AnimationFillMode m_fillMode : 2;

Put this next to the other bitfield member to optimize packing.

> diff --git a/WebCore/platform/graphics/mac/GraphicsLayerCA.mm b/WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> index 22e39f5..9e79b8f 100644
> --- a/WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> +++ b/WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> @@ -1859,12 +1859,20 @@ void GraphicsLayerCA::setupAnimation(CAPropertyAnimation* propertyAnim, const An
>      else if (anim->direction() == Animation::AnimationDirectionAlternate)
>          repeatCount /= 2;
>  
> +    NSString* fillMode = kCAFillModeRemoved;
> +    if (anim->fillMode() == Animation::AnimationFillModeBackwards)
> +       fillMode = kCAFillModeBackwards;
> +    else if (anim->fillMode() == Animation::AnimationFillModeForwards)
> +       fillMode = kCAFillModeForwards;
> +    else if (anim->fillMode() == Animation::AnimationFillModeBoth)
> +       fillMode = kCAFillModeBoth;

switch()?

r=me, if you can explain why calling endAnimation() is correct even when filling forwards, and fix up the layout tests not rely on a lot of setTimeouts.

[Dean Jackson]

> r=me, if you can explain why calling endAnimation() is correct even when
> filling forwards

That's because the state machine calls endAnimation when it no longer needs to do calculations - basically when it enters the Done state (and now FillingForwards). We still want to fire the events and make the last paint (because we may not have painted at progress == 1)

> and fix up the layout tests not rely on a lot of setTimeouts.

This is a pretty huge task, since it would require fixing the pause animation API - which doesn't handle animation-delay (and delay is essential to test filling backwards). This sounds like a separate bug.

[Dean Jackson]

We found a bug. Don't commit this patch.

[Dean Jackson]

Comment on attachment 49857 [details]
added convenience methods for testing fill mode

cancelling review

[Dean Jackson]

Created attachment 49979 [details]
updated patch

Cleaned up the patch a lot. Removal of animations now works. Also, endAnimation() is unchanged, with other parts of the code guarding against entering it.

The tests now do not use timeouts (*), which should make the bots happy. They are slightly less comprehensive as a result, but too bad. Added an extra test for animation removal.

(*) There is one timeout, but that is a zero delay just to give style a chance to resolve. The result of the test is not dependent on timing.

[WebKit Review Bot]

Attachment 49979 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/animation/Animation.h:102:  More than one command on the same line  [whitespace/newline] [4]
WebCore/page/animation/AnimationBase.h:74:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:75:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:89:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/animation/Animation.cpp:116:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/animation/Animation.cpp:120:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 50001 [details]
fill mode patch

minor build error

[WebKit Review Bot]

Attachment 50001 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/animation/Animation.h:102:  More than one command on the same line  [whitespace/newline] [4]
WebCore/page/animation/AnimationBase.h:74:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:75:  One space before end of line comments  [whitespace/comments] [5]
WebCore/page/animation/AnimationBase.h:89:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/animation/Animation.cpp:116:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/animation/Animation.cpp:120:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Marrin]

Comment on attachment 50001 [details]
fill mode patch

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index ed01ea2..03b5f1b 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,32 @@
> [phone]  Dean Jackson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Reviewed by Simon Fraser and Chris Marrin.
> +
> +        Bug 26869: Add fill modes for CSS Animations
> +        https://bugs.webkit.org/show_bug.cgi?id=26869
>                  
>                  if (m_object) {
> -                    resumeOverriddenAnimations();
> +                    if (m_animation->fillsForwards())
> +                        m_animState = AnimationStateFillingForwards;
> +                    else
> +                        resumeOverriddenAnimations();

The interaction between fill-mode and resumed transitions needs to be tested. I don't think it will work in it's current form. When you're in AnimationStateFillingForwards and the animation is destroyed, you probably need to call resumeOverriddenAnimations().

> +        case AnimationStateFillingForwards:
>          case AnimationStateDone:
>              // We're done. Stay in this state until we are deleted
>              break;

Maybe getting an event in this state should call resumeOverriddenAnimations()?

[Dean Jackson]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/animations/computed-style-expected.txt
	M	LayoutTests/animations/computed-style.html
	A	LayoutTests/animations/fill-mode-expected.txt
	A	LayoutTests/animations/fill-mode-removed-expected.txt
	A	LayoutTests/animations/fill-mode-removed.html
	A	LayoutTests/animations/fill-mode-transform-expected.txt
	A	LayoutTests/animations/fill-mode-transform.html
	A	LayoutTests/animations/fill-mode.html
	M	LayoutTests/animations/fill-unset-properties-expected.txt
	M	LayoutTests/animations/fill-unset-properties.html
	M	LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt
	M	LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt
	M	LayoutTests/platform/mac/fast/css/getComputedStyle/computed-style-expected.txt
	M	LayoutTests/platform/mac/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt
	M	LayoutTests/svg/css/getComputedStyle-basic-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/css/CSSComputedStyleDeclaration.cpp
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSParser.h
	M	WebCore/css/CSSPropertyNames.in
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/CSSStyleSelector.h
	M	WebCore/css/CSSValueKeywords.in
	M	WebCore/page/animation/AnimationBase.cpp
	M	WebCore/page/animation/AnimationBase.h
	M	WebCore/page/animation/KeyframeAnimation.cpp
	M	WebCore/platform/animation/Animation.cpp
	M	WebCore/platform/animation/Animation.h
	M	WebCore/platform/animation/AnimationList.cpp
	M	WebCore/platform/graphics/mac/GraphicsLayerCA.mm
Committed r55576

[Eric Seidel (no email)]

This broke the leopard bots:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r55576%20(11326)/animations/fill-mode-transform-pretty-diff.html

[Eric Seidel (no email)]

Bots have been broken for 9 hours.  I'll email Dino and Simon, but I think we should roll this out.

http://trac.webkit.org/changeset/55576

[Dean Jackson]

Let's not rollout just yet. I'll update the Leopard expected results. In fact, they are the correct results. It's actually caused by another bug (mentioned in the diagnosis 35714)

[Dean Jackson]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/mac-leopard/Skipped
Committed r55596

Skipped the test on leopard. Other tests hit the code.

[Eric Seidel (no email)]

Thank you for fixing.