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

[css-animations-2] Specify the animation-trigger property #10128

Merged
merged 23 commits into from
Jan 31, 2025

Conversation

ydaniv
Copy link
Contributor

@ydaniv ydaniv commented Mar 25, 2024

Adding initial draft for Animation Triggers and the animation-trigger property as resolved in #8942 (comment).

I cleared all the issues and fleshed out the gist of it.

cc @flackr @birtles @dbaron

@ydaniv ydaniv marked this pull request as ready for review March 28, 2024 11:57
Copy link
Contributor

@flackr flackr left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started! I think some of the low level details of how an animation trigger affects the animation probably belong in web-animations-1 with the css-animations spec setting properties by which you set up those triggers declaratively. You should also of course be able to set the same properties from the web-animations api.

css-animations-2/Overview.bs Show resolved Hide resolved
An <dfn export>animation trigger</dfn> is used to control the playback
of its associated [=animation=] for time-driven animations.
[=Animation triggers=] follow the same [[web-animations-1#hierarchical|hierarchy]]
as animations and are associated with a [=timeline=] and an [=animation trigger effect|effect=].
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it sound like animation triggers are separate things from animations, but I suspect we should say that as a part of updating the current time of animations we evaluate whether a trigger condition has been reached and apply a stateful change to the animation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll work on that

[=Animation triggers=] follow the same [[web-animations-1#hierarchical|hierarchy]]
as animations and are associated with a [=timeline=] and an [=animation trigger effect|effect=].

Issue: Should there be any effect of triggers on scroll-driven animations?
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect triggers on scroll-driven animations should "just work". E.g. they could play / stop the animation when the trigger condition occurs / finishes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does each type affects a scroll-driven animation?
Furthermore, what could be the actual use-case for this? Perhaps one that I can think of now is if we have a hover-timeline in the future and we want to enable/disable it on different scroll positions. Is that what you mean?

::

<dl class=switch>
: If |effect| is inside its [=active interval=],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think defining a trigger in terms of an internal animation effect's time might be overkill. E.g. I think we could define this simply in terms of the animation timeline's time with respect to the specified range

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 struggled with that quite a bit. Eventually it worked out once I defined the same hierarchy as animations', and defined a special state for the effect. I'll try simplifying this further.


Issue: Should there be any effect of triggers on scroll-driven animations?

## Animation Trigger Effects ## {#trigger-effects}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the internal control of how triggers work probably belongs in web-animations where we define things like how an updated time updates corresponding animations: https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events

We should also be able to set up animation triggers from javascript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, sure. We didn't discuss the WAAPI part of triggers in the issue, should I try fleshing that too in this PR? Or you're just stating this as a reason for this change?

@birtles
Copy link
Contributor

birtles commented Apr 16, 2024

@ydaniv Thanks for doing this. I started going through it today but I was a little confused about the hierarchy of components from the Web Animations API point-of-view. It sounds like we have the following parts:

  • Animation trigger timeline (currently a DocumentTimeline but possibly scroll-driven timelines too)
  • Animation trigger
  • Animation trigger effect
  • Animation

What is the relationship between them? Do the Animation trigger and Animation share an effect? Is the Animation trigger connected directory to the Animation trigger effect or only via the Animation? Is the Animation attached to the Animation trigger timeline or just the Animation trigger?

Sorry, I'm probably being really daft here but I couldn't grasp how the pieces fit together.

Defining some of these primitives as Web Animations constructs like @flackr suggested might help clarify how they fit together.

@ydaniv
Copy link
Contributor Author

ydaniv commented Apr 16, 2024

@birtles thanks for reviewing!

The intention of the hierarchy was mostly to simplify the spec, by having triggers follow animations as in:
For animations: timeline -> animation -> effect
For triggers: timeline -> trigger -> effect
And since effects are attached to attachment ranges, have trigger follow the same mechanism.

The idea was that they don't share anything, just follow the same structure, and the trigger simply affects the animation's playback according to its properties and state.

But, as @flackr mentioned, it's still too confusing and too complex, so I'll try and rewrite this without the effect part, and only with a trigger attached to a timeline and a range.

I've tried defining the constructs first in the initial draft. I'll move those into the Web Animations spec as suggested, and try to organize them together again in a simpler way.

@birtles
Copy link
Contributor

birtles commented Apr 17, 2024

The intention of the hierarchy was mostly to simplify the spec, by having triggers follow animations as in:
For animations: timeline -> animation -> effect
For triggers: timeline -> trigger -> effect
And since effects are attached to attachment ranges, have trigger follow the same mechanism.

The idea was that they don't share anything, just follow the same structure, and the trigger simply affects the animation's playback according to its properties and state.

Makes sense!

But, as @flackr mentioned, it's still too confusing and too complex, so I'll try and rewrite this without the effect part, and only with a trigger attached to a timeline and a range.

Ok, I'll look forward to seeing what you come up with!

Removed dfn of animation trigger effect
@ydaniv
Copy link
Contributor Author

ydaniv commented Apr 17, 2024

@flackr @birtles I've just pushed changes, I think I've covered all the comments, thanks!

Comment on lines 3563 to 3564
The [=animation effect=] associated with |trigger| remains in
its [=animation effect/before phase=] and stays at zero [=animation/start time=].
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't quite get the hierarchy here. This suggests an animation effect is associated with a trigger but previous we said a trigger is associated with an animation. Which is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the relation here should be indirect via the animation. The trigger is associated to the animation. The required behavior of the animation is to "hold its effect in a 'just before playing' state". I'll try to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I think it's better now.

@ydaniv
Copy link
Contributor Author

ydaniv commented May 1, 2024

kind ping @birtles @flackr, ready for another review.

@birtles
Copy link
Contributor

birtles commented May 3, 2024

kind ping @birtles flackr, ready for another review.

@ydaniv Sorry for the delay--currently on parental leave. Just a few meta points:

  1. I think this should be part of Web Animations level 2. Level 1 is basically done except for fixing existing issues (most notably the commitStyles changes we agreed to but I've yet to edit in).
  2. I think we need to define the API parts for triggers?
  3. We probably need to review all the descriptions about timelines and animations to make sure it accounts for triggers. (e.g. the sentence "Updating the current time of any animations associated with the timeline." should probably refer to triggers too?)

@ydaniv
Copy link
Contributor Author

ydaniv commented May 3, 2024

Thanks @birtles!

currently on parental leave

Congrats ^_^!

  1. I think this should be part of Web Animations level 2.

Ok, I also wondered about that, but @flackr wrote Web Animations 1, and some things I needed to update were actually there, so not sure whether this was intentional or not.

  1. I think we need to define the API parts for triggers?

You mean the DOM interfaces, as in here?

  1. We probably need to review all the descriptions about timelines and animations to make sure it accounts for triggers. (e.g. the sentence "Updating the current time of any animations associated with the timeline." should probably refer to triggers too?)

I've added this text to that section:

  • Updating the [=animation trigger state=] of any [=animation trigger=]
    [=associated with a timeline|associated with=] the timeline.

Shouldn't this cover it? I think basically comes down to checking triggers state on timeline's current time update.

@birtles
Copy link
Contributor

birtles commented May 4, 2024

currently on parental leave

Congrats ^_^!

Thanks!

  1. I think this should be part of Web Animations level 2.

Ok, I also wondered about that, but @flackr wrote Web Animations 1, and some things I needed to update were actually there, so not sure whether this was intentional or not.

Yeah, that would be good to know. My understanding is it should be level 2 but maybe @flackr has some ideas about that.

  1. I think we need to define the API parts for triggers?

You mean the DOM interfaces, as in here?

I was thinking about the Web Animations DOM interfaces. Just like we have the Animation interface I assume we'll have an AnimationTrigger interface?

That also makes me wonder how authors would expect to be able to inspect these from script. Is there some way to fetch the triggers in a document? Or the trigger driving a particular Animation?

  1. We probably need to review all the descriptions about timelines and animations to make sure it accounts for triggers. (e.g. the sentence "Updating the current time of any animations associated with the timeline." should probably refer to triggers too?)

I've added this text to that section:

  • Updating the [=animation trigger state=] of any [=animation trigger=]
    [=associated with a timeline|associated with=] the timeline.

Shouldn't this cover it? I think basically comes down to checking triggers state on timeline's current time update.

Yeah, I was thinking that we need to review the whole section to check for other references that need to be updated like the sentence I mentioned and other definitions like this one:

An animation effect, effect, is associated with a timeline, timeline, if effect is associated with an animation which, in turn, is associated with timeline.

Presumably that needs to be updated to include triggers too?

This is a pretty fundamental change to the whole timing hierarchy so we need to be careful about it. I wonder if it would be better to introduce an abstract concept that covers both animations and animation triggers like "timeline subscribers" or somesuch.

@ydaniv
Copy link
Contributor Author

ydaniv commented May 5, 2024

I was thinking about the Web Animations DOM interfaces. Just like we have the Animation interface I assume we'll have an AnimationTrigger interface?

I opened an issue with a rough proposal here.
I could add that in already, as an initial proposal for API. I thought to wait with that for a later PR, since we only resolved on adding an initial draft and going with it back to the group for approval.

That also makes me wonder how authors would expect to be able to inspect these from script. Is there some way to fetch the triggers in a document? Or the trigger driving a particular Animation?

I missed that part, I guess I could add that in. Regarding additional APIs, like getting all triggers from document, I guess we could always add these later on demand?

Yeah, I was thinking that we need to review the whole section to check for other references that need to be updated like the sentence I mentioned and other definitions like this one:

An animation effect, effect, is associated with a timeline, timeline, if effect is associated with an animation which, in turn, is associated with timeline.

Presumably that needs to be updated to include triggers too?

I checked everything and didn't find anything else. I think since triggers only affect playback state and are associated with a timeline, it should be enough to check/update their state according to the timeline, since the trigger's spec describes what to do on state change.

This is a pretty fundamental change to the whole timing hierarchy so we need to be careful about it. I wonder if it would be better to introduce an abstract concept that covers both animations and animation triggers like "timeline subscribers" or somesuch.

Agreed. I tried simplifying it on the last change.

@flackr
Copy link
Contributor

flackr commented May 10, 2024

Ok, I also wondered about that, but @flackr wrote Web Animations 1, and some things I needed to update were actually there, so not sure whether this was intentional or not.

Yeah, that would be good to know. My understanding is it should be level 2 but maybe @flackr has some ideas about that.

Sorry if I created some confusion here. I was mostly getting at that the concepts that need updating are in the web-animations-1 spec, but of course additions to the spec should go in web-animations-2.

Copy link
Contributor

@flackr flackr left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on the review. Thanks for putting this together!

Canonical order: per grammar
</pre>

<span class=prod><dfn>&lt;single-animation-trigger-type></dfn> = once | repeat | alternate | state</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a description of what each property value means. Also we probably need a none value on this property to represent the default case right?

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 was pondering all the none/normal/auto options, and I think we currently don't need them, since the initial value is once, with a timeline value of auto, which I think is exactly what we have today.
We could also add an issue on that, in case it needs an approval from the group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The definition of the values is done here.
Should I add here another description? More user/less implementor facing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was pondering all the none/normal/auto options, and I think we currently don't need them, since the initial value is once, with a timeline value of auto, which I think is exactly what we have today.

Yeah, I like having the default values basically explain the triggering behavior we get today and adding the ability to modify it. It makes it feel like less of an added on property and more just part of the standard animation infrastructure.

We could also add an issue on that, in case it needs an approval from the group.

I think this can be part of the general review when it's all written up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you add a description of what each property value means.

Done

Animation Type: not animatable
</pre>

Issue: Probably a trigger with timeline "none" is under-specified here. Need to clarify what it means.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the default timeline is the document timeline set up in a way such that the "trigger" is always active explaining the normal behavior of animations? You could imagine then using this as a way to coordinate starting an animation after some length of time, e.g. animation-trigger-range: 10s. Admittedly this has some overlap with animation-delay, but unlike animation delay could use absolute times since page load or other events I suppose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I remember we considered this in a previous issue specifically on syncing animations, but we said there that this will be the stuff that will eventually be done via GroupEffects. That said, if you see some way of changing the definition here and keeping this more open for later extensions I'm all for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but unlike animation delay could use absolute times since page load or other events I suppose?

I feel like we'll still the pseudo-selector you suggested in #8175, for example, to defer until DOM and assets are loaded. This is pretty common.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove the none state, I'm not sure what it even means and the initial value is auto so it's not clear what you'd use none for. With the initial value auto, as you've said it has the same meaning as values of animation-timeline and as such would map to the document timeline. I think we should try to go with this model as it explains the existing behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but none here is part of the single-animation-timeline syntax. Should we remove it just here? Or entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.. I suppose suppose we can keep it and I guess the animation would never start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess it resets state back to idle.

css-animations-2/Overview.bs Show resolved Hide resolved
@@ -772,6 +772,9 @@ Animation Frames {#animation-frame-loop}
* Updating the [=animation/current time=] of any [=animations=]
[=associated with a timeline|associated with=] the timeline.

* Updating the [=animation trigger state=] of any [=animation trigger=]
[=associated with a timeline|associated with=] the timeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can move to web-animations-2 right? Also I think this needs to happen before updating the times of animations so that we can set their start time and know to make their effects active if they've been triggered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the animation-frame-loop definition exists only here, and I had to only add this one bullet. So what would be the proper way to add it to web-animations-2? Should I add just this delta somehow? Another way?

Regarding order, yes, I'll pull it up to happen before updating animations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we'd add a delta for this section to the ewb-animations-2 spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<dl class=switch>
: If |trigger|’s [=local time=] is [=unresolved=],
::
Then |state| is ''animation trigger state/idle'', and |did trigger| is set to false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't setting did trigger to false this reset once triggers anytime the timeline goes idle (e.g. anytime a ScrollTimeline's scroller becomes large enough that it doesn't scroll)? I think this is probably unexpected.

Note, it doesn't look like these links, e.g. ''animation trigger state/idle'' have linked to the definitions correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! I'm not sure whether this is unexpected. I guess we should open another issue for that.

Regarding the links, I'll check and fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't setting did trigger to false this reset once triggers anytime the timeline goes idle (e.g. anytime a ScrollTimeline's scroller becomes large enough that it doesn't scroll)? I think this is probably unexpected.

Gave this another thought. I think this should be expected, but definitely worth opening an issue and discuss this explicitly.

Note, it doesn't look like these links, e.g. ''animation trigger state/idle'' have linked to the definitions correctly.

Right, appears as though these aren't linked correctly, but I used the suggested syntax, so not sure what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

On re-reading this, I realized the did trigger flag actually says nothing about whether it will retrigger once animations, and the text the definition for once says:

The animation is triggered and played once and only once.

Which implies that once animations are not retriggered by this, which I think addresses my primary concern.

I was thinking about whether resizing the scroller / browser such that the area is no longer scrollable should be treated differently than exiting the exit range? My thinking is it shouldn't be any different. You by definition can't be in the exit range. With this in mind I'm not exactly sure what did trigger is trying to accomplish other than perhaps giving different behavior to being outside the active range to start, and going inactive. If this is the case, I think we should just consider the previous state rather than introduce this did trigger variable. E.g. if you're outside the active range and you have no previous state or your previous state was idle, you're still idle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The did trigger flag is used to differentiate between the initial idle state and the rest of the primary/inverse states going in/out of active interval. Mostly because you always want the initial state to be a non-triggered animation and the rest can be anything else defined by type: running/paused, played/reversed, etc.

I think it's OK for it to be reset on cases like timeline becoming inactive or if setting to a new trigger.
So not sure whether the once and only once is actually called for.


### Animation Trigger Ranges ### {#trigger-ranges}

An [=animation trigger=] has at least one [=animation attachment range|range=],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should define a new term for the trigger interval's active range, as the combination of the two ranges would probably be worth eventually having a diagram to illustrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll try adding that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


The ''inverse'' behavior is [=reverse an animation|reversing=] the associated animation.

<dt><dfn>state</dfn>
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if something like play-pause would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's another option. The idea behind state was a reference to play-state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I could see play-state as an alternative. State feels too strange, I didn't have any idea what this meant until I read the description :-).

@ydaniv
Copy link
Contributor Author

ydaniv commented Jun 6, 2024

@flackr finished addressing all comments from last review

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I6ac50730653e70219775895c315f91f771ea7c13
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I6ac50730653e70219775895c315f91f771ea7c13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6182723
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409620}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I6ac50730653e70219775895c315f91f771ea7c13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6182723
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409620}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I6ac50730653e70219775895c315f91f771ea7c13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6182723
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409620}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I2c335d6be7b6e264fb1cbf4ae4cc3fa9aedc9136
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I2c335d6be7b6e264fb1cbf4ae4cc3fa9aedc9136
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I2c335d6be7b6e264fb1cbf4ae4cc3fa9aedc9136
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6187119
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Commit-Queue: David Awogbemila <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409968}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I2c335d6be7b6e264fb1cbf4ae4cc3fa9aedc9136
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6187119
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Commit-Queue: David Awogbemila <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409968}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2025
The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I2c335d6be7b6e264fb1cbf4ae4cc3fa9aedc9136
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6187119
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Commit-Queue: David Awogbemila <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409968}
Added AnimationTrigger to Animation, Animatable mixin, and KeyframeAnimationOptions interfaces
@ydaniv
Copy link
Contributor Author

ydaniv commented Jan 26, 2025

@flackr just pushed fixes for the last round of reviews, I think it's ready for another check.

Comment on lines +2404 to +2406
: Otherwise,
::
The ''primary'' behavior is [=reverse an animation|reversing=] the associated animation.
Copy link
Contributor

Choose a reason for hiding this comment

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

A little curious about how this works. Am I correct that for animation-trigger-type: alternate the animation should be played when entering the animation-trigger-range and reversed when exiting the animation-trigger-exit-range? If so, it seems like the behavior defined for alternate here could result in reversing the animation while we're inside the exit range?

Let me try to use an example and perhaps you can point out what I've misunderstood. Say we have:

ainmation-trigger: view() alternate cover 0% cover 100%;

and lets say cover 0% and cover 100% correspond to 100px and 200px respectively.
So the default range and the exit range are both [100, 200].
The page loads, scrollTop= 0, the trigger's state is idle (active interval is [100,200]).
Then we scroll to 150px, so we set the trigger's state to primary (the active interval is now the exit range, still [100, 200]. since did trigger is false we play the animation) and set did trigger to true.
We then scroll to 160px and the animation trigger's state is updated: since we are still inside the active interval,
we set the state to primary (since did trigger is true, we reverse the animation) and set did trigger to true.
So we would have reversed the animation despite not having exited the exit range?
Or perhaps I'm not quite correct about when the animation trigger gets updated?
It seems like we don't need the if/else and the alternate behavior should just be primary->trigger, inverse->reverse?
I might also have missed something about did trigger... it seems the algorithm is given a flag did trigger, but also defines a did trigger var, and the animation trigger also has an internal did trigger, so maybe I should clarify how those are related.

