-
Notifications
You must be signed in to change notification settings - Fork 312
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
fix animation reversing #620
base: master
Are you sure you want to change the base?
fix animation reversing #620
Conversation
Hi, and thanks for the PR! Can you elaborate a bit more. What is the buggy behavior you mention? How does this PR change the current behavior? Looks like this factor was introduced all the way back in 23bdc17, it's a while ago, but looks like it's related to implementation of the CSS spec reverse shortening factor. |
Before: Reverse animation duration is not shortened properly. After: The reverse transition's duration is properly shortened based on how far in the animation the previous transition was. Re-reading the spec, I'm certain the reverse shortening factor related stuff was not implemented correctly. https://www.w3.org/TR/css-transitions-1/#ref-for-running-transition%E2%91%A5 I can't quite parse all the jargon in there, but the "After" animations definitely look better to my eyes. Seems that there should also be a check that makes sure that the new transition's ending state matches the old transition's starting state. |
Right, thanks for elaborating on the motivation and the screenshots. Yeah, I see this change here is essentially equivalent to setting the factor to One issue seems to be that we don't keep track of the factor, thus leading the duration to be compressed for each successive reversal. This actually seems to be a visible issue in practice. E.g. try a hover transition to around 50% completion, then quickly move in and out of it, then let it complete. It looks quite buggy. I see that the spec tries to account for this, I think we should do the same. Another thing, I notice the factor can go outside the [0, 1] range (e.g. with exponential-out interpolation). Just like in the spec, we should clamp it to the [0, 1] range. This should be a quick change though. |
The spec is making sense the more I let my brain sit on it and all those changes sound like good ideas. I'm already planning out how to add all that stuff in. |
8d9fbb2
to
c64885c
Compare
b5d337e
to
68ff4b4
Compare
First of all, thanks a lot! This looks like a great effort, and it's nice if we can follow the standard a bit better in some of these corner cases. Can you tell a bit more about the observable changes we get from this? We are currently in a stabilization phase towards RmlUi 6.0. There are quite a few changes here, and some issues can easily creep in from experience. So we will have to merge this one after release, I'll see if I can find some time to review it in the meantime. |
Sorry for the late response.
The main thing is that animation reversing (and reversing the reversing repeatedly, and reversing that..., etc) does not result in strange behaviours / visual glitches. In general, I've just been trying to follow the CSS spec as much as possible. |
fix animation reversing by removing undocumented and buggy "reverse_adjustment_factor"