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] End < Start offset can cause the ScrollTimeline to produce negative time #4351

Closed
majido opened this issue Sep 26, 2018 · 5 comments

Comments

@majido
Copy link
Contributor

majido commented Sep 26, 2018

The spec does not require start and end offset to have a strict ordering so it is possible to define
start offset that is larger than end offset. This combined with how the current time is computed can lead to a negative current time.

(current scroll offset - startScrollOffset) / (endScrollOffset - startScrollOffset) × effective time range

A similar concern exists for timeRange which can also be negative.

Is this intentional or should there be more constraints over these values so that currentTime is always guaranteed to be a positive values?

@birtles
Copy link
Contributor

birtles commented Sep 27, 2018

A negative current time is not a problem per se but I'm not sure if it was intentional.

@dontcallmedom dontcallmedom transferred this issue from WICG/scroll-animations Sep 19, 2019
@majido majido added the scroll-animations-1 Current Work label Sep 19, 2019
@birtles birtles changed the title End < Start offset can cause the ScrollTimeline to produce negative time [scroll-animations] End < Start offset can cause the ScrollTimeline to produce negative time Oct 7, 2019
@majido
Copy link
Contributor Author

majido commented Mar 31, 2020

I suggest we restrict this by requiring the end be larger or equal to start otherwise the timeline would return an unresolved time.

BTW today there was a proposal to have an array of scroll offsets and not just two in which case we should sort such offsets before using them to compute the current time. This will also solve this issue.

@birtles
Copy link
Contributor

birtles commented Apr 9, 2020

Yes, sorting might make more sense. At least String.substring() does that. I'm not sure if it's a common JS pattern, however.

@ogerchikov
Copy link
Collaborator

Sorting solves the problem for container based offsets. If offsets are element based, the elements can be rearranged within the scroller post sorting and, as a result, produce negative time.

@flackr
Copy link
Contributor

flackr commented Aug 11, 2022

I don't think there is a problem with negative times. However, if the start offset is greater than the end offset, the common case will be that the start offset is also greater than the current scroll offset (e.g. this is the case with RTL text) and the calculation will result in a positive time.

We have chosen not to clamp time values as it allows the animation effect's fill behavior to properly dictate what the effect output should be negating the need for phases.

@flackr flackr closed this as completed Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants