Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[web-animations-1] Don't preserve current time for scroll linked animations when changing playbackRate #2075

Closed
birtles opened this issue Dec 5, 2017 · 17 comments · Fixed by #8090

Comments

@birtles
Copy link
Contributor

birtles commented Dec 5, 2017

From @stephenmcgruer on November 3, 2017 14:28

According to the spec, current time is calculated as:

current time = (timeline time - start time) × playback rate

This makes sense for DocumentTimeline, where we want to provide an offset from the start of the animation, but it doesn't seem to make sense for a ScrollTimeline (https://wicg.github.io/scroll-animations/#scrolltimeline-interface). Consider the following case:

let effect = new KeyframeEffect(
    foo,
    [ { width: "0vw" }, { width: "100vw" } ],
    { duration: 1000, easing: "linear" });
let timeline = new ScrollTimeline({
    scrollSource: document.documentElement,
    orientation: "block",
    range: "auto"
});
let anim1 = new Animation(effect, timeline);
anim1.play();

document.documentElement.scrollTop = "50vh";
let anim2 = new Animation(effect, timeline);
anim2.play();

// According to the spec:
anim1.currentTime == 500
anim2.currentTime == 0

The start time is set from the timeline time. This means that if the user manages to scroll the scrollSource before the animation setup script runs (e.g. perhaps because the script has not yet loaded over the network), the animation will be offset by whatever value the user has already scrolled - which I don't think is what the developer would want.

A first thought (courtesy of majidvp@) is to differentiate between montonic (use start offset) and non-monotonic (don't use start offset) timelines. DocumentTimeline is obviously monotonic, whereas ScrollTimeline isn't.

Copied from original issue: w3c/web-animations#204

@birtles
Copy link
Contributor Author

birtles commented Dec 5, 2017

#174 #2066 is possibly related.

@majido
Copy link
Contributor

majido commented Apr 3, 2018

Any other ideas on how this can be fixed beside treating these timeline differently?

  • @theres-waldo for visibility since this involves scroll timeline.

FYI, the link above is no longer correct after migration it should be #2066.

@majido
Copy link
Contributor

majido commented Jan 16, 2019

So far, for our implementation of ScrollTimeline in Chrome we have treated scroll linked animations as it they always have a set Zero start time (i.e., as if they always start at 0).

However this breaks some other behaviors in web-animations specifically where startTime is adjusted during seeking. The main two cases that I know about are:

  1. when currentTime of a playing animation is set via animation.currentTime.
  2. when playbackRate is changed, in order to maintain the previous currentTime and prevents a jump in the animation output.

By making the startTime be always Zero for scroll-linked animations we are preventing these to happen as expected. So for (1) setting the currentTime of a playing animation is simply ignored and for (2) we will see a jump in animation output as you pointed out.

Here are potential solutions to this that we have thought about:

  • A - Keep scroll-linked animation's startTime always Zero (this is current Chromium behavior) and accept the above implications of currentTime jumping in certain cases.
  • B. Always start an scroll-linked animation with Zero startTime but allow the startTime to change afterward e.g., when seeking. This addresses the main concern in original issue but has its own odd behavior e.g., startTime can fall outside valid range of the ScrollTimeline! ^
  • C. (B) without the initial behavior which is simply what web-animations does and I think can be footgun for devs per initial bug report.

Any other ideas or alternatives here?

^: Consider an scroll-linked animation @ t = T * 0.5 where T is the ScrollTimeline's timeRange. If one sets the currentTime of the animations to T, it will force its startTime to be adjusted to -T * 0.5 which is outside the valid range of the scrollTimeline. Similar situation can arise by adjusting playbackRate.

@majido
Copy link
Contributor

majido commented Jan 16, 2019

/cc @stephenmcgruer

@ogerchikov
Copy link
Collaborator

Another solution is:

  • D. Redefine currentTime for scroll-linked animations to be based on the Timeline."currentDelta" as opposed to the Timeline.currentTime as follows:
current_timeline_time = Timeline.currentTime;
current_time = prev_current_time + (current_timeline_time – prev_timeline_time) * playback_rate;
prev_current_time = current_time;
prev_timeline_time = current_timeline_time;

This solution will ensure that scroll and document timeline linked animations behave similarly (no jump in the animation output) when playback_rate is changed.

@birtles
Copy link
Contributor Author

birtles commented Jan 21, 2019

I think #2066 was toying with the idea of simply changing the behavior when the timeline of an animation is initially set. That is, it's similar to B, but it's not related to when an animation is started, but simply the moment when it's timeline is assigned (where we ignore the start offset if the assigned timeline is either null or a non-monotonic one).

@ogerchikov ogerchikov added the scroll-animations-1 Current Work label Mar 27, 2020
ogerchikov added a commit that referenced this issue Apr 13, 2020
JTensai pushed a commit to JTensai/csswg-drafts that referenced this issue May 13, 2020
ogerchikov added a commit that referenced this issue May 28, 2020
…ns (#2075) (#5059)

* Fixed play animation procedure for scroll animations.

* Refactored play and pause procedures.
@flackr
Copy link
Contributor

flackr commented Aug 11, 2022

I think we effectively have the behavior of B in the spec now for playing an animation:

In the procedure to play an animation step 5 sets the seek time to either 0 or effect end (based on playbackRate), and then step 6 sets the start time to seek time. This should correctly handle the case outlined in the OP.

Note that we do still preserve the current time (which would adjust the start time) when setting the playback rate. If we think that the intent for updating the playback rate on a scroll linked animation is to make the effect progress faster or slower with the original timing then perhaps we should update this procedure as well to not update the current time for such animations.

@flackr
Copy link
Contributor

flackr commented Aug 11, 2022

One other use case that I think may come up more often is setting the timeline of an animation. For example, if you specify:

animation-timeline: foo;

This may bind to different named scroll or view timelines throughout that animation's lifetime, which would result in setting the timeline of an already running animation. I think the developer would expect the start time to remain 0 when going to a scroll/view timeline.

Update: Going through the procedure, it looks like the start time should not be updated by setting the timeline as it runs the procedure to update an animation's finished state with did_seek = false.

@birtles
Copy link
Contributor Author

birtles commented Aug 12, 2022

Note that we do still preserve the current time (which would adjust the start time) when setting the playback rate. If we think that the intent for updating the playback rate on a scroll linked animation is to make the effect progress faster or slower with the original timing then perhaps we should update this procedure as well to not update the current time for such animations.

Sorry, I really don't have all the scroll animation timeline behavior in my head, but first impression is that it's probably not important to preserve the current time for a scroll animation (or any animation operating on a finite timeline) when updating the playback rate. At least to me, jumping would seem acceptable in that case.

@flackr flackr changed the title [web-animations-1] Current time calculation has odd interaction with ScrollTimeline [web-animations-1] Don't preserve current time for scroll linked animations when changing playbackRate Oct 7, 2022
@flackr
Copy link
Contributor

flackr commented Nov 8, 2022

Proposed resolution:

When setting the playback rate, skip step 4 (setting the current time to previous time) if has finite timeline (i.e. the timeline is scroll linked).

@kevers-google
Copy link
Contributor

The start time of a running scroll-linked animation is set to 0 or 100% depending on whether the playback rate is negative. If we preserve the start time when updating the playback rate and the new playback rate has an inverted sign, then we have a scroll linked animation that is either running in the negative direction from the start or in the positive direction from the end. Both cases result in an animation that is stuck in the before or after effect phases.

If the new and old playback rate have opposite signs, we should probably update the start time to the other endpoint of the timeline: start time 0 for positive playback rate, or start time 100% for a negative playback rate.

@kevers-google kevers-google reopened this Nov 25, 2022
@kevers-google
Copy link
Contributor

Looks like we have a similar issue when calling reverse() an animation with a scroll-linked animation. If current time is resolved and in bounds, then a new seek time is not set. When we run the "ready" task, we try to preserve the value of current time.

current time to match = (ready time - old start time) × old playback rate
new start time = ready time - current time to match / new playback rate

This update can cause the start time to become unaligned with an endpoint of the scroll range.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 28, 2022
…nimation

This patch does not fully align with recent spec resolutions since
the spec does not properly address reversing a scroll-linking
animation via playbackRate or reverse.

This patch does not handle the zero to non-zero case, or the reverse
problem, but posting it early to help drive spec resolution.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
@flackr
Copy link
Contributor

flackr commented Nov 28, 2022

If the new and old playback rate have opposite signs, we should probably update the start time to the other endpoint of the timeline: start time 0 for positive playback rate, or start time 100% for a negative playback rate.

Handling all of the edge cases for this to just work may get complicated. For example:

  • What if the developer sets a start time - possibly 0 or non-zero? It seems that we should perhaps preserve this. Perhaps the specified start time should effectively become an end time in reverse.
  • Do we also need to flip the animation delays? Otherwise animation-delay: enter 0% enter 100% would resolve to a start delay after the end of the animation, right?

flackr pushed a commit to flackr/csswg-drafts that referenced this issue Nov 29, 2022
When updating the playback rate of a non-monotonically increasing
timeline we want to preserve the effective start time behavior of
playing the animation. If the playback rate flips directions this
requires flipping the start time to the other end of the effect. See
discussion on w3c#2075.
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 29, 2022
…tion

* Preserve start time when updating playback rate for a scroll-linked
  animation.
* If changing playback rate for < 0 to >= 0 or vice versa, and start
  time is resolved:
  start time = effect end - start time.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 1, 2022
…tion

* Preserve start time when updating playback rate for a scroll-linked
  animation.
* If changing playback rate for < 0 to >= 0 or vice versa, and start
  time is resolved:
  start time = effect end - start time.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4060553
Reviewed-by: Robert Flack <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1078080}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 1, 2022
…tion

* Preserve start time when updating playback rate for a scroll-linked
  animation.
* If changing playback rate for < 0 to >= 0 or vice versa, and start
  time is resolved:
  start time = effect end - start time.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4060553
Reviewed-by: Robert Flack <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1078080}
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Dec 1, 2022
…tion

* Preserve start time when updating playback rate for a scroll-linked
  animation.
* If changing playback rate for < 0 to >= 0 or vice versa, and start
  time is resolved:
  start time = effect end - start time.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4060553
Reviewed-by: Robert Flack <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1078080}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 11, 2022
…ate for a scroll-linked animation, a=testonly

Automatic update from web-platform-tests
Update handling of changes to playback rate for a scroll-linked animation

* Preserve start time when updating playback rate for a scroll-linked
  animation.
* If changing playback rate for < 0 to >= 0 or vice versa, and start
  time is resolved:
  start time = effect end - start time.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4060553
Reviewed-by: Robert Flack <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1078080}

--

wpt-commits: b8e1b9e39c9f595844a07ae89438266a6274aef3
wpt-pr: 37191
BruceDai pushed a commit to BruceDai/wpt that referenced this issue Dec 13, 2022
…tion

* Preserve start time when updating playback rate for a scroll-linked
  animation.
* If changing playback rate for < 0 to >= 0 or vice versa, and start
  time is resolved:
  start time = effect end - start time.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4060553
Reviewed-by: Robert Flack <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1078080}
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Dec 14, 2022
…ate for a scroll-linked animation, a=testonly

Automatic update from web-platform-tests
Update handling of changes to playback rate for a scroll-linked animation

* Preserve start time when updating playback rate for a scroll-linked
  animation.
* If changing playback rate for < 0 to >= 0 or vice versa, and start
  time is resolved:
  start time = effect end - start time.

Spec issue is here: w3c/csswg-drafts#2075

Bug: 1393060
Change-Id: I1afc5618decc8fdf245a230ae4af7de87ee8caf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4060553
Reviewed-by: Robert Flack <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1078080}

--

wpt-commits: b8e1b9e39c9f595844a07ae89438266a6274aef3
wpt-pr: 37191
@flackr flackr added the Agenda+ label Dec 15, 2022
@flackr
Copy link
Contributor

flackr commented Dec 15, 2022

@kevers-google can you add the demo and video from your implementation of this feature to make it easier to visualize the behavior now?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [web-animations-1] Don't preserve current time for scroll linked animations when changing playbackRate, and agreed to the following:

  • RESOLVED: Accept flackr's fix
The full IRC log of that discussion <TabAtkins> flackr: We set up SLA so the time of th etimeline in which you play them isn't used for the start time, that would lead to skew with pages starting their SLA after the scroll started
<TabAtkins> flackr: We didn't account for some of the other APIs tho. I did an audit and playbackRate is the only one we haven't fixed yet.
<TabAtkins> flackr: So this proposal is that SLA doesn't try to preserve their current time when you change playback rate
<TabAtkins> flackr: I landed a simple fix, but Kevin brought up that when you reverse the naimation via negative playback rate, we shoudl fix it the same way as when you actually do a reversed animations.
<TabAtkins> flackr: So I landed a fix to change the start time there.
<TabAtkins> flackr: This aligns the playbackRate change to the behavior when you call .play()
<Rossen_> Zakim, open queue
<Zakim> ok, Rossen_, the speaker queue is open
<TabAtkins> I didn't look in detail but the expl makes sense to me
<TabAtkins> Rossen_: Objections?
<TabAtkins> RESOLVED: Accept flackr's fix

@flackr
Copy link
Contributor

flackr commented Mar 10, 2023

PR in progress in #8146

flackr added a commit that referenced this issue Mar 29, 2023
…pped on non-monotonic timeline (#8146)

When updating the playback rate of a non-monotonically increasing
timeline we want to preserve the effective start time behavior of
playing the animation. If the playback rate flips directions this
requires flipping the start time to the other end of the effect. See
discussion on #2075.
@flackr
Copy link
Contributor

flackr commented Mar 29, 2023

With the PR landed I believe we can close out this issue.

@flackr flackr closed this as completed Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants