Our animation code is currently tied to renderers. This means we can't compute animated style correctly in the initial style resolution before we construct them. This causes various problems and requires workarounds (like bug 171883).
Created attachment 320739 [details] patch
<rdar://problem/34428035>
Comment on attachment 320739 [details] patch Attachment 320739 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4543104 New failing tests: transitions/transition-display-property.html
Created attachment 320746 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 320739 [details] patch Attachment 320739 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4543110 New failing tests: transitions/transition-display-property.html
Created attachment 320747 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 320739 [details] patch Attachment 320739 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4543197 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/default-src-object-data-url-blocked.html transitions/transition-display-property.html imported/blink/fast/animation/animation-without-parent-crash.html fast/shadow-dom/slot-renderer-teardown.html
Created attachment 320748 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 320739 [details] patch Attachment 320739 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4543494 New failing tests: transitions/transition-display-property.html
Created attachment 320750 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 320753 [details] patch
Comment on attachment 320753 [details] patch Attachment 320753 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4544454 New failing tests: transitions/transition-display-property.html imported/blink/animations/display-none-cancels-nested-animations.html
Created attachment 320758 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 320753 [details] patch Attachment 320753 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4544463 New failing tests: transitions/transition-display-property.html imported/blink/animations/display-none-cancels-nested-animations.html
Created attachment 320759 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 320753 [details] patch Attachment 320753 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4544470 New failing tests: transitions/transition-display-property.html imported/blink/animations/display-none-cancels-nested-animations.html
Created attachment 320760 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 320753 [details] patch Attachment 320753 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4544486 New failing tests: transitions/transition-display-property.html imported/blink/animations/display-none-cancels-nested-animations.html
Created attachment 320761 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 320764 [details] patch
Created attachment 320765 [details] patch
Created attachment 320766 [details] patch
Are we going to be able to still support animations on pseudo elements after these changes?
(In reply to Simon Fraser (smfr) from comment #23) > Are we going to be able to still support animations on pseudo elements after > these changes? Sure.
We currently create actual Element instances (PseudoElement) for pseudos which is silly. If we get around cleaning that up we can then just use (Element, pseudoid) pair s in the animation system.
Comment on attachment 320766 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=320766&action=review > Source/WebCore/page/animation/CompositeAnimation.h:57 > + std::unique_ptr<RenderStyle> animate(Element&, const RenderStyle* currentStyle, const RenderStyle& targetStyle, bool& animationStateChanged); Perhaps this could use a comment explaining what meaning of the style it returns is (is it still fair to call it the 'blended style'? Better yet, as I note below, return a struct with a RenderStyle and animationStateChanged bit might make it more clear. > Source/WebCore/style/StyleTreeResolver.cpp:251 > + std::unique_ptr<RenderStyle> animatedStyle; > > - auto makeUpdate = [&] (std::unique_ptr<RenderStyle> style, Change change) { > - if (validity >= Validity::SubtreeInvalid) > - change = std::max(change, validity == Validity::SubtreeAndRenderersInvalid ? Detach : Force); > - if (parentChange >= Force) > - change = std::max(change, parentChange); > - return ElementUpdate { WTFMove(style), change, recompositeLayer }; > - }; > + auto& animationController = element.document().frame()->animation(); > > - auto* renderer = element.renderer(); > + animatedStyle = animationController.updateAnimations(element, *newStyle, oldStyle, recompositeLayer); There doesn't seem to be a good reason to declare std::unique_ptr<RenderStyle> animatedStyle; up front. Could be done right when calling animationController.updateAnimations and use auto. It also seems a bit unclear that recompositeLayer can change here. I think this would be a bit more straightforward to read if updateAnimations returned a struct with animatedStyle and the animationStateChanged bit. > Source/WebCore/style/StyleTreeResolver.cpp:258 > + auto change = Detach; > + if (oldStyle) > + change = determineChange(*oldStyle, *newStyle); Would this be clearer as: auto change = oldStyle ? determineChange(*oldStyle, *newStyle) : Detach; I dunno. > Source/WebCore/style/StyleTreeResolver.cpp:266 > + return ElementUpdate { WTFMove(newStyle), change, recompositeLayer }; I think you can remove the ElementUpdate identifier here. It should be implicit.
Created attachment 320778 [details] patch
Good suggestions, I think I implemented them all.
Created attachment 320783 [details] patch
Comment on attachment 320783 [details] patch Clearing flags on attachment: 320783 Committed r222040: <http://trac.webkit.org/changeset/222040>
All reviewed patches have been landed. Closing bug.
The LayoutTest added with this change is a flaky image failure on Sierra/El Capitan Debug WK1: https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r222080%20(3617)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=transitions%2Ftransition-display-property-2.html
Reverted r222040 for reason: The LayoutTest added with this change is a flaky image failure on mac-wk1 debug bots. Committed r222104: <http://trac.webkit.org/changeset/222104>
Created attachment 321003 [details] patch Tried to unflake the test.
Comment on attachment 321003 [details] patch Clearing flags on attachment: 321003 Committed r222129: <http://trac.webkit.org/changeset/222129>
Another unflake attempt https://trac.webkit.org/changeset/222162/webkit
(In reply to Antti Koivisto from comment #37) > Another unflake attempt https://trac.webkit.org/changeset/222162/webkit Just saw a failure with r222168 on Sierra Release WK1: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/4651
Another WK1 unflake attempt https://trac.webkit.org/r222178