Page MenuHomePhabricator

Bug 1805727 - Part 5: Support transition-behavior when animation values fall back to discrete.
ClosedPublic

Authored by boris on Feb 14 2024, 8:22 PM.
Referenced Files
Unknown Object (File)
Mon, May 4, 9:27 PM
Unknown Object (File)
Mon, May 4, 9:04 PM
Unknown Object (File)
Sun, May 3, 7:45 PM
Unknown Object (File)
Sun, May 3, 6:38 PM
Unknown Object (File)
Sun, May 3, 12:44 AM
Unknown Object (File)
Sat, May 2, 4:15 PM
Unknown Object (File)
Wed, Apr 29, 6:08 AM
Unknown Object (File)
Sun, Apr 26, 12:28 PM
Subscribers

Details

Summary

When transition-behavior is allow-discrete, the animation values are
transitionable even if they are not interpoltable, given that the
animation type of the CSS property is by computed value.

Also, we remove animate() check from needs_transitions_update_per_property.
This check was added for handling the transition between auto and
other values long time ago, but now it may be redundant (because we
still pass the tests without it) and we do the same things in
nsTransitionManager as well, so it should be fine to drop it, especially
after we support discrete transitions.

Event Timeline

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
emilio requested changes to this revision.Feb 19 2024, 10:28 AM

Maybe we can make this simpler, wdyt? If you think the animate() check is worth the optimization lmk, but per https://bugzilla.mozilla.org/show_bug.cgi?id=1876263#c1 it might not be worth it actually...

servo/components/style/gecko/wrapper.rs
867

This should probably be called might_allow_discrete

895

we might want to just remove the animate() check altogether, I think it's only an optimization in practice, and it might not be worth it, what do you think?

servo/ports/geckolib/glue.rs
1257–1258

Let's do the behavior check first since it should be cheaper?

This revision now requires changes to proceed.Feb 19 2024, 10:28 AM
servo/components/style/gecko/wrapper.rs
895

It seems we added animate() in https://github.com/servo/servo/pull/18081. (note: it used interpolate() but I think they are similar.)

However, I ran the mochitest (test_transitions_per_property.html) mentioned in the bug and it was passed. I guess we landed a lot of improvement for discrete after this bug, so we may not hit this issue anymore.

I'm still waiting for the try result, and it's likely to pass all the tests without animate() check here. Once everything looks good, I'd like to remove this altogether.

boris marked 3 inline comments as done.

Addressed comments

emilio added a project: testing-approved.
emilio added inline comments.
servo/components/style/gecko/wrapper.rs
890

Bonus points for filing a follow-up to do this check in a faster way (since extracting the animation values is not super-trivial). We should be able to do something like !before_change_style.computed_value_equals(after_change_style, property_declaration_id) or such. Anyways, looks good for now of course.

894

Probably let's move the combined duration check before the extraction of animation values, since that's more expensive, so before let from =:

if combined_duration_seconds <= 0.0f32 {
    return false;
}
This revision is now accepted and ready to land.Feb 23 2024, 9:11 PM
boris marked an inline comment as done.

Addressed comments

boris added inline comments.
servo/components/style/gecko/wrapper.rs
890
boris marked an inline comment as done.

Rebased