Bug 171926 – Computing animated style should not require renderers

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antti Koivisto]

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).

[Antti Koivisto]

Created attachment 320739 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/34428035>

[Build Bot]

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]

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 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]

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 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]

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 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]

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]

Created attachment 320753 [details]
patch

[Build Bot]

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]

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 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]

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 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]

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 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]

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]

Created attachment 320764 [details]
patch

[Antti Koivisto]

Created attachment 320765 [details]
patch

[Antti Koivisto]

Created attachment 320766 [details]
patch

[Simon Fraser (smfr)]

Are we going to be able to still support animations on pseudo elements after these changes?

[Antti Koivisto]

(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]

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 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]

Created attachment 320778 [details]
patch

[Antti Koivisto]

Good suggestions, I think I implemented them all.

[Antti Koivisto]

Created attachment 320783 [details]
patch

[WebKit Commit Bot]

Comment on attachment 320783 [details]
patch

Clearing flags on attachment: 320783

Committed r222040: <http://trac.webkit.org/changeset/222040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Ryan Haddad]

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]

Created attachment 321003 [details]
patch

Tried to unflake the test.

[WebKit Commit Bot]

Comment on attachment 321003 [details]
patch

Clearing flags on attachment: 321003

Committed r222129: <http://trac.webkit.org/changeset/222129>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Another unflake attempt https://trac.webkit.org/changeset/222162/webkit

[Ryan Haddad]

(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]

Another WK1 unflake attempt https://trac.webkit.org/r222178