If we have, for example, a scale animation that is overridden by an !important rule, then we should not run any transform or rotate etc. animations on the compositor since we'll be unable to compose the final result correctly on the compositor (since we don't have the final value of scale there).

Currently we implement this in KeyframeEffect::HasEffectiveAnimationOfPropertySet but that seems like the wrong place.

It means that, for example, that nsLayoutUtils::GetAnimationPropertiesForCompositor will fail to apply that restriction. It also makes it awkward to report a performance warning for this case, even once bug 1533594 is fixed.

We really should move this check to KeyframeEffect::ShouldBlockAsyncTransformAnimations or some such.

I just checked the performance warning of this case [1] (i.e. overridden by an !important rule), and now what I got is: AnimationPerformanceWarning::Type::TransformFrameInactive [2]. That means, currently, both IsTransformMaybeAnimated() and HasAnimationsForCompositor() check the important rules. and if both are false (due to the !important rules), we will use this performance warning (i.e. TransformFrameInactive). It's not correct, I think, so we should move the check out of
KeyframeEffect::HasEffectiveAnimationOfPropertySet, as comment 0 mentioned.

In order to get the correct performance warning, I think it is necessary to move this out of HasEffectiveAnimationOfPropertySet, so I decide not to add the new performance warning (e.g. AnimationPerformanceWarning::Type::TransformIsBlockedByOtherTransformLikeProperties) in bug 1533594. Instead, we should add it in this bug after refactoring them.

[1] https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/dom/animation/test/chrome/test_running_on_compositor.html#1137-1140
[2] https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/layout/painting/nsDisplayList.cpp#7779-7787

Yes, I agree. I think it will be simpler to make bug 1533594 just focus on refactoring the existing warnings. In this bug we can add a separate warning as part of moving the check to KeyframeEffect::ShouldBlockAsyncTransformAnimations or somewhere similar.

I just reverted the change in KeyframeEffect::HasEffectiveAnimationOfPropertySet() (i.e. Drop the transform check), and we still fall back to main thread if we have an !important rule because EffectCompositor::FindAnimationForCompositor() also checks the important rules [1].

In other words, it seems we don't really need to move it into KeyframeEffect::ShouldBlockAsyncTransformAnimations(). In EffectCompositor::FindAnimationForCompositor(), the check of important rules [1] is prior to KeyframeEffect::IsMatchForCompositor() [2], which calls KeyframeEffect::ShouldBlockAsyncTransformAnimations().

In conclusion, perhaps what we have to do is:

1. We just remove the check of transform from KeyframeEffect::HasEffectiveAnimationOfPropertySet()
2. Add the warning and set it in EffectCompositor::FindAnimationForCompositor(), around [1] (for transform only)

[1] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/animation/EffectCompositor.cpp#141-147
[2] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/animation/EffectCompositor.cpp#180-181

I'm not sure we would want to do the check directly in FindAnimationsForCompositor would we? That seems like the wrong place since all the checks for whether things should run on the compositor happen in IsMatchForCompositor. If we do this somewhere inside IsMatchForCompositor (and ShouldBlockAsyncTransformAnimations feels like the right place based on its name) then KeyframeEffect::GetPropertiesForCompositor should also return the correct result. Does that sound right?

(In reply to Brian Birtles (:birtles) from comment #4)

I'm not sure we would want to do the check directly in FindAnimationsForCompositor would we? That seems like the wrong place since all the checks for whether things should run on the compositor happen in IsMatchForCompositor. If we do this somewhere inside IsMatchForCompositor (and ShouldBlockAsyncTransformAnimations feels like the right place based on its name) then KeyframeEffect::GetPropertiesForCompositor should also return the correct result. Does that sound right?

Ya. This makes senses. For now, [1] doesn't return any performance warning, so moving it into ShouldBlockAsyncTransformAnimations would be better to me. However, this might be a little bit tricky to make KeyframeEffect::GetPropertiesForCompositor also return the correct result (especially for transform-like property set) because KeyframeEffect::GetPropertiesForCompositor() calls IsMatchForCompositor() on each single property, instead of a property set. Need to do some minor refactoring in this function. (FindAnimationsForCompositor calls IsMatchForCompositor() on the entire property set, so should be simpler.)

[1] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/animation/EffectCompositor.cpp#141-147

We move the check of important rule and animation level into
IsMatchForCompositor(), and add a new warning for it.

So now, all transform-like properties should be passed all together into
IsMatchForCompositor() because they depends on each others.

https://hg.mozilla.org/mozilla-central/rev/9f687685a13c

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.