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] Clean build warnings and remove obsolete hold phase #7487

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

kevers-google
Copy link
Contributor

@kevers-google kevers-google commented Jul 11, 2022

[web-animations-2]: Cleanup all build warnings in prep for transition to editor's draft. As before and after phases for timelines have been deprecated, these are also removed as part of the cleanup.

@birtles
Copy link
Contributor

birtles commented Jul 11, 2022

Thanks for doing this. Just checking, but is the hold phase part of the timeline phase removal?

And the addition of the unlimited current time to the definition of at progress timeline boundary seems new. Is this intended?

@kevers-google
Copy link
Contributor Author

With Timeline before/after phase being removed from web-animations-1, the hold phase is irrelevant. Rather than making animation.currentTime unresolved when in the timeline before/after phase, we now allow timeline.currentTime to extend beyond the range of 0 to 100%, and let the animation effect phase take care of the rest. This plays nicely with animation delays and is a simpler solution.

Issue tracked here: #7240

The "unlimited current time" for the "at progress timeline boundary" calculation is new and intentional. the problem is that a finished animation has a start time and a hold time so the current time is clamped at end time and it appears that the timeline is at the boundary when it fact the in the after phase. This problem was previously mitigated by considering the timeline phase, but once the timeline phase is removed this safeguard becomes necessary.

I'm still not overly keen on the method for detecting if at a boundary. The current method is trying to be overly clever and accommodate the playback rate. It might simply be better to detect if the timeline is at 0 or 100% and leave it at that. If the playback rate is not 1, then these boundaries simply won't align with the animation effect boundary, which seems fine. Having said that, I'd like to leave the choice of algorithm for a later discussion. The fix here was aimed at making the current algorithm work with the removal of timeline phase.

@birtles
Copy link
Contributor

birtles commented Jul 13, 2022

Thanks for the very detailed explanation. That looks really great.

I'm not sure why the ipr check is stuck or even how to debug that. I guess we need @flackr's review before merging anyway.

@flackr
Copy link
Contributor

flackr commented Jul 13, 2022

Thanks Kevin, the hold phase removal and delint is great!

@flackr flackr changed the title [web-animations-2]: Delint [web-animations-2] Clean build warnings and remove obsolete hold phase Jul 13, 2022
@birtles
Copy link
Contributor

birtles commented Jul 14, 2022

So I get:

image

It appears to be stuck? Or is there something you need to do at your end @kevers-google? I'm pretty sure you've submitted PRs before so presumably you have the necessary IPR clearance?

@kevers-google
Copy link
Contributor Author

I see the same. I have not encountered this problem previously with PRs.

@tabatkins tabatkins merged commit 80bc3d5 into w3c:main Jul 14, 2022
@tabatkins
Copy link
Member

k, merged with admin privilege since the bot was borked ^_^

tabatkins added a commit that referenced this pull request Oct 19, 2022
…o just make the query invalid, not the function. #7487
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.

4 participants