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.
Is this still an issue? Is it reproducible in winlauncher?
This is obviously still reproducible. Why ask when there is a perfectly good test case attached?
Created attachment 100171 [details] The test case for both safari and firefox
Created attachment 100177 [details] TestCase for both safari and Firefox 5.0
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.
Why would zooming change the duration?
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??
Created attachment 101740 [details] Code changes
I think someone should tell AnimationController when the zoom changes, then it knows that the animations need to be rejiggered.
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.
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
<rdar://problem/9821291>
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.
Created attachment 110928 [details] This contain the patch and the layout test for the issue .
Created attachment 110929 [details] This contain the patch and the layout test for the issue .
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+
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.
Created attachment 111098 [details] In-corporated review comments
Created attachment 115464 [details] Updated the patch Made some changes to the ChangeLog file and the Layout Test
Created attachment 115469 [details] Updated patch
Dean/Simon, any further comment on the updated patch?
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.