Bug 29993 – Zooming while a CSS animation is running does not recompute endpoints

• Status: NEW
• Component: CSS

[Jeffrey Rosen]

Created attachment 40492 [details]
test case of the mentioned problem

Values in webkit animations are independent of zooming in or out of the page. See attached test case.

[Mustafizur Rahaman (rahaman)]

Is this still an issue? Is it reproducible in winlauncher?

[Alexey Proskuryakov]

This is obviously still reproducible. Why ask when there is a perfectly good test case attached?

[Sneha Bhat]

Created attachment 100171 [details]
The test case for both safari and firefox

[Sneha Bhat]

Created attachment 100177 [details]
TestCase for both safari and Firefox 5.0

[Sneha Bhat]

It seems like the value of animation-duration is not effected by the zoom factor.
The keyframe "to" value calculated in function Length blend(const Length& from, float progress) in Length.h is not effected by the Zoom factor.

Some observations:
1.This works well with Firefox 5.0
2.If your current zoom factor is >1.0 and the if you try to load the html page(either of the two attachments),  the "to" value in blend function will be multiplied by zoom factor.

[Simon Fraser (smfr)]

Why would zooming change the duration?

[Sneha Bhat]

Here is a fix for the issue.

The KeyframeList(m_keyframe) is created in the in the constructor of KeyframeAnimation class and is never updated even if the  the zoom factor changes.

So if we update the m_keyframe in KeyFrameAnimation.cpp, then the animations works fine.
I have done some changes in KeyfarmeAnimation.cpp. With these changes m_keyframes is updated every time.But this may be an over head. So we have to update it only if the zoom factor changes.

This may not be a apropriate fix, but with the changes in keyframeAnimation.cpp and compositeAnimation.cpp, I was able to get the desired output.

I have attached the code differences along.

Any suggestions on this??

[Sneha Bhat]

Created attachment 101740 [details]
Code changes

[Simon Fraser (smfr)]

I think someone should tell AnimationController when the zoom changes, then it knows that the animations need to be rejiggered.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/page/animation/CompositeAni..." exit_code: 1

Source/WebCore/page/animation/CompositeAnimation.cpp:221:  Extra space before )  [whitespace/parens] [2]
Total errors found: 1 in 3 files


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

[WebKit Review Bot]

Comment on attachment 101740 [details]
Code changes

Attachment 101740 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9228407

New failing tests:
animations/suspend-resume-animation-events.html
animations/fill-mode.html
animations/fill-mode-transform.html
animations/animation-events-create.html
animations/transition-and-animation-1.html
animations/transition-and-animation-3.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-from-to.html
animations/transition-and-animation-2.html
animations/fill-mode-multiple-keyframes.html
animations/animation-shorthand-removed.html
fast/css/getComputedStyle/getComputedStyle-zIndex-auto.html

[Simon Fraser (smfr)]

<rdar://problem/9821291>

[Sneha Bhat]

Created attachment 104580 [details]
Change Keyframe only when Zoom factor changes

In the previous code changes, we were updating the m_keyframes every single time.But this is not the right way, so now we only update the Keyframes if there is a change in Zoom factor.

[Sneha Bhat]

Created attachment 110928 [details]
This contain the patch and the layout test for the issue .

[Sneha Bhat]

Created attachment 110929 [details]
This contain the patch and the layout test for the issue .

[Dean Jackson]

Comment on attachment 110929 [details]
This contain the patch and the layout test for the issue .

I think you meant to add review? not review+

[Dean Jackson]

Comment on attachment 110929 [details]
This contain the patch and the layout test for the issue .

View in context: https://bugs.webkit.org/attachment.cgi?id=110929&action=review

> LayoutTests/ChangeLog:3
> +        The animation is not affected with change in zoom factor

Why not use the bug title here? "Zooming while a CSS animation is running does not recompute endpoints"