Copy link
Contributor Author

@ydaniv ydaniv Jan 27, 2025

Choose a reason for hiding this comment

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

Yeah, this one is a bit tricky. The idea is that if did trigger is false then we play "forwards". Then, on every state change we reverse. The problem, as you described it, is that state change will occur on every scroll change. But my internet was that a state change occurs only when the active attachment range is either entered or exited.
So the way I intend for it behave is to not change state until the range is exited. Then reverse on exit, and reverse back on re-enter, and so on

Does this make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I might not be totally clear on the last line:

Then reverse on exit, and reverse back on re-enter, and so on

I understand that we would reverse the animation on exit (exiting the exit range), but on re-enter (re-entering the trigger range) we would just play the animation forwards again right? is this what you mean by "reverse back"?

If that's right, what do you think about simplifying the primary behavior for alternate? I think that simply having:

'alternate'
   The primary behavior is triggering the associated animation. The inverse behavior is reversing the associated animation.

fixes the issue of reversing on every scroll?

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 understand that we would reverse the animation on exit (exiting the exit range), but on re-enter (re-entering the trigger range) we would just play the animation forwards again right? is this what you mean by "reverse back"?

What I mean is that after reversing an animation its effective playback rate remains inveresed, so you'll first need to inverse the playback rate, and then invoke the play an animation procedure, which is exactly the reversing an animation procedure.

