Bug 201048 - Accelerated animations freeze on render tree rebuild
Summary: Accelerated animations freeze on render tree rebuild
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari 12
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 204805 (view as bug list)
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2019-08-22 12:47 PDT by Liam DeBeasi
Modified: 2020-02-21 00:05 PST (History)
17 users (show)

See Also:


Attachments
Sample app shown on iOS 12 (180.98 KB, video/quicktime)
2019-12-19 01:22 PST, Niklas Merz
no flags Details
Sample app on iOS 13 (2.50 MB, video/quicktime)
2019-12-19 01:32 PST, Niklas Merz
no flags Details
patch (9.23 KB, patch)
2020-02-03 08:25 PST, Antti Koivisto
graouts: review+
Details | Formatted Diff | Diff
patch (9.45 KB, patch)
2020-02-03 09:12 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (9.46 KB, patch)
2020-02-03 09:34 PST, 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 Liam DeBeasi 2019-08-22 12:47:00 PDT
When animating an element within a shadow root in a custom element, layout invalidations via `Node.insertBefore` cause the animation to freeze. This does not happen when using a custom element without a shadow root. I am able to reproduce this in Safari 12 on both iOS and macOS.

Code reproduction: https://codepen.io/liamdebeasi/pen/YzKNGzW

Steps to reproduce:
1. Go to the link above.
2. You should see a box that is rotating, as well as a button.
3. Click the "Click Me" button.
4. Notice that the box stops rotating.
5. Subsequent clicks of the "Click Me" button will cause the animation to render for a frame and then stop again.

Hovering over the ".square" element in Web Inspector will reveal that the animation is still playing. The "highlight" that appears over the element in the DOM tree should update every time you hover over it. Switching to another tab and then back to the original tab somehow fixes the issue.

This bug does not happen when using a custom element without a shadow root, as seen in this code example: https://codepen.io/liamdebeasi/pen/xxKgErw (follow the same steps as before)

As an additional reference, Angular can sometimes call `Node.insertBefore` when running change detection. This can cause animations in a user's application to break when using the shadow DOM. See https://github.com/ionic-team/ionic/issues/17202 for more info)


Web Animations API Note:

The same bug affects Safari's implementation of the Web Animations API (tested using Safari Technology Preview 90). With just "Web Animations" enabled in "Experiments", the tab switching trick mentioned above does not work. When also enabling "CSS Animations via Web Animations" the tab switching trick works.

Web Animations Code Reproduction: https://codepen.io/liamdebeasi/pen/LYPxbed
Comment 1 Radar WebKit Bug Importer 2019-08-22 13:47:37 PDT
<rdar://problem/54612621>
Comment 2 Niklas Merz 2019-11-15 06:32:12 PST
I can add that this seems to be a regression. The same transitions work more smoothly on iOS 12.4 than on iOS 13.
Comment 3 Niklas Merz 2019-12-19 01:22:32 PST
Created attachment 386084 [details]
Sample app shown on iOS 12
Comment 4 Niklas Merz 2019-12-19 01:32:03 PST
Created attachment 386085 [details]
Sample app on iOS 13

I just built a small sample with Ionic that shows why this bug is really bad for the UX. The same app runs on iOS 12 better and the animation is less stuttering. 

That's why I think this might be a regression.
Comment 5 Niklas Merz 2020-01-27 01:58:37 PST
Sorry for the ping. This is really a huge UX flaw for page transitions in Ionic apps with certain pages.

