Transform misplaces element 50% of the time
Created attachment 310529 [details] Patch
Comment on attachment 310529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review > Source/WebCore/ChangeLog:11 > + animation. The "50% of the time" comes in to play because I wonder if this fixes any existing flaky tests! > LayoutTests/animations/needs-layout.html:50 > + setTimeout(test, 100); Why is the 100 ms timeout needed? Such short timeouts tend to make flaky tests.
Comment on attachment 310529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review > Source/WebCore/page/animation/CSSAnimationController.cpp:662 > + frameView.setNeedsLayout(); Why isn't this renderer.setNeedsLayout()? > Source/WebCore/page/animation/CSSAnimationController.cpp:663 > + frameView.scheduleRelayout(); Is this needed as well as setNeedsLayout()? > Source/WebCore/page/animation/KeyframeAnimation.cpp:84 > + if (!m_keyframes.containsProperty(CSSPropertyTransform)) Is this a linear walk just like your loop below? > Source/WebCore/page/animation/KeyframeAnimation.cpp:99 > + if (translation->x().isPercent() || translation->y().isPercent() || translation->z().isPercent()) { translation->z() cannot be a percentage.
Comment on attachment 310529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review >> Source/WebCore/page/animation/CSSAnimationController.cpp:662 >> + frameView.setNeedsLayout(); > > Why isn't this renderer.setNeedsLayout()? Fixed. >> Source/WebCore/page/animation/KeyframeAnimation.cpp:84 >> + if (!m_keyframes.containsProperty(CSSPropertyTransform)) > > Is this a linear walk just like your loop below? Nope. This is a hash set lookup. >> Source/WebCore/page/animation/KeyframeAnimation.cpp:99 >> + if (translation->x().isPercent() || translation->y().isPercent() || translation->z().isPercent()) { > > translation->z() cannot be a percentage. Ah, I forgot. It's still a length, but not percentage. >> LayoutTests/animations/needs-layout.html:50 >> + setTimeout(test, 100); > > Why is the 100 ms timeout needed? Such short timeouts tend to make flaky tests. It's not. I've reduced it to nothing. I just need a timeout.
Comment on attachment 310529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review >>> Source/WebCore/page/animation/CSSAnimationController.cpp:662 >>> + frameView.setNeedsLayout(); >> >> Why isn't this renderer.setNeedsLayout()? > > Fixed. Actually, it has to be the frameView, otherwise the scheduleLayout() on the frameView returns early without actually scheduling a layout.
Comment on attachment 310529 [details] Patch smfr_: it’s just about the order of the 2 timers smfr_: i think there’s another fix smfr_: if the animation timer fires and there’s a layout pending, do a forced layout
Created attachment 310569 [details] Patch
Committed r217075: <http://trac.webkit.org/changeset/217075>