Repository navigation
[scroll-animations-1] Animation.getCurrentTime is easily confused with Animation.currentTime #8201
Description
I think it's confusing to have both currentTime and getCurrentTime.
Would it be possible to:
Move this to the(Update: I see now that this is proposed to be used forScrollTimelineinterface,DocumentTimelinetoo--I still don't quite follow the use cases but assuming we want to do that, I guess it should stay onAnimationTimeline)- Rename it to
getCurrentTimeForRange, and - Make the
rangeNameargument non-optional
?
Activity
fantasai commentedon Dec 13, 2022
getCurrentTimeis essentially a replacement for.currentTimethat returns the same concept, but has more expressivity: So I think it makes sense to keep the same name.birtles commentedon Dec 14, 2022
I don't think we shouldn't do that. We'll confuse authors unnecessarily. We can't deprecate
currentTimeand for the overwhelming majority of use cases it's the right choice. We should be encouraging users to continue usingcurrentTimefor the sake of compatibility.For users that need a time for a range, there's
getCurrentTimeForRangefor which the function name spells out what the string argument represents. (If we really must usegetCurrentTimethen it needs to take a property bag, not a string.)css-meeting-bot commentedon Mar 20, 2023
The CSS Working Group just discussed
[scroll-animations-1] Animation.getCurrentTime is easily confused with Animation.currentTime, and agreed to the following:RESOLVED: Pending Brian's potential objections, go with `getCurrentTime(rangeName?)`The full IRC log of that discussion
<fantasai> s/Topic/Subtopic/<TabAtkins> flackr: Bikeshedding issue
<TabAtkins> flackr: we added an API to get a range-specific current time
<TabAtkins> flackr: But Brian brought up that when you're not specifying a range, and just getting the time of the timeline, there's already an accessor for that
<TabAtkins> flackr: So q is, should this get a range-specific name, or just consider the .currentTime accessor legacy?
<TabAtkins> flackr: Because one bit about the function is we can return the TypedOM object, rather than just the double that .currentTime returns
<TabAtkins> flackr: I'm slightly in preference to this being the new way to get times, so use a generic name, and have range name specified in options with a property bag as Brian suggested
<TabAtkins> astearns: But Brian thinks we shoudl go with getCurrentTimeForRange()?
<TabAtkins> flackr: Yeah, Brian thinks it's confusing to have another API for something we already have an accessor for
<astearns> ack fantasai
<TabAtkins> flackr: I'm not concerned with calling it getCurrenTtimeForRange(), just need to decide
<TabAtkins> fantasai: My thoughts are we'd add this to get the TypedOM object, and to get it relative to a range.
<TabAtkins> fantasai: Feel like taking a property bag is a little awkward from a usage pov
<TabAtkins> fantasai: I think ranges are a fundamental enough concept that having it just be a string argument is fine
<TabAtkins> flackr: His argument for a property bag is that we dont' want to confused authors, and might want to extend in the future
<TabAtkins> fantasai: Those can go in a property bag
<TabAtkins> astearns: I'm a little concerned with resolving without Brian's okay
<TabAtkins> flackr: Least contentious is to resolve on the ForRange naming, we know Brian is okay with that
<TabAtkins> astearns: fantasai, you okay with ForRange?
<TabAtkins> fantasai: Think it depends on if we want a plain getCurrentTime at any point
<TabAtkins> fantasai: Coujld push to apac call and get Brian on
<fantasai> TabAtkins: I'm mildly against extremely long names, especially if the arg is obvious, extra typing is annoying
<fantasai> TabAtkins: not strongly for/against property bag vs positional argument, but lean towards positional b/c fantasai's arg that it's quite fundamental
<fantasai> TabAtkins: if we put in property bag, we can make an overload later
<fantasai> TabAtkins: for API niceness, I think I like what I believe the spec currently specifies
<fantasai> TabAtkins: getCurrentTime("rangename")
<fantasai> TabAtkins: can add property bag later
<fantasai> flackr: Should we resolve this is what we prefer and ask Brian to take a look?
<TabAtkins> astearns: So proposed reoslution is to use the short name with a string argument, reasons above, ping Brian to see if he wants to object
<TabAtkins> astearns: objections?
<TabAtkins> RESOLVED: Pending Brian's potential objections, go with `getCurrentTime(rangeName?)`
<bramus> 8192?
birtles commentedon Mar 21, 2023
I'm afraid I'm still opposed to the current naming.
I don't think ranges are a fundamental concept. The overwhelming majority usage of the API will continue to be for time-based animations and I think we should bias towards that.
getCurrentTimemakes using the API harder in the common case. When your editor suggests bothcurrentTimeandgetCurrentTimehow do you know which one to choose? You have to look them up to find out what their (subtle) distinction is.currentTimebecause it is more backwards compatible.If we must have
getCurrentTimebecause we expect it to be useful for non-range cases then we really should use a property bag. Doing so is necessary to:getCurrentTimerepresents a range otherwise?Now that TS/JS language servers are able to automatically refactor positional arguments into property bags, property bags have become much more common so I believe this approach is also idiomatic.
bramus commentedon Apr 2, 2023
After playing with the API a bit, I follow what Brian is saying and think
getCurrentTimeForRangemakes most sense, as that’s what the function essentially does. Other names didn’t cover what the function does.An alternative name that came up was
getProgressForRangebut I see how reusingCurrentTimein the name makes sense becausecurrentTimealso returns a progress for non-time-based timelines.Also fine with the property bag approach. To be clear, that would then, for example, be
getCurrentTime({ rangeName: "cover" })?flackr commentedon Apr 6, 2023
What is the use case for this API? What would a developer do given the "time" of various phases on the timeline?
Given our discussion in #8669 it seems the thing @bramus would like would be best provided by a more convenient way to get a particular animation's progress.
LeaVerou commentedon Apr 11, 2023
Per the TAG Design Principles, optional and/or primitive arguments should be accepted through dictionaries, not positional arguments.
In this case, this would mean
getCurrentTime({ rangeName: "cover" })(or perhapsgetCurrentTime({ range: "cover" })). This also allows.getCurrentTime()without arguments to continue being equivalent to.currentTime, as it should be.css-meeting-bot commentedon Apr 12, 2023
The CSS Working Group just discussed
[scroll-animations-1] Animation.getCurrentTime is easily confused with Animation.currentTime, and agreed to the following:RESOLVED: Change getCurrentTime() parameter to a dict to satisfy Brian's/the TAG's concerns.The full IRC log of that discussion
<TabAtkins> astearns: Previously decided on using getCurrentTime() with a position param<TabAtkins> astearns: Comments since have noted that the tag prefers property bags
<TabAtkins> astearns: And the person who objects to the name would be okay with property bag instead of positional arg
<TabAtkins> astearns: So proposal is to change the previous resolution to use a property bag instead
<TabAtkins> astearns: Comments?
<TabAtkins> `getCurrentTime({ range: "cover" })` looks fine to me
<TabAtkins> astearns: Objections?
<TabAtkins> RESOLVED: Change getCurrentTime() parameter to a dict to satisfy Brian's/the TAG's concerns.
[scroll-animations-1] Use options dictionary for getCurrentTime() w3c…
[scroll-animations-1] Use options dictionary for getCurrentTime() #8201
yoavweiss commentedon May 17, 2023
Let me know if this requires a separate issue, but I was surprised to see that
getCurrentTimereturns a CSSNumericValue, rather than a DOMHighResTimestamp. I'm guessing that it'd also be worthwhile to hook up its processing/clamping to current high resolution timeflackr commentedon May 17, 2023
Scroll timelines don't produce millisecond-based times, so we decided that we should future proof APIs with a CSSNumericValue which can represent the difference between milliseconds and percentages. We should still apply the same clamping to those millisecond values in the CSSNumericValue as we do for DOMHighResTimestamp.
fantasai commentedon Jun 1, 2023
I believe this issue has been addressed, but there's some follow-up concerns in #8765