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][web-animations-2] getCurrentTime is self-inconsistent wrt representing time #8765

Open
fantasai opened this issue Apr 24, 2023 · 9 comments

Comments

@fantasai
Copy link
Collaborator

fantasai commented Apr 24, 2023

In order to represent time in relation to named timeline ranges, we have two options:

  • Represent as absolute progress from the start of the range, in units consistent with the overall timeline (i.e. equivalent to currentTime).
  • Represent as percentage progress between the start and end of the range.

If we consider its default behavior as returning the time with respect to a timeline range that encompasses the entire timeline, the current definition for getCurrentTime() switches between the two modes depending on whether you specify a range name or not. (!)

This doesn't feel unnatural to scroll timelines, because their “absolute timeline units” are percentages of the timeline (which itself has a finite range)... We would probably have noticed the discrepency sooner if they used length-based time. :/

But it runs into problems if we extend the named timeline concept to other types of timelines, such as time-based timelines. These bring in (potentially) two differences:

  • Their absolute time is not a percentage, but an absolute unit.
  • They might possibly not be finite?

I think we might want to rethink this API. Some possible options:

  • Restrict (and define) it as returning percentage progress through a range, rather than also being able to return the global currentTime, and name it accordingly as @birtles suggests in [scroll-animations-1] Animation.getCurrentTime is easily confused with Animation.currentTime #8201.
  • Define two APIs, one that returns percentage progress and one that returns absolute progress (i.e. units compatible with currentTime) in relation to a named range.
  • Define a single API that can return either percentage progress or absolute progress depending on its arguments.

I'm not sure which direction we want to go... @bramus, thoughts?

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

bramus commented Apr 25, 2023

I can see that as an author you’d sometimes want to know the relative “time” (i.e. a percentage) and sometimes you’d want the absolute “time”. We could go with two different methods getRelative… and getAbsolute… for this, or add an extra argument to the property bag of the existing one. Undecided that the default argument value – relative or absolute – should be in the latter case.

Side note: seeing that I put time between quotes in the previous paragraph and @fantasai used the word “progress” – and I also hinted at it here – maybe it’s better to rename the method to getProgress()?

@fantasai
Copy link
Collaborator Author

fantasai commented Apr 25, 2023

@bramus ... so maybe we want parallel getCurrentTime and getCurrentProgress functions where the former is in .currentTime units and the latter is in percentages (always) and returns null on non-finite ranges?

  • Option A: getCurrentTime() to return the absolute time offset and getCurrentProgress() to get the percentage time offset. Passing { range: "name" } gets the offset / percentage relative to that range, otherwise relative to entire timeline.
  • Option B: getCurrentTimeForRange("name") to return the absolute time offset from the start of the named range, getCurrentProgressForRange("name") to return the percentage progress through the named range.

@bramus
Copy link
Contributor

bramus commented May 3, 2023

/ping @flackr to get your take on this. I think this needs to be tackled before shipping.

@birtles
Copy link
Contributor

birtles commented May 3, 2023

I agree this should be resolved before shipping. I wonder how important the API even is? Do authors need it? Is it needed for WPT? If we're not sure what the ergonomics/naming/use cases are for this, would it make sense to ship without it?

@flackr
Copy link
Contributor

flackr commented May 4, 2023

I agree with @birtles that I think this is purely additive to scroll driven animations and doesn't need to be part of the initial API. I'm in favor of shipping without it and adding it later.

@flackr
Copy link
Contributor

flackr commented May 4, 2023

The use cases I've seen presented for this have been about matching an animation's progress at which point it seems like we should provide an animation-wide progress or suggest developers access the effect progress rather than trying to make an API which usually aligns with an animation effect.

That said, if we think an API is needed on the timeline for use cases which aren't trying to match an animation progress, I would suggest using terminology like progress if we are going to return progress through a range rather than an absolute time.

@fantasai
Copy link
Collaborator Author

fantasai commented Jun 1, 2023

So we resolved to add .progress to the Animation API in #8799 ; but that still leaves this issue open. If we're deferring getCurrentTime() from Level 1, we need to remove it from the specs. If we want to fix this issue, then we need to pick a solution.

@birtles
Copy link
Contributor

birtles commented Jun 1, 2023

So long as we don't have a strong use case (it seems like .progress covers the use cases we're aware of) and we don't have a clear picture of what the shape of the API should me, I'm very much in favor of dropping it from Level 1.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [scroll-animations-1][web-animations-2] getCurrentTime is self-inconsistent wrt representing time, and agreed to the following:

  • RESOLVED: Defer on getCurrentTime for L1
The full IRC log of that discussion <dael> flackr: We've had a bunch of debate about best way getCurrentTime on a timeline should work, if needed, what are use cases. Resolve dto add progress to animation to handle common use case
<dael> flackr: With that, think we should remove getCurrentTime from the APIa nd re-add when we have clearer sense of use cases
<dael> fantasai: I'm fine with deferring. It means there is no real way to figure out where you are in a timeline range. So you don't know when unless you make an animation and measure progress. We don't have API to say how ranges relate to rest of timeline
<dael> fantasai: But I'm fine with deferring. Not a problem
<dael> Rossen_: Anyone else?
<dael> Rossen_: Objections to defer on getCurrentTime
<dael> RESOLVED: Defer on getCurrentTime for L1
<dael> Rossen_: I'm assuming bramus is on same page as you? I don't see us doing a disservice for any commentor

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

No branches or pull requests

5 participants