Bug 212213 - REGRESSION: Delayed updating of the parallax images on pacificvoyages.net/posts
Summary: REGRESSION: Delayed updating of the parallax images on pacificvoyages.net/posts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL: https://www.pacificvoyages.net/posts/
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-21 08:17 PDT by Simon Fraser (smfr)
Modified: 2020-06-24 10:58 PDT (History)
5 users (show)

See Also:


Attachments
Reduction (1.19 KB, text/html)
2020-05-28 03:07 PDT, Antoine Quint
no flags Details
Reduction (1.58 KB, text/html)
2020-05-28 03:21 PDT, Antoine Quint
no flags Details
Patch for EWS (8.88 KB, patch)
2020-06-04 09:37 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for EWS (8.81 KB, patch)
2020-06-17 14:03 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (19.22 KB, patch)
2020-06-24 09:57 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-05-21 08:17:45 PDT
When scrolling https://www.pacificvoyages.net/posts/ the images are updated after a delay in Safari. In Chrome and Firefox, they move more smoothly.
Comment 1 Radar WebKit Bug Importer 2020-05-21 10:00:21 PDT
<rdar://problem/63497946>
Comment 2 Simon Fraser (smfr) 2020-05-21 10:06:20 PDT
Page seems to be using http://www.jarallax.com/ and https://greensock.com/
Comment 3 Simon Fraser (smfr) 2020-05-24 10:31:45 PDT
I think there's an implicit CA animation running here.
Comment 4 Simon Fraser (smfr) 2020-05-24 10:40:18 PDT
Ah, the page has:
transition: all .25s cubic-bezier(0.165, 0.84, 0.44, 1);

So I think this is about how animations behave if you repeatedly trigger them by changing the target value (i.e. interrupted transitions).
Comment 5 Antoine Quint 2020-05-25 05:37:26 PDT
This doesn't reproduce with Web Animations disabled, this is a regression of the previous behavior, and given Chrome and Firefox's behavior, the previous implementation was correct and the Web Animations one incorrect.
Comment 6 Antoine Quint 2020-05-27 08:47:51 PDT
The generated transform transitions seem to be as expected, albeit nonsensical since they're running transitions over small increments. So it's something to do with the generated CA animations. Maybe we fail to cancel some of them?
Comment 7 Antoine Quint 2020-05-28 02:45:15 PDT
The issue is that we're always using the same `from` value for the transitions on each change of scroll position, instead of using the value that set in the previous style change.
Comment 8 Antoine Quint 2020-05-28 03:07:31 PDT
Created attachment 400440 [details]
Reduction

Reduction shows how we're always using the initial style value for a transitioned element when we start a new transition instead of the current computed value.
Comment 9 Antoine Quint 2020-05-28 03:21:55 PDT
Created attachment 400441 [details]
Reduction

Update reduction shows that this is likely an animation timing issue. The test now shows the startTime and progress for each transition before it gets replaced by a new one. If you look at Firefox Nightly, Chrome Canary and WebKit ToT you notice three different things:

    - Firefox: startTime is somewhere between timeline.currentTime for the previous and current frames and as a result progress > 0
    - Chrome: startTime is timeline.currentTime value for the previous frame (with a rounding error) and as a result progress > 0
    - WebKit: startTime is timeline.currentTime value for the _current_ frame and as a result progress = 0

Our transitions update code is probably correct and picks up the current animated value, but that value is the same as the initial value because the animation hasn't progressed yet.
Comment 10 Antoine Quint 2020-05-28 12:45:19 PDT
Have a source change that fixes this issue, but it also regresses a fair few WPT tests. I thought the issue was that we would run the pending play task while updating animations and instead opted to do it in Page::doAfterUpdateRendering(). Need to look harder at this.
Comment 11 Antoine Quint 2020-05-29 03:24:21 PDT
Here's what I'm going to try: place all newly-pending animations in a "pending animations" list on DocumentTimelinesController, then when we resolve style for the first time for one of those animations, move them to a separate "pending animations with resolved styles" list, and when the "pending animations" list is empty, process all items in the "pending animations with resolved styles" list and run their pending play or pause tasks. In the case of animations that can run accelerated, we might want to delay adding them to the "pending animations with resolved styles" list until the CA animation has been committed, although that might not be necessary since the first frame may be drawn by style anyway.

For CSS Animations and CSS Transitions, this means that those animations will be marked as ready in the next microtask since they'll update styles immediately after being created under Style::TreeResolver::createAnimatedElementUpdate().
Comment 12 Antoine Quint 2020-06-04 09:36:57 PDT
After many attempts, I believe I have the right approach to fixing this. There are however a handful of regressions that need to be addressed. I'll send an EWS patch for broader testing first, but there is hope that this can be fixed shortly.
Comment 13 Antoine Quint 2020-06-04 09:37:39 PDT
Created attachment 401029 [details]
Patch for EWS
Comment 14 Antoine Quint 2020-06-17 14:03:28 PDT
Created attachment 402153 [details]
Patch for EWS
Comment 15 Antoine Quint 2020-06-24 09:57:43 PDT
Created attachment 402659 [details]
Patch
Comment 16 EWS 2020-06-24 10:58:51 PDT
Committed r263464: <https://trac.webkit.org/changeset/263464>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402659 [details].