LibWeb: Store animation time values in abstract type #7030
Conversation
We always have a value for this so there is no need to have it as optional
This method did two things: 1) on the base class (`AnimationTimeline`) it was a setter for `m_current_time` and; 2) on the child classes (e.g. `DocumentTimeline`) it updated the timeline's current time given a document timestamp It makes more sense for theses to be distinct methods
This was added in Level 2 of the Web Animations spec In this level of the spec, all time values associated with animations (including this duration property) are no longer simple numbers but are instead either percentages or numbers for progress- and time-based timelines respectively. These values are represented internally by a new `TimeValue` wrapper struct, and externally (i.e. in JS) as `CSSNumberish`.
Web Animations Level 2 disallows setting some `AnimationEffect` timing values (start delay, end delay, iteration duration) directly and instead allows authors to set the specified values which are then normalized into the actual used values taking into account the type of the associated timeline (i.e. progress- vs time-based)
ac56b76 to
213c443
Compare
|
Changes: Fixed two GCC compilation issues |
AtkinsSJ
left a comment
There was a problem hiding this comment.
It'll be nice when the spec progresses to not being a delta. 😅
| // 8. Set animation’s start time to new start time. | ||
| m_start_time = valid_start_time; |
There was a problem hiding this comment.
It's not clear to me if this is correct now. Maybe a spec bug?
There was a problem hiding this comment.
I'm not sure what you are referring to here, all seems correct to me (possibly confusion around the naming of the valid_start_time variable which is resolved in your other comment)
There was a problem hiding this comment.
It gets a bit confusing because when you switch to the updated spec text, it now has both new start time and valid start time as variables. Step 1 defines valid start time as different from new start time, and then step 8 here uses new start time. My question is which one should actually be used in step 8.
It'd also mean renaming the variables back in this commit. 😅
There was a problem hiding this comment.
valid start time in the spec is just a boolean which is whether new start time is valid, the valid_start_time variable I introduced was ad-hoc and has been renamed (see other comment).
We don't actually need the valid start time variable in our implementation since we handle the invalid case by throwing which is handled by the TRY() macro (perhaps we need an AD-HOC comment here?).
In level 2 of the web animations spec, times are no longer always measures in milliseconds, they can also be percents when dealing with progress-based (i.e. scroll-based) timelines. We don't actually support percent times yet but this change will make it easier to implement when we do.
This also allows us to remove the now redundant `CSSNumberishTime::as_milliseconds()` method
213c443 to
17fc57f
Compare
In level 2 of the Web Animations spec, all time values associated with animations are no longer simple numbers but are instead either percentages or numbers for progress- and time-based timelines respectively. These values are represented internally by a new
TimeValuewrapper struct, and externally (i.e. in JS) asCSSNumberish.This is a another step towards supporting progress (i.e. scroll) based animations.
See individual commits for details.