Bug 172300

Summary: Transform misplaces element 50% of the time
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch simon.fraser: review+

Dean Jackson
Reported 2017-05-18 12:04:44 PDT
Transform misplaces element 50% of the time
Attachments
Patch (10.78 KB, patch)
2017-05-18 12:22 PDT, Dean Jackson 		no flags
DetailsFormatted DiffDiff
Patch (12.82 KB, patch)
2017-05-18 16:07 PDT, Dean Jackson 		simon.fraser: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dean Jackson
Comment 1 2017-05-18 12:22:57 PDT
Created attachment 310529 [details] Patch
Alexey Proskuryakov
Comment 2 2017-05-18 12:33:49 PDT
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.
Simon Fraser (smfr)
Comment 3 2017-05-18 13:18:56 PDT
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.
Dean Jackson
Comment 4 2017-05-18 13:29:00 PDT
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.
Dean Jackson
Comment 5 2017-05-18 13:36:38 PDT
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.
Simon Fraser (smfr)
Comment 6 2017-05-18 13:52:40 PDT
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
Dean Jackson
Comment 7 2017-05-18 16:07:54 PDT
Created attachment 310569 [details] Patch
Dean Jackson
Comment 8 2017-05-18 16:22:44 PDT
Committed r217075: <http://trac.webkit.org/changeset/217075>
Note You need to log in before you can comment on or make changes to this bug.