| Summary: | Transform misplaces element 50% of the time | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||
| Component: | New Bugs | Assignee: | Dean Jackson <dino> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, buildbot, dstockwell, simon.fraser | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=172397 | ||||||||
| Attachments: |
|
||||||||
|
Description
Dean Jackson
2017-05-18 12:04:44 PDT
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> |