> LayoutTests/animations/animation-Zoom-test.html:52
> +      box.addEventListener( 'webkitAnimationEnd', function() {

space between ( and '

> LayoutTests/animations/animation-Zoom-test.html:53
> +        var boxelement = document.getElementById("box");

Above you use boxElement (camel case). You should probably do that through this entire test.

> LayoutTests/animations/animation-Zoom-test.html:55
> +        var y1 = webkitConvertPointFromNodeToPage(boxelement, new WebKitPoint(0,0)).y;

You never use y1 again. And why not just have the var be a point then use .x and .y later?

> LayoutTests/animations/animation-Zoom-test.html:57
> +        var width = (window.getComputedStyle(containerelement,null).getPropertyValue("width") ) ;

You can remove the parentheses. And add a space after the comma.

> LayoutTests/animations/animation-Zoom-test.html:62
> +          resultString += "PASS - " + "The Animation Length resizes with the new zoom factor applied."  ;

4 spaces for indentation (this applies all through the file)

> LayoutTests/animations/animation-Zoom-test.html:64
> +          resultString += "FAIL - Expected to animate till  "  +out + "px" + " but stopped at "+ width + "" ;

Weird spacing.

> Source/WebCore/ChangeLog:3
> +        The animation is not affected with change in zoom factor

Again, might as well use the bug title.

> Source/WebCore/ChangeLog:8
> +        The KeyframeList(m_keyframe) is created in the in the constructor of KeyframeAnimation 

in the in the

> Source/WebCore/ChangeLog:9
> +        class and is never updated even if the  the zoom factor changes.

the space space the

> Source/WebCore/ChangeLog:12
> +        calculate and update m_keyframe.

This isn't quite right. We never touch m_keyframe, we edit m_keyframeAnimations.

> Source/WebCore/page/animation/CompositeAnimation.cpp:213
> +        } else {

I haven't followed this through completely, but don't you still want to run the above step even if the zoom has changed, after you do the code below? Otherwise a completed (postActive) animation will use an extra step.

> Source/WebCore/page/animation/CompositeAnimation.cpp:223
> +                keyframeAnim->keyFrameAnimationStyle(renderer, targetStyle);

Inconsistent case here: keyframeAnim and keyFrameAnimationStyle. The method should use keyframe (lower case f)

> Source/WebCore/page/animation/CompositeAnimation.cpp:227
> +                    m_keyframeAnimationOrderMap.append(keyframeAnim->name().impl());

Don't you already have the name as a local variable: animationName? (same above)

> Source/WebCore/page/animation/KeyframeAnimation.h:57
> +    void keyFrameAnimationStyle(RenderObject* renderer, RenderStyle* unanimatedStyle);

keyframe not keyFrame

Also, I think the name is misleading. This method doesn't return the keyframeAnimationStyle.

[Sneha Bhat]

Created attachment 111098 [details]
In-corporated review comments

[Sneha Bhat]

Created attachment 115464 [details]
Updated the patch

Made some changes to the ChangeLog file and the Layout Test

[Sneha Bhat]

Created attachment 115469 [details]
Updated patch

[Ariya Hidayat]

Dean/Simon, any further comment on the updated patch?

[Simon Fraser (smfr)]

Comment on attachment 115469 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115469&action=review

> Source/WebCore/page/animation/CompositeAnimation.cpp:209
> +            int numAnims = targetStyle->animations()->size();

Shouldn't this be a size_t?

> Source/WebCore/page/animation/CompositeAnimation.cpp:212
> +                AtomicString animationName(anim->name());

Why make the AtomicString if you're going to continue two lines below?

> Source/WebCore/page/animation/CompositeAnimation.cpp:217
> +                RefPtr<KeyframeAnimation> keyframeAnim = m_keyframeAnimations.get(animationName.impl());

Does this actually need an AtomicString?

> Source/WebCore/page/animation/CompositeAnimation.cpp:219
> +                m_keyframeAnimations.set(animationName.impl(), keyframeAnim);

Why does it need to be put back in the hash map?

> Source/WebCore/page/animation/KeyframeAnimation.h:57
> +    void updateKeyframes(RenderObject* renderer, RenderStyle* unanimatedStyle);

'renderer' can be removed.