fixes the issue of reversing on every scroll?

My issue with "every scroll" is that I'd just like to clarify that we don't do anything on every scroll change. Even though we're using Scroll and View timelines here, the changes are singular on range entry/exit, and don't occur repeatedly on progress change, like SDA's do. Otherwise we would need to re-trigger primary on each scroll change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the "reverse" on re-enter is the forwards direction, however, it is worth noting that it might not start from the beginning of the animation if the reverse animation hadn't finished. E.g. if you temporarily move out of the exit range and then back into the trigger range you may only play from backwards from 100% back to 80%, then reverse back to 100%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course. This is the precise intent, just like flipping a transition mid-way.

1. Set |state| to ''animation trigger state/primary''.
1. Set |did trigger| to true.

: Otherwise,
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to my comment above, would we have an issue if we have non-overlapping trigger and exit ranges? One use case I can think of for non-overlapping ranges is if you wanted a repeat trigger but in only one scrolling direction. E.g. you only want the animation to trigger scrolling downwards (item translates upwards into view). In this case you'd define the exit range and the default range as, e.g. [0, 100] and [150, infinity] so you can only reset the animation by scrolling back up by a sufficient amount. As currently written it looks like this algorithm will probably reset the animation shortly after we trigger it:

  • enter the trigger range, e.g. 160px
    • plays the animation, state becomes primary -> exit range [0, 100] becomes active interval
  • further scrolling (still outside exit range, e.g. 170px)
    • animation is reset.
      ?

One way we could address this would be to define two types of (active) intervals, an entry interval and an exit interval and have the trigger keep track of whether, at the last update it was within the interval within active interval defaulting to false. Then the algorithm can decide whether to apply the primary or inverse behavior based on whether we are entering or exiting the relevant interval.

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'm a little confused here.
Let's first see we're on the same page about the exit range:
Once the user is inside the default range, and primary behavior is triggered, the active range is immediately switched to the exit range. And only once you exit that range, the state switches again.
If we allow exit ranges that are "smaller" then their corresponding default ranges then we could be allowing endless loops or other weird behaviors. So I suggested as an issue in the PR to perhaps restrict exit ranges to only be equal or larger than default ranges, and that they can not constrict them, see here.

I can't think now about any interesting effect one could do with a non-overlapping exit range, so I suggest avoiding it, just like we enforce range start is not larger than range end, or the timeline becomes inactive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thanks for adding that as an issue in the spec. I guess it (restricting valid values of the exit range to be "equal to or greater than" the trigger range) is something the working group will want to consider and we can file an issue for it after this PR has landed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think to make things simple we may just want to spec that being in the trigger range also counts as being in the exit range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, they both define the active range. I'm not following. Could you be more specific technically what would that mean in the spec?

Copy link
Contributor

@DavMila DavMila Jan 30, 2025

