Bug 26869 - Add fill modes for CSS Animations (don't reset state after the animation finishes)
Summary: Add fill modes for CSS Animations (don't reset state after the animation fini...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Enhancement
Assignee: Dean Jackson
URL:
Keywords:
: 32465 (view as bug list)
Depends on:
Blocks: 35772 35815
  Show dependency treegraph
 
Reported: 2009-06-30 16:23 PDT by Andy Matuschak
Modified: 2010-03-05 15:11 PST (History)
7 users (show)

See Also:


Attachments
Patch and testcases for fill-mode (44.46 KB, patch)
2010-03-02 13:56 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
updated patch (fixing some style errors) (44.34 KB, patch)
2010-03-02 14:15 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
added convenience methods for testing fill mode (44.26 KB, patch)
2010-03-02 15:24 PST, Dean Jackson
dino: review-
Details | Formatted Diff | Diff
updated patch (51.70 KB, patch)
2010-03-03 19:32 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
fill mode patch (51.71 KB, patch)
2010-03-04 02:56 PST, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Matuschak 2009-06-30 16:23:04 PDT
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.
Comment 1 Simon Fraser (smfr) 2009-12-14 16:13:26 PST
*** Bug 32465 has been marked as a duplicate of this bug. ***
Comment 3 Dean Jackson 2010-03-02 13:56:04 PST
Created attachment 49843 [details]
Patch and testcases for fill-mode
Comment 4 WebKit Review Bot 2010-03-02 14:00:34 PST
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.
Comment 5 Dean Jackson 2010-03-02 14:15:26 PST
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.
Comment 6 WebKit Review Bot 2010-03-02 14:20:02 PST
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.
Comment 7 Dean Jackson 2010-03-02 15:24:50 PST
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.
Comment 8 WebKit Review Bot 2010-03-02 15:28:26 PST
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.
Comment 9 Simon Fraser (smfr) 2010-03-02 15:41:26 PST
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.
Comment 10 Dean Jackson 2010-03-02 16:15:17 PST
> 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.
Comment 11 Dean Jackson 2010-03-03 10:55:00 PST
We found a bug. Don't commit this patch.
Comment 12 Dean Jackson 2010-03-03 11:00:06 PST
Comment on attachment 49857 [details]
added convenience methods for testing fill mode

cancelling review
Comment 13 Dean Jackson 2010-03-03 19:32:29 PST
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.
Comment 14 WebKit Review Bot 2010-03-03 19:38:21 PST
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.
Comment 15 Dean Jackson 2010-03-04 02:56:24 PST
Created attachment 50001 [details]
fill mode patch

minor build error
Comment 16 WebKit Review Bot 2010-03-04 03:01:48 PST
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.
Comment 17 Chris Marrin 2010-03-04 11:16:01 PST
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 @@
> +2010-03-03  Dean Jackson  <dino@apple.com>
> +
> +        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()?
Comment 18 Dean Jackson 2010-03-05 05:00:05 PST
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
Comment 20 Eric Seidel (no email) 2010-03-05 14:36:35 PST
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
Comment 21 Dean Jackson 2010-03-05 14:44:09 PST
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)
Comment 22 Dean Jackson 2010-03-05 14:54:57 PST
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.
Comment 23 Eric Seidel (no email) 2010-03-05 14:58:17 PST
Thank you for fixing.