Bug 21001 – Starting transition after animation, when animation is finished, transition is wrong

• Component: CSS
• Status: RESOLVED FIXED

[Chris Marrin]

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...

[Chris Marrin]

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(). 

[Chris Marrin]

Created attachment 23658 [details]
Patch, including LayoutTest file

[Eric Seidel (no email)]

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.

[Chris Marrin]

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.

[Chris Marrin]

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.

[Chris Marrin]

Created attachment 23825 [details]
New Patch incorporating comments from eseidel

[Eric Seidel (no email)]

Comment on attachment 23825 [details]
New Patch incorporating comments from eseidel

Looks sane enough.  Thanks!

[Simon Fraser (smfr)]

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)