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-2] Specify Animation progress API #9937

Merged
merged 3 commits into from Feb 29, 2024

Conversation

DavMila
Copy link
Contributor

@DavMila DavMila commented Feb 12, 2024

This is to specify the agreed upon animation.progress API.

Copy link
Contributor

@birtles birtles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I suggested a few tweaks.

web-animations-2/Overview.bs Outdated Show resolved Hide resolved

: If <em>any</em> of the following are true:
* the animation does not have an [=animation/associated effect=], or
* the animation's associated effect's [=end time=] is infinite, or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the thinking behind making infinite → null as opposed to zero? I think zero might be more useful overall since it represents where the animation is actually up to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took @flackr 's point about distinguishing between 0 due to infinite duration and 0 that can actually make progress into consideration. But yes, 0 is probably more accurate. I'll write that in the algorithm.

web-animations-2/Overview.bs Outdated Show resolved Hide resolved
web-animations-2/Overview.bs Outdated Show resolved Hide resolved
web-animations-2/Overview.bs Outdated Show resolved Hide resolved
web-animations-2/Overview.bs Outdated Show resolved Hide resolved
@DavMila DavMila requested a review from birtles February 13, 2024 19:02

: If <em>any</em> of the following are true:
* |animation| does not have an [=animation/associated effect=], or
* |animation|'s [=associated effect end=] is zero, or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these edits. Two follow-up questions that occurred to me:

  1. If the associated effect end is zero, should the progress be 1 (or 0 if the current time is negative)? It seems odd that it's unresolved here but maybe I'm just not remembering how all this works?
  2. Just to check, should we be clamping the progress to [0, 1]? I assume we shouldn't but I guess it depends on how we expect this property to be used? The answer to this will likely affect how we define the behavior for zero-duration animations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these edits. Two follow-up questions that occurred to me:

  1. If the associated effect end is zero, should the progress be 1 (or 0 if the current time is negative)? It seems odd that it's unresolved here but maybe I'm just not remembering how all this works?

I agree, it more accurately reflects the animated progress to return a value here.

  1. Just to check, should we be clamping the progress to [0, 1]? I assume we shouldn't but I guess it depends on how we expect this property to be used? The answer to this will likely affect how we define the behavior for zero-duration animations.

It seems like it's more useful to allow progresses outside of the [0, 1] range, as especially for 0 it matters to know whether the effect is active or not. That said, allowing values outside of [0, 1] makes it more complicated for how to handle the 0 duration case. Perhaps we should use +/- infinity depending on whether you're before or after the start time?

@bramus @fantasai any thoughts here?

Copy link
Contributor

@flackr flackr Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking further on this, it may be simpler to restrict to [0, 1] and encourage authors to combine with the phase of the animation which seems like another thing that's not too easy to get today. Restricting to the [0, 1] range makes the answer to edge cases much easier to reason about, e.g. what we should do for a 0 duration effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you're right. Ultimately, though, it probably depends on how we expect the API to be used.

We could mimic the behaviour of progress on ComputedEffectTiming and use the fill mode and phase of the root animation effect to make progress be unresolved when the root effect is not in effect but that might be a bit of a layering violation?

For reference, I believe ComputedEffectTiming.progress is mostly in the range [0, 1] but can extend outside that range due to easing functions.

I guess it comes back to reviewing these issues and seeing what would work best in each of those cases (quoting #8799):

In #8669, #8765 and #8201 there seems to be a common desire to have more convenient APIs for getting some representation of progress.


: If <em>any</em> of the following are true:
* |animation| does not have an [=animation/associated effect=], or
* |animation|'s [=associated effect end=] is zero, or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these edits. Two follow-up questions that occurred to me:

  1. If the associated effect end is zero, should the progress be 1 (or 0 if the current time is negative)? It seems odd that it's unresolved here but maybe I'm just not remembering how all this works?

I agree, it more accurately reflects the animated progress to return a value here.

  1. Just to check, should we be clamping the progress to [0, 1]? I assume we shouldn't but I guess it depends on how we expect this property to be used? The answer to this will likely affect how we define the behavior for zero-duration animations.

It seems like it's more useful to allow progresses outside of the [0, 1] range, as especially for 0 it matters to know whether the effect is active or not. That said, allowing values outside of [0, 1] makes it more complicated for how to handle the 0 duration case. Perhaps we should use +/- infinity depending on whether you're before or after the start time?

@bramus @fantasai any thoughts here?

::
|animation|'s [=animation/progress=] is null.
: If |animation|'s [=associated effect end=] is infinite,
|animation|'s [=animation/progress=] is zero.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the author tell if we're active (after the start time) or inactive (before the start time)?

-Clamp progress to [0,1].
-Zero duration depends on nonegativity of currentTime.
@birtles
Copy link
Contributor

birtles commented Feb 22, 2024

I've reviewed this PR and I think it looks good. Thanks @DavMila for your work on this.

My only outstanding question is from here:

We could mimic the behaviour of progress on ComputedEffectTiming and use the fill mode and phase of the root animation effect to make progress be unresolved when the root effect is not in effect but that might be a bit of a layering violation?

...

I guess it comes back to reviewing these issues and seeing what would work best in each of those cases (quoting #8799):

In #8669, #8765 and #8201 there seems to be a common desire to have more convenient APIs for getting some representation of progress.

Does anyone know any specific intended uses of this API that would help determine if the progress should be unresolved when the animation is in the before/after phase and not filling?

@flackr
Copy link
Contributor

flackr commented Feb 29, 2024

I've reviewed this PR and I think it looks good. Thanks @DavMila for your work on this.

My only outstanding question is from here:

We could mimic the behaviour of progress on ComputedEffectTiming and use the fill mode and phase of the root animation effect to make progress be unresolved when the root effect is not in effect but that might be a bit of a layering violation?
...
I guess it comes back to reviewing these issues and seeing what would work best in each of those cases (quoting #8799):

In #8669, #8765 and #8201 there seems to be a common desire to have more convenient APIs for getting some representation of progress.

Does anyone know any specific intended uses of this API that would help determine if the progress should be unresolved when the animation is in the before/after phase and not filling?

I left a comment on the original issue #8799 (comment) to get a resolution here. I suppose we can land the clamping behavior and change it if we decide differently there.

@flackr flackr merged commit 79819a4 into w3c:main Feb 29, 2024
1 check passed
@DavMila DavMila deleted the animation.progress branch March 25, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants