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] separate animation-range and animation-delay #7901 #8219

Merged
merged 1 commit into from Feb 6, 2023

Conversation

fantasai
Copy link
Collaborator

No description provided.

@fantasai fantasai requested a review from flackr December 13, 2022 20:50
@@ -736,67 +736,79 @@ spec: cssom-view-1; type: dfn;

A set of animation keyframes can be attached
in reference to [=named timeline ranges=]
by adjusting the duration of the animation.
by restricting the to the specified timeline range.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "the to the" I think is meant to be "them"?

Though this isn't quite correct. The keyframes can be whenever they want to be - but the animation start and end are restricted to the range. It is valid to make an animation with keyframes that are outside of the range, e.g.

@keyframes anim {
  cover 0% { opacity: 0; }
  contain 100% { opacity: 1; }
}
.target {
  animation: anim;
  animation-timeline: view();
  animation-range: contain;
}

This will result in an animation which when the element enters the contain phase starts at some percentage which depends on its size.

animation-range (further insetted by delay) is used to define the start and end of the active phase of the animation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, updated terminology. Let me know if I got it right yet. (I also inserted a diagram.)

are set within this restricted range.
If a duration ('animation-duration') is given,
it measures from the range-restricted start,
and further restricts the range-restricted end.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is just used instead rather than replacing it, isn't it? E.g. similar to how width is just used instead of right for absolute positioning regardless of whether that further restricts or expands the width: https://jsbin.com/bekiceg/edit?html,css,output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would make more sense to take the more restrictive one, since if you set a shorter animation-range-end and a longer duration, it'd seem weird to ignore the animation-range-end?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by the interaction with duration here. Ranges relate to ViewTimeines, and these don't have a specified duration (unless we specify interaction with animation-iteration-count) but rather a duration of auto or 100%, right?
So unless there's some sort of animation-iteration-count: infinite/auto i'd expect animation-duration to be ignored and be calculated from ranges and delays.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ydaniv That's another approach we could take: if animation-range-end has a valid non-normal value, ignore the duration. (It seemed the discussion in an earlier issue wanted them to both apply somehow.)

Copy link
Contributor

@ydaniv ydaniv Dec 22, 2022

Choose a reason for hiding this comment

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

Yes, I thought that's the main use-case, so you're saying range-end could be normal and duration in <length> can determine the actual duration, staring from the range-start?
SGTM, I'd also prefer to respect the range-end and clamp to that

Copy link
Contributor

Choose a reason for hiding this comment

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

So taking the shorter of <range-end> - <range-start> and <duration> still isn't really honoring the request:

  • If the range is shorter, you're not using the specified duration
  • If the duration is shorter, the animation doesn't end at range-end.

If we don't need to follow the precedent for box positioning, I'd suggest that when a start and end range are both not normal we use that instead of the duration.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I'll open a new issue about this. -> #8405

scroll-animations-1/Overview.bs Outdated Show resolved Hide resolved
scroll-animations-1/Overview.bs Show resolved Hide resolved
scroll-animations-1/Overview.bs Outdated Show resolved Hide resolved
i.e. a ''2s'' delay effectively shortens the animation duration,
whereas ''-2s'' lengthens it.
Shifts the end of the animation
(i.e. when keyframes attached to 100% progress are mapped)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true if iteration count = 1 and end delay is 0, but if iteration count > 1 there will be multiple points at which 100% is reached and if end delay is > 0 100% will be reached before animation-range-end. This is the end time of the animation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I think I fixed this.

<dl dfn-for="animation-range-start, animation-range-end" dfn-type=value>
<dt><dfn>auto</dfn>
<dd>
The end of the animation attaches to the end of the animation duration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as for range-start.

@fantasai fantasai force-pushed the animation-range branch 2 times, most recently from 1253065 to aa6b2f6 Compare December 15, 2022 21:14
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. I don't know why but this review doesn't show up in PR's awaiting my review (https://github.com/w3c/csswg-drafts/pulls?q=is%3Apr+is%3Aopen+user-review-requested%3A%40me) so i have to search for it every time.

Overall I think this is looking really close now and if you want to move my particular comments to followup issues that's fine with me.

are set within this restricted range.
If a duration ('animation-duration') is given,
it measures from the range-restricted start,
and further restricts the range-restricted end.
Copy link
Contributor

Choose a reason for hiding this comment

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

So taking the shorter of <range-end> - <range-start> and <duration> still isn't really honoring the request:

  • If the range is shorter, you're not using the specified duration
  • If the duration is shorter, the animation doesn't end at range-end.

If we don't need to follow the precedent for box positioning, I'd suggest that when a start and end range are both not normal we use that instead of the duration.

scroll-animations-1/Overview.bs Outdated Show resolved Hide resolved
<dd>
Defines the delay as a duration. See [[CSS-ANIMATIONS-1]].
The start of the [=active phase=]
is determined as normal.
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer auto as it seems more in line with how implicitly computed defaults are specified elsewhere and aligns with animation-duration: auto.

I believe how we determine the start time of the animation should be updated in web-animations-2 3.3.7. Playing an animation to account for range start and end. This currently explains how the "normal" behavior gets a 0 start time for scroll driven animations and the current timeline time for time-based animations. The logic there should be updated to account for range start and range end and then the css-animations spec can refer to that procedure to play the animation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went with normal because (unlike duration) we aren't magically computing a range: if the range is not otherwise defined, the timeline range is the whole timeline, as normal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Filed as #8406

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

Successfully merging this pull request may close these issues.

None yet

3 participants