WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(15)
View All
Add attachment
proposed patch, testcase, etc.
Antti Koivisto
Comment 1
2017-09-14 00:29:24 PDT
Created
attachment 320739
[details]
patch
Radar WebKit Bug Importer
Comment 2
2017-09-14 00:31:31 PDT
<
rdar://problem/34428035
>
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
Created
attachment 320753
[details]
patch
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
Created
attachment 320764
[details]
patch
Antti Koivisto
Comment 21
2017-09-14 05:41:57 PDT
Created
attachment 320765
[details]
patch
Antti Koivisto
Comment 22
2017-09-14 05:53:23 PDT
Created
attachment 320766
[details]
patch
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
Created
attachment 320778
[details]
patch
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
Created
attachment 320783
[details]
patch
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 32
2017-09-15 10:39:44 PDT
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
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
Another unflake attempt
https://trac.webkit.org/changeset/222162/webkit
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.
Top of Page
Format For Printing
XML
Clone This Bug