Can we do anything to get this fixed? Thanks
Comment 6 Ryosuke Niwa 2020-01-28 23:23:06 PST
(In reply to Niklas Merz from comment #5)
> Sorry for the ping. This is really a huge UX flaw for page transitions in
> Ionic apps with certain pages.

I appreciate that you provided this detail. It allows us to priorities this bug better.

> Can we do anything to get this fixed?

It would be ideal if we had a minimum reproduction that doesn't rely on custom elements instead of https://codepen.io/liamdebeasi/pen/YzKNGzW but it's not a big deal. I can easily make a reduction myself.

FWIW, I don't think I can work on it anytime soon due to other commitments. Having said that, it's probably not that hard to diagnose this issue even as the first WebKit bug for someone so if you can find someone who can work on this that might work too.
Comment 7 Liam DeBeasi 2020-01-29 06:06:53 PST
(In reply to Ryosuke Niwa from comment #6)
> It would be ideal if we had a minimum reproduction that doesn't rely on
> custom elements instead of https://codepen.io/liamdebeasi/pen/YzKNGzW but
> it's not a big deal. I can easily make a reduction myself.
> 
> FWIW, I don't think I can work on it anytime soon due to other commitments.
> Having said that, it's probably not that hard to diagnose this issue even as
> the first WebKit bug for someone so if you can find someone who can work on
> this that might work too.

I can try and diagnose it a bit, though I am not very familiar with the WebKit code base.

What kind of reproduction would be helpful here? The issue only happens with custom elements/Shadow DOM, so I am not sure that a reproduction without custom elements is possible here.
Comment 8 Tim Guan-tin Chien [:timdream] 2020-02-02 19:22:38 PST
Maybe the test case in bug 204805 can be helpful.
Comment 9 Antti Koivisto 2020-02-03 06:55:06 PST
*** Bug 204805 has been marked as a duplicate of this bug. ***
Comment 10 Antti Koivisto 2020-02-03 06:55:56 PST
204805 has working WIP patch and test case without shadow trees.
Comment 11 Antti Koivisto 2020-02-03 08:25:01 PST
Created attachment 389514 [details]
patch
Comment 12 Antoine Quint 2020-02-03 08:30:34 PST
Comment on attachment 389514 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389514&action=review

> LayoutTests/webanimations/accelerated-animation-renderer-change.html:48
> +    await new Promise(resolve => setTimeout(resolve, 50));

It would be preferable to get a reference to the animation using element.getAnimations() and then wait for its ready promise to resolve and then wait one frame using requestAnimationFrame(), that way there is no setTimeout() involved.

> LayoutTests/webanimations/accelerated-animation-renderer-change.html:54
> +    await new Promise(resolve => setTimeout(resolve, 500));

Ideally we could use requestAnimationFrame here too, possibly with 2 frames to guarantee the animation would have ticked a couple of times.
Comment 13 Antti Koivisto 2020-02-03 09:12:50 PST
Created attachment 389525 [details]
patch
Comment 14 Antti Koivisto 2020-02-03 09:34:29 PST
Created attachment 389527 [details]
patch
Comment 15 WebKit Commit Bot 2020-02-03 21:56:09 PST
Comment on attachment 389527 [details]
patch

Clearing flags on attachment: 389527

Committed r255663: <https://trac.webkit.org/changeset/255663>
Comment 16 WebKit Commit Bot 2020-02-03 21:56:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Antti Koivisto 2020-02-06 15:00:52 PST
Adding slot based test in https://bugs.webkit.org/show_bug.cgi?id=207359
Comment 18 Niklas Merz 2020-02-20 23:16:44 PST
I just did a quick test with Safari Technology Preview on macOS and the latest iOS 13.4 Developer Beta 2. Looks like the issue is fixed and the patch has landed in the beta versions!

The codepen works now and the page transitions in our app are much smoother!

Thank you very much for fixing this.
Comment 19 Ryosuke Niwa 2020-02-21 00:05:13 PST
(In reply to Niklas Merz from comment #18)
> I just did a quick test with Safari Technology Preview on macOS and the
> latest iOS 13.4 Developer Beta 2. Looks like the issue is fixed and the
> patch has landed in the beta versions!
> 
> The codepen works now and the page transitions in our app are much smoother!

Thanks for verifying that the bug is fixed!