Choose a reason for hiding this comment

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

I think to make things simple we may just want to spec that being in the trigger range also counts as being in the exit range.

Hmm, could you elaborate? I'm not sure that will fully address the concern about how the current PR will handle non-overlapping ranges, particularly if I changed the entry range from [150, infinity] to [150, 200] and scrolling beyond 200px is possible (ignoring that that now makes it possible to trigger the animation by scrolling either up or down).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think to make things simple we may just want to spec that being in the trigger range also counts as being in the exit range.

Hmm, could you elaborate? I'm not sure that will fully address the concern about how the current PR will handle non-overlapping ranges, particularly if I changed the entry range from [150, infinity] to [150, 200] and scrolling beyond 200px is possible (ignoring that that now makes it possible to trigger the animation by scrolling either up or down).

Ah.. but maybe it's not such a valid use case anymore if you shrink the entry range - you can trigger it scrolling up or down, but you can only reset it scrolling up far enough and then scrolling down again :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ydaniv I haven't re-reviewed the specific algorithm, but the idea is we wouldn't enter the state/inverse state if you're in the primary trigger range - which means it doesn't matter if your exit range is defined to be inclusive of it or not, the entry range is by definition part of it.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 27, 2025
…range-*, a=testonly

Automatic update from web-platform-tests
Implement parsing for animation-trigger-range-*

The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I6ac50730653e70219775895c315f91f771ea7c13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6182723
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409620}

--

wpt-commits: 78c349f14e31d5bbba8a109e1529ee7cb94dd0bf
wpt-pr: 50216
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 27, 2025
…{exit-}range shorthands, a=testonly

Automatic update from web-platform-tests
Implement parsing for animation-trigger-{exit-}range shorthands

The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I2c335d6be7b6e264fb1cbf4ae4cc3fa9aedc9136
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6187119
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Commit-Queue: David Awogbemila <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409968}

--

wpt-commits: eb78c18f83a97a696c4293facc05c7b877b49fd6
wpt-pr: 50225
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jan 28, 2025
…range-*, a=testonly

Automatic update from web-platform-tests
Implement parsing for animation-trigger-range-*

The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I6ac50730653e70219775895c315f91f771ea7c13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6182723
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409620}

--

wpt-commits: 78c349f14e31d5bbba8a109e1529ee7cb94dd0bf
wpt-pr: 50216
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jan 28, 2025
…{exit-}range shorthands, a=testonly

Automatic update from web-platform-tests
Implement parsing for animation-trigger-{exit-}range shorthands

The spec is currently a work-in-progress[1] but this comment[2]
describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I2c335d6be7b6e264fb1cbf4ae4cc3fa9aedc9136
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6187119
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Commit-Queue: David Awogbemila <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409968}

--

wpt-commits: eb78c18f83a97a696c4293facc05c7b877b49fd6
wpt-pr: 50225
1. Set |state| as follows:
<dl class=switch>
: If |trigger|’s [=local time=] is [=unresolved=],
::
1. Set |state| to ''animation trigger state/idle''.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to count this as not being inside the trigger range, i.e. switching to state/inverse if we've triggered? It might be a bit odd that you stay in the triggered state when the timeline is inactive.

Copy link
Contributor

@flackr flackr left a comment

Choose a reason for hiding this comment

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

I think this is looking good. There are still a few details to work out but overall I think the general algorithm should work pretty well and I don't want to delay getting the initial spec in :-)

@ydaniv ydaniv merged commit 05b336a into w3c:main Jan 31, 2025
1 check passed
@ydaniv ydaniv deleted the animation-trigger branch January 31, 2025 00:17
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 31, 2025
For reference: the spec is currently a work-in-progress[1] but this
comment[2] describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I3b273f4565efddd2fd48d407102f9f1376d816dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6207949
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1414034}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2025
For reference: the spec is currently a work-in-progress[1] but this
comment[2] describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I3b273f4565efddd2fd48d407102f9f1376d816dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6207949
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1414034}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2025
For reference: the spec is currently a work-in-progress[1] but this
comment[2] describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I3b273f4565efddd2fd48d407102f9f1376d816dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6207949
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1414034}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 2, 2025
…tonly

Automatic update from web-platform-tests
Parse animation-trigger shorthand

For reference: the spec is currently a work-in-progress[1] but this
comment[2] describes the API.

[1] w3c/csswg-drafts#10128
[2] w3c/csswg-drafts#8942 (comment)

Bug: 390314945
Change-Id: I3b273f4565efddd2fd48d407102f9f1376d816dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6207949
Commit-Queue: David Awogbemila <[email protected]>
Reviewed-by: Anders Hartvoll Ruud <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1414034}

--

wpt-commits: 52bce37bd96221c1fc5a60da383faef154724895
wpt-pr: 50409
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants