RESOLVED FIXED 171926
Computing animated style should not require renderers
https://bugs.webkit.org/show_bug.cgi?id=171926
Summary Computing animated style should not require renderers
Antti Koivisto
Reported 2017-05-10 09:08:23 PDT
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).
Attachments
patch (7.69 KB, patch)
2017-09-14 00:29 PDT, Antti Koivisto
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-elcapitan (1.37 MB, application/zip)
2017-09-14 01:13 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.74 MB, application/zip)
2017-09-14 01:18 PDT, Build Bot
no flags
Archive of layout-test-results from ews113 for mac-elcapitan (2.33 MB, application/zip)
2017-09-14 01:34 PDT, Build Bot
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.34 MB, application/zip)
2017-09-14 02:07 PDT, Build Bot
no flags
patch (12.86 KB, patch)
2017-09-14 03:07 PDT, Antti Koivisto
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan (1.40 MB, application/zip)
2017-09-14 04:27 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.52 MB, application/zip)
2017-09-14 04:35 PDT, Build Bot
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (2.14 MB, application/zip)
2017-09-14 04:44 PDT, Build Bot
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.40 MB, application/zip)
2017-09-14 04:58 PDT, Build Bot
no flags
patch (20.25 KB, patch)
2017-09-14 05:39 PDT, Antti Koivisto
no flags
patch (20.23 KB, patch)
2017-09-14 05:41 PDT, Antti Koivisto
no flags
patch (20.21 KB, patch)
2017-09-14 05:53 PDT, Antti Koivisto
sam: review+
patch (23.07 KB, patch)
2017-09-14 09:44 PDT, Antti Koivisto
no flags
patch (23.48 KB, patch)
2017-09-14 10:21 PDT, Antti Koivisto
no flags
patch (23.48 KB, patch)
2017-09-16 04:51 PDT, Antti Koivisto
no flags
Antti Koivisto
Comment 1 2017-09-14 00:29:24 PDT
Radar WebKit Bug Importer
Comment 2 2017-09-14 00:31:31 PDT
Build Bot
Comment 3 2017-09-14 01:13:37 PDT
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
Build Bot
Comment 4 2017-09-14 01:13:38 PDT
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
Build Bot
Comment 5 2017-09-14 01:18:52 PDT
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
Build Bot
Comment 6 2017-09-14 01:18:53 PDT
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
Build Bot
Comment 7 2017-09-14 01:34:17 PDT
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
Build Bot
Comment 8 2017-09-14 01:34:18 PDT
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
Build Bot
Comment 9 2017-09-14 02:07:26 PDT
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
Build Bot
Comment 10 2017-09-14 02:07:27 PDT
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
Antti Koivisto
Comment 11 2017-09-14 03:07:56 PDT
Build Bot
Comment 12 2017-09-14 04:27:33 PDT
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
Build Bot
Comment 13 2017-09-14 04:27:35 PDT
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
Build Bot
Comment 14 2017-09-14 04:35:39 PDT
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
Build Bot
Comment 15 2017-09-14 04:35:40 PDT
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
Build Bot
Comment 16 2017-09-14 04:44:47 PDT
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
Build Bot
Comment 17 2017-09-14 04:44:48 PDT
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
Build Bot
Comment 18 2017-09-14 04:58:09 PDT
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
Build Bot
Comment 19 2017-09-14 04:58:10 PDT
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
Antti Koivisto
Comment 20 2017-09-14 05:39:24 PDT
Antti Koivisto
Comment 21 2017-09-14 05:41:57 PDT
Antti Koivisto
Comment 22 2017-09-14 05:53:23 PDT
Simon Fraser (smfr)
Comment 23 2017-09-14 08:22:35 PDT
Are we going to be able to still support animations on pseudo elements after these changes?
Antti Koivisto
Comment 24 2017-09-14 08:24:13 PDT
(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.
Antti Koivisto
Comment 25 2017-09-14 08:29:58 PDT
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.
Sam Weinig
Comment 26 2017-09-14 09:02:54 PDT
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.
Antti Koivisto
Comment 27 2017-09-14 09:44:22 PDT
Antti Koivisto
Comment 28 2017-09-14 09:45:03 PDT
Good suggestions, I think I implemented them all.
Antti Koivisto
Comment 29 2017-09-14 10:21:13 PDT
WebKit Commit Bot
Comment 30 2017-09-14 12:18:29 PDT
Comment on attachment 320783 [details] patch Clearing flags on attachment: 320783 Committed r222040: <http://trac.webkit.org/changeset/222040>
WebKit Commit Bot
Comment 31 2017-09-14 12:18:31 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 33 2017-09-15 12:54:05 PDT
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>
Antti Koivisto
Comment 34 2017-09-16 04:51:17 PDT
Created attachment 321003 [details] patch Tried to unflake the test.
WebKit Commit Bot
Comment 35 2017-09-16 05:33:08 PDT
Comment on attachment 321003 [details] patch Clearing flags on attachment: 321003 Committed r222129: <http://trac.webkit.org/changeset/222129>
WebKit Commit Bot
Comment 36 2017-09-16 05:33:10 PDT
All reviewed patches have been landed. Closing bug.
Antti Koivisto
Comment 37 2017-09-18 10:54:56 PDT
Ryan Haddad
Comment 38 2017-09-18 13:16:13 PDT
(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
Antti Koivisto
Comment 39 2017-09-18 14:02:18 PDT
Another WK1 unflake attempt https://trac.webkit.org/r222178
Note You need to log in before you can comment on or make changes to this bug.