Bug 171926 - Computing animated style should not require renderers
Summary: Computing animated style should not require renderers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 176807 176832
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-10 09:08 PDT by Antti Koivisto
Modified: 2017-09-18 14:02 PDT (History)
9 users (show)

See Also:


Attachments
patch (7.69 KB, patch)
2017-09-14 00:29 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
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 Details
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 Details
patch (12.86 KB, patch)
2017-09-14 03:07 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
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 Details
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 Details
patch (20.25 KB, patch)
2017-09-14 05:39 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (20.23 KB, patch)
2017-09-14 05:41 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (20.21 KB, patch)
2017-09-14 05:53 PDT, Antti Koivisto
sam: review+
Details | Formatted Diff | Diff
patch (23.07 KB, patch)
2017-09-14 09:44 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (23.48 KB, patch)
2017-09-14 10:21 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (23.48 KB, patch)
2017-09-16 04:51 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 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).
Comment 1 Antti Koivisto 2017-09-14 00:29:24 PDT
Created attachment 320739 [details]
patch
Comment 2 Radar WebKit Bug Importer 2017-09-14 00:31:31 PDT
<rdar://problem/34428035>
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Antti Koivisto 2017-09-14 03:07:56 PDT
Created attachment 320753 [details]
patch
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Build Bot 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
Comment 20 Antti Koivisto 2017-09-14 05:39:24 PDT
Created attachment 320764 [details]
patch
Comment 21 Antti Koivisto 2017-09-14 05:41:57 PDT
Created attachment 320765 [details]
patch
Comment 22 Antti Koivisto 2017-09-14 05:53:23 PDT
Created attachment 320766 [details]
patch
Comment 23 Simon Fraser (smfr) 2017-09-14 08:22:35 PDT
Are we going to be able to still support animations on pseudo elements after these changes?
Comment 24 Antti Koivisto 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.
Comment 25 Antti Koivisto 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.
Comment 26 Sam Weinig 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.
Comment 27 Antti Koivisto 2017-09-14 09:44:22 PDT
Created attachment 320778 [details]
patch
Comment 28 Antti Koivisto 2017-09-14 09:45:03 PDT
Good suggestions, I think I implemented them all.
Comment 29 Antti Koivisto 2017-09-14 10:21:13 PDT
Created attachment 320783 [details]
patch
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2017-09-14 12:18:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Ryan Haddad 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>
Comment 34 Antti Koivisto 2017-09-16 04:51:17 PDT
Created attachment 321003 [details]
patch

Tried to unflake the test.
Comment 35 WebKit Commit Bot 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>
Comment 36 WebKit Commit Bot 2017-09-16 05:33:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Antti Koivisto 2017-09-18 10:54:56 PDT
Another unflake attempt https://trac.webkit.org/changeset/222162/webkit
Comment 38 Ryan Haddad 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
Comment 39 Antti Koivisto 2017-09-18 14:02:18 PDT
Another WK1 unflake attempt https://trac.webkit.org/r222178