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

[scroll-animations-1] currentTime for animations attached to a timeline attachment range #8669

Closed
bramus opened this issue Apr 2, 2023 · 11 comments
Labels

Comments

@bramus
Copy link
Contributor

bramus commented Apr 2, 2023

While playing with ViewTimelines and trying to derive how much of an animation-range had already progressed, it surprised me that reading currentTime always returned the progress relative to the entire ViewTimeline range.

To demonstrate, I have a demo up on CodePen which you can check using the latest Chrome Canary. The demo tries to resprent the progression within the currently set animation-range for the ViewTimeline and has these controls:

  • Using the radio buttons it’s possible to change the animation-range
  • Using the “set range-start to 20% and range-end to 80%“ checkbox it’s possible to set the range-start and range-end percentages to 20% and 80% respectively. When not checked, the default values of 0% and 100% are used.
  • The “force main thread animation“ checkbox is added to work around https://crbug.com/1421690

On load, the animation-range is set to entry 20% entry 80%.

The demo outputs the currentTime in three possible ways:

  1. Directly read it from the animation.

    let progress = animation.currentTime;

    This doesn’t work when set to a value other than cover. This because italways returns the progress measured against the entire range of the ViewTimeline (which coincides with cover)

  2. Read it from the range

    let progress = animation.timeline.getCurrentTime(animation.rangeStart.rangeName).value;

    Using getCurrentTime[ForRange] doesn’t cut it here things here, as it only covers animations that have the same rangeName in animation-range-start and animation-range-end. Above that, it also only works for when the entire range is covered (from 0% to 100%).

  3. Read from the animation’s effect

    As a workaround, I turned to reading the animation’s effect, which did the trick:

    // This works
    let progress = animation.effect.getComputedTiming().progress * 100;
    if (animation.playState === 'finished') progress = 100;

    This, however, felt a lot like jumping through hoops to be honest.

To simplify things for authors, I would like to propose that currentTime (the method used in the first approach) returns the progress relative to the timeline attachment range.

  • If one explicitly sets the animation-range to, for example, entry 50% cover 100% the currentTime should be relative to that set (custom) range.
  • If one sets no animation-range, the progress should be relative to the entire range, which for View Timelines equals to cover

I think this change also lines up with what @fantasai described here (and which was adopted by the WG):

The animation is attached to a timeline. But more specifically, it is attached to a timeline attachment range. By default this is the entire timeline, which in the case of a scroll-driven timeline is finite, and in the case of a time-driven timeline is infinite. The attachment range is the portion of the timeline that the animation is laid out in (somewhat analogous to a containing block).

Note that in this case getCurrentTime[ForRange] would still be relevant to keep, as it allows you to get the currentTime as if the range was spanning from 0% to 100%.

WDYT @fantasai @flackr @kevers-google @birtles?

@bramus bramus added the scroll-animations-1 Current Work label Apr 2, 2023
@flackr
Copy link
Contributor

flackr commented Apr 3, 2023

The current time of an animation is spec'd in web-animations-1 4.4.3 as the following:

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

In this sense, the current time is in the units of the timeline time and is working as expected. I can see how this is confusing when the timeline time is also in percentage units, but it is consistent with how the currentTime in a regular animation does not give you the progress, but rather the timeline time that has "passed" since the start of the animation.

@fantasai
Copy link
Collaborator

fantasai commented Apr 3, 2023

I agree with @flackr that this is working as expected. Timeline attachment ranges are a property of the animation, not of the timeline. .currentTime is answering "what time is it on this timeline", not "how far has this animation progressed".

Note: We decided to go with percentages for measuring "time" on scroll/view timelines, but we could have just as easily gone with pixels; maybe the intuitive answer here would be different if we had gone with pixels?

@bramus
Copy link
Contributor Author

bramus commented Apr 4, 2023

I see.

Maybe then, for author convenience, we could alter getCurrentTimeForRange() so that when it has no argument set, it gives the progress relative to the specified range? This wouldn’t alter the current inner workings of .currentTime while also making getCurrentTimeForRange() more useful.

@fantasai
Copy link
Collaborator

fantasai commented Apr 4, 2023

@bramus That would require the timeline to belong to an animation, i.e. have a one-to-one mapping between a timeline and an animation, right? Because each animation can have its own animation attachment range. But isn't it the case that a timeline can be associated with multiple animations?

@bramus
Copy link
Contributor Author

bramus commented Apr 6, 2023

Good call. Could be solved by pass the animation.rangeStart and animation.rangeEnd into getCurrentTime[ForRange], so that it can do its calculations.

I think that would rhyme nicely with the suggested property bag approach, no?

const { timeline, rangeStart, rangeEnd } = animation;
 
const value = timeline.getCurrentTime({
  rangeStart,
  rangeEnd
});

@flackr
Copy link
Contributor

flackr commented Apr 6, 2023

If you have to pass in the specific animation rangeStart and rangeEnd and you want the animation progress, I feel like the computed timing on the effect is a better fit, e.g.:

const value = animation.effect.getComputedTiming().progress;

I also have concerns with getCurrentTime being expected to produce a progress. Maybe we could consider whether we should expose progress on the animation object directly? e.g. animation.progress

@bramus
Copy link
Contributor Author

bramus commented Apr 6, 2023

Maybe we could consider whether we should expose progress on the animation object directly? e.g. animation.progress

Oh, that would be the easiest indeed, and nicely compliment currentTime

@birtles
Copy link
Contributor

birtles commented Apr 10, 2023

I'm not sure mapping animation.progress to animation.effect.getComputedTiming().progress is a good idea.

The returned value is based on the various timing properties of the of rootmost effect so it feels like a layering violation to make it look like a property of the animation.

Admittedly, we do something similar when we report an animation as "finished" since the point in time at which it finishes is based on the timing of its rootmost effect, if any but we've very deliberately chosen to make that compromise for that specific case and we've gone all in on it, adding finish(), finished and finish events.

I'm not sure if reporting the iteration progress for the rootmost effect deserves that kind of treatment, especially because it is the iteration progress.

If we need something to report the fraction from where an animation starts to where it finishes, maybe we should introduce a new concept for that (e.g. "animation progress") and expose that instead? Such a value would presumably be independent of any delays etc. specified on the rootmost effect.

I guess it depends on what the ultimate use case is for this?

@flackr
Copy link
Contributor

flackr commented Apr 11, 2023

I'm not sure mapping animation.progress to animation.effect.getComputedTiming().progress is a good idea.

The returned value is based on the various timing properties of the of rootmost effect so it feels like a layering violation to make it look like a property of the animation.

I agree, see below.

Admittedly, we do something similar when we report an animation as "finished" since the point in time at which it finishes is based on the timing of its rootmost effect, if any but we've very deliberately chosen to make that compromise for that specific case and we've gone all in on it, adding finish(), finished and finish events.

I'm not sure if reporting the iteration progress for the rootmost effect deserves that kind of treatment, especially because it is the iteration progress.

If we need something to report the fraction from where an animation starts to where it finishes, maybe we should introduce a new concept for that (e.g. "animation progress") and expose that instead? Such a value would presumably be independent of any delays etc. specified on the rootmost effect.

Yeah I was thinking that if we did introduce something like this it could be the overall animation progress through the active interval. It would of course then have to be undefined for an animation with infinite iterations.

Another thing that might have made @bramus's use case simpler is if iteration progress were accessible without needing to call getComputedTiming(), e.g. animation.effect.progress.

I guess it depends on what the ultimate use case is for this?

I agree we should think about what the particular use case is for this to make sure we're looking at this in the context of how it will be used. If it's for demos such as the one that this issue was filed for I suspect the root effect's iteration progress is what's expected - but then I agree with @birtles that it should be on the root effect and not on the animation.

@flackr
Copy link
Contributor

flackr commented May 12, 2023

@bramus can you review the proposal in #8799 and close this if that meets the needs?

@bramus
Copy link
Contributor Author

bramus commented May 12, 2023

In favor of a general approach as described in #8799.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants