Bug 21001 - Starting transition after animation, when animation is finished, transition is wrong
Summary: Starting transition after animation, when animation is finished, transition i...
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:
Depends on:
Blocks:
 
Reported: 2008-09-22 11:06 PDT by Chris Marrin
Modified: 2008-09-29 14:21 PDT (History)
3 users (show)

See Also:


Attachments
Patch, including LayoutTest file (13.75 KB, patch)
2008-09-22 11:14 PDT, Chris Marrin
eric: review-
Details | Formatted Diff | Diff
New Patch incorporating comments from eseidel (16.28 KB, patch)
2008-09-25 15:51 PDT, Chris Marrin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2008-09-22 11:06:31 PDT
The test case has an animation and transition, both of which start on load. The transition is started by setting webKitTransform from JS after the animation starts. This should use the unanimated style (which is 0,0) as the start position. But it instead uses the starting animation position, which (0,100).

Patch on the way...
Comment 1 Chris Marrin 2008-09-22 11:11:56 PDT
The correct fromStyle (which is now correctly computed because of https://bugs.webkit.org/show_bug.cgi?id=20892) is now passed to the ctor of ImplicitAnimation and used rather than being passed in via reset(). 
Comment 2 Chris Marrin 2008-09-22 11:14:06 PDT
Created attachment 23658 [details]
Patch, including LayoutTest file
Comment 3 Eric Seidel (no email) 2008-09-25 01:18:14 PDT
Comment on attachment 23658 [details]
Patch, including LayoutTest file

style:
 86             const RenderStyle *fromStyle = kfAnim ? kfAnim->unanimatedStyle() : currentStyle;

Also, I personally prefer longer variable names than kfAnim.  Makes me think you're trying to use hungarian notation and making a float constant named anim. :)


Can't this just be a deleteAllValues(m_transitions) call?
5     for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it)
 276         delete it->second;

And this too:
deleteAllValues(finishedTransitions);

And again...
finishedAnimations

Seems m_fromStyle should maybe not be const...

Otherwise looks fine.

I think we should resolve the deleteAllValues issue before landing.
Comment 4 Chris Marrin 2008-09-25 14:49:34 PDT
I will make all the other changes. But I'd like to keep the RenderStyle a const just to show that I don't intend to change it. Of course that means I have to const_cast to ref/deref. But I think it's better to do that in isolation and maintain the overall safety of the object.
Comment 5 Chris Marrin 2008-09-25 15:40:07 PDT
You're right about changing that m_transitions case to use deleteAllValues, but the other two are just Vectors of int, which tell me which elements in the HashMaps to delete. So I think I need iterators there.

Comment 6 Chris Marrin 2008-09-25 15:51:14 PDT
Created attachment 23825 [details]
New Patch incorporating comments from eseidel
Comment 7 Eric Seidel (no email) 2008-09-25 16:25:34 PDT
Comment on attachment 23825 [details]
New Patch incorporating comments from eseidel

Looks sane enough.  Thanks!
Comment 8 Simon Fraser (smfr) 2008-09-29 14:21:50 PDT
Committed r37075
	M	WebCore/ChangeLog
	M	WebCore/page/animation/AnimationBase.h
	M	WebCore/page/animation/ImplicitAnimation.h
	M	WebCore/page/animation/CompositeAnimation.cpp
	M	WebCore/page/animation/ImplicitAnimation.cpp
	M	LayoutTests/ChangeLog
	A	LayoutTests/animations/transition-and-animation-2-expected.txt
	A	LayoutTests/animations/transition-and-animation-2.html
r37075 = d63ba9a851d9897f92c12499ccc7bdcdee8e9108 (trunk)