Bug 26943 - Transitions restart sometimes when they shouldn't
Summary: Transitions restart sometimes when they shouldn't
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
: 26689 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-07-02 16:38 PDT by Chris Marrin
Modified: 2009-07-05 18:47 PDT (History)
1 user (show)

See Also:


Attachments
test case (2.63 KB, text/html)
2009-07-02 16:40 PDT, Chris Marrin
no flags Details
image used by test case (1.31 MB, image/jpeg)
2009-07-02 16:41 PDT, Chris Marrin
no flags Details
Patch with test (1.79 MB, patch)
2009-07-02 16:47 PDT, Chris Marrin
simon.fraser: review-
Details | Formatted Diff | Diff
Testcase that the patch breaks (1.90 KB, text/html)
2009-07-03 14:28 PDT, Simon Fraser (smfr)
no flags Details
Testcase broken by the revised patch (1.43 KB, text/html)
2009-07-05 11:07 PDT, Simon Fraser (smfr)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2009-07-02 16:38:21 PDT
This happens when they are two transitions, one of which has a slightly longer (50ms) duration than the other. The finish of the first one causes the second to run a second time.

I'm attaching a test case showing this.
Comment 1 Chris Marrin 2009-07-02 16:40:01 PDT
Created attachment 32208 [details]
test case
Comment 2 Chris Marrin 2009-07-02 16:41:10 PDT
Created attachment 32209 [details]
image used by test case

This image needs to be put into a resources subdirectory to be used with the test case
Comment 3 Chris Marrin 2009-07-02 16:47:04 PDT
Created attachment 32210 [details]
Patch with test

NOTE: Please don't review this patch until we have done some tests with more complex content.
Comment 4 Simon Fraser (smfr) 2009-07-03 14:13:24 PDT
Comment on attachment 32210 [details]
Patch with test

This breaks a testcase that I will attach.
Comment 5 Simon Fraser (smfr) 2009-07-03 14:28:13 PDT
*** Bug 26689 has been marked as a duplicate of this bug. ***
Comment 6 Simon Fraser (smfr) 2009-07-03 14:28:30 PDT
Created attachment 32248 [details]
Testcase that the patch breaks
Comment 7 Chris Marrin 2009-07-04 13:24:19 PDT
The current patch file is in error. In CompositeAnimation.cpp, at line 200 there is a for loop:

        for (AnimationNameMap::const_iterator it = m_keyframeAnimations.begin(); it != kfend; ++it)

But the line that should be after it is missing. It should say:

        for (AnimationNameMap::const_iterator it = m_keyframeAnimations.begin(); it != kfend; ++it)
            it->second->setIndex(-1);

If you add that line, Simon's new test case works fine.
Comment 8 Simon Fraser (smfr) 2009-07-05 11:07:26 PDT
Created attachment 32275 [details]
Testcase broken by the revised patch
Comment 9 Simon Fraser (smfr) 2009-07-05 11:08:01 PDT
With the fixed-up patch, I see an assertion and test failing in the test I just attached. When you unhover the lower box, it doesn't start animating again.
Comment 10 Simon Fraser (smfr) 2009-07-05 12:19:50 PDT
Fixed patch reviewed and committed:
http://trac.webkit.org/changeset/45554
Comment 11 Simon Fraser (smfr) 2009-07-05 12:41:00 PDT
Bad news. I had to revert this because it broke LayoutTests/animations/transition-and-animation-1.html.
Comment 12 Chris Marrin 2009-07-05 17:13:14 PDT
I have a fix for the transition-and-animation-1.html bug. The problem is that, if a single element has both a transition and a keyframe animation, the keyframe animation gets removed from the list before the transitions are updated. So when a keyframe animation ends, the transition gets updated and it looks like the style is different (because it is putting in the unanimated style from the old animation) without there being a keyframe animation. Updating transitions before animations fixes the problem. This makes sense since animations can override transitions, so running transitions first allows them to see if there are any animations overriding them.

I will submit a new patch soon...
Comment 13 Chris Marrin 2009-07-05 17:18:55 PDT
Actually, I won't supply a new patch, since Simon as made an additional change. The fix is in CompositeAnimation.cpp. Take the updateKeyframeAnimations() method at line 263 and move it after the updateTransitions() method at line 266 (along with the comments, of course. If needed I can create a new patch with all my and Simon's fixes.
Comment 14 Simon Fraser (smfr) 2009-07-05 18:47:13 PDT
http://trac.webkit.org/changeset/45556