1279403 - Opacity animation is not sent to the compositor when changing the target element if the animation on a 100% opacity keyframe

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: An automation test

I could finally write a test case to prove that there is a case we need a paint trigger to send animations to the compositor when changing the target element.  Or, as Brian noted to me in bug 1223658 in comment 137, we need to tweak 
ElementRestyler::AddLayerChangesForAnimation (with tweaks to skip optimizations in DoApplyRenderingChangeToTree)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I've decided that I will handle setTarget/setKeyframes cases here and handle fill:forwards or paused or running on the main-thread on a transform:none or 100% opacity segment in bug 1278136, and handle delay phase cases in bug 1223658.  As a result, the part 5 patch in bug 1223658 will be much simpler.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1279403 - Part 1: Force to apply corresponding change hint if there is no corresponding layer to generate display item even if animation's segment is transform:none or 100% opacity.

To create a stacking context for animations on transform:none segment,
we need to set NS_FRAME_MAY_BE_TRANSFORMED.  The fix is comming in part 2.

Note that in case of animations which has properties preventing running on
the compositor, e.g., width or height, corresponding layer is not created
at all, but even in such cases, we normally set valid change hint for such
animations in each tick, i.e. restyles in each tick. For example:

div.animate([{ opacity: 1, width: '100px' }, { opacity: 0, width: '200px' }], 1000);

This animation causes restyles in every ticks without this patch, this patch
does not affect such animations at all. The only animations which will be
affected by this patch is that animations which has opacity/transform but
did not have those properies. e.g,  setting transform by setKeyframes or
changing target element from other target which prevents running on the
compositor, etc.

Review commit: https://reviewboard.mozilla.org/r/59648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59648/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1279403 - Part 2: Set NS_FRAME_MAY_BE_TRANSFORMED bit if the target nsIFrame has transform when setting target or keyframes.

Review commit: https://reviewboard.mozilla.org/r/59650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59650/

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8763449 [details]
Bug 1279403 - Part 1: Force to apply corresponding change hint if there is no corresponding layer to generate display item even if animation's segment is transform:none or 100% opacity.

https://reviewboard.mozilla.org/r/59648/#review57556

This looks good but I want to check the updated comment for AddLayerChangesForAnimation().

::: dom/animation/test/chrome/test_running_on_compositor.html:453
(Diff revision 1)
> +                               { opacity: 1, offset: 0.99 },
> +                               { opacity: 0, offset: 1 }], 100 * MS_PER_SEC);

I'm curious, if we simply had [{ opacity: 1, offset: 0 }, { opacity: 1, offset: 1 }] would this test case still be valid? Or is there some optimization that means we would skip such an animation?

It seems like in the reftests you use { opacity: [1, 1] } so I'm curious why we can't do that here.

::: dom/animation/test/chrome/test_running_on_compositor.html:471
(Diff revision 1)
> +}, 'opacity animation on a 100% opacity keyframe keeps running on the ' +
> +   'compositor after changing the target element');

'100% opacity animation with keeps... ' ?

::: dom/animation/test/chrome/test_running_on_compositor.html:489
(Diff revision 1)
> +                  '100% opacity animation switched from property which is ' +
> +                  'not running on the compositor is running on the compositor');

How about, '100% opacity animation set by using setKeyframes reports that it is running on the compositor' ?

::: dom/animation/test/chrome/test_running_on_compositor.html:492
(Diff revision 1)
> +}, '100% opacity animation switched from property which can not be running on ' +
> +   'the compositor is running on the compositor');

'100% opacity animation set up by converting an existing animation with cannot be run on the compositor, is run on the compositor' ?

::: layout/base/RestyleManager.cpp:2758
(Diff revision 1)
> +    // We consider it's the first paint for the frame if we have an animation
> +    // for the property but have no layer.

I think you need to move some of the commit message comment here. Anyone reading this code is likely to think,

"Ok, so we don't have a layer, but we do have an animation for the property. What if some other condition prevented us from running the animation on the compositor? Why is it ok to set the change hint in that case?"

I think your commit message comment covers that but I think it needs to go here instead.

::: layout/reftests/web-animations/stacking-context-animation-changing-target-ref.html:2
(Diff revision 1)
> +<!DOCTYPE html>
> +<title>Reference of testcases for bug 1223658</title>

Is this the right bug number?

::: layout/reftests/web-animations/stacking-context-opacity-changing-target.html:4
(Diff revision 1)
> +<!DOCTYPE html>
> +<html class="reftest-wait">
> +<title>
> +Opacity animation creates a stacking context when changing its target

Did we check that this test fails without the changes in this patch?

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8763450 [details]
Bug 1279403 - Part 2: Set NS_FRAME_MAY_BE_TRANSFORMED bit if the target nsIFrame has transform when setting target or keyframes.

https://reviewboard.mozilla.org/r/59650/#review57554

This also looks good, but I wonder about the display:none question?

::: dom/animation/KeyframeEffect.h:388
(Diff revision 1)
> +  // Update the associated frame state to send the effect to the compositor
> +  // or create a stacking context if necessary.

Perhaps something like:

  Update the associated frame state bits so that, if necessary, a stacking context will be created and the effect sent to the compositor. We typically need to do this when the properties referenced by the keyframe has changed, or when the target frame might have changed.

And, writing this out, I now wonder, does this work for an animation when we initially don't have a frame? e.g. If we create an animation on a display:none element and then later set it to display:block?

::: dom/animation/KeyframeEffect.cpp:1379
(Diff revision 1)
> +  // NS_FRAME_MAY_BE_TRANSFORMED flag should be removed when the animation
> +  // will be removed from effect set or the transform keyframes are removed
> +  // by setKeyframes. The latter case will be hard to solve though.
> +  for (const AnimationProperty& property : mProperties) {
> +    if (property.mProperty == eCSSProperty_transform) {
> +      // Set NS_FRAME_MAY_BE_TRANSFORMED on the associated frame.

I don't think we need this (one-line) comment.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> ::: dom/animation/KeyframeEffect.h:388
> (Diff revision 1)
> > +  // Update the associated frame state to send the effect to the compositor
> > +  // or create a stacking context if necessary.
> 
> Perhaps something like:
> 
>   Update the associated frame state bits so that, if necessary, a stacking
> context will be created and the effect sent to the compositor. We typically
> need to do this when the properties referenced by the keyframe has changed,
> or when the target frame might have changed.
> 
> And, writing this out, I now wonder, does this work for an animation when we
> initially don't have a frame? e.g. If we create an animation on a
> display:none element and then later set it to display:block?

Oho! Good point!  I will add a test case for it.  Considering the test case for 'display:none' restyles[1], in such cases, the NS_FRAME_MAY_BE_TRANSFORMED flag is set to the corresponding frame, but styling process should be entirely skipped.  

[1] http://hg.mozilla.org/mozilla-central/file/c2da34d96746/dom/animation/test/chrome/test_restyles.html#l594

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8763449 [details]
Bug 1279403 - Part 1: Force to apply corresponding change hint if there is no corresponding layer to generate display item even if animation's segment is transform:none or 100% opacity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59648/diff/1-2/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8763450 [details]
Bug 1279403 - Part 2: Set NS_FRAME_MAY_BE_TRANSFORMED bit if the target nsIFrame has transform when setting target or keyframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59650/diff/1-2/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/59648/#review57556

> Did we check that this test fails without the changes in this patch?

It turns out that reftest-no-flush is needed to make this test failure without the change.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8763450 [details]
Bug 1279403 - Part 2: Set NS_FRAME_MAY_BE_TRANSFORMED bit if the target nsIFrame has transform when setting target or keyframes.

https://reviewboard.mozilla.org/r/59650/#review60216

::: dom/animation/KeyframeEffect.h:392
(Diff revision 2)
>    // Remove the current effect target from its EffectSet.
>    void UnregisterTarget();
>  
>    void RequestRestyle(EffectCompositor::RestyleType aRestyleType);
>  
> +  // Update the associated frame state bits so that,if necessary, a stacking

Nit: Space after the comma in "that,if"

::: dom/animation/KeyframeEffect.h:395
(Diff revision 2)
>    void RequestRestyle(EffectCompositor::RestyleType aRestyleType);
>  
> +  // Update the associated frame state bits so that,if necessary, a stacking
> +  // context will be created and the effect sent to the compositor.  We
> +  // typically need to do this when the properties referenced by the keyframe
> +  // has changed, or when the target frame might have changed.

Nit: s/has/have/ (this was my mistake from comment 5).

::: layout/reftests/web-animations/stacking-context-transform-changing-display-property.html:4
(Diff revision 2)
> +<!DOCTYPE html>
> +<html class="reftest-wait reftest-no-flush">
> +<title>
> +Transform animation creates a stacking context when changing its dispaly style

s/dispaly/display/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8763449 [details]
Bug 1279403 - Part 1: Force to apply corresponding change hint if there is no corresponding layer to generate display item even if animation's segment is transform:none or 100% opacity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59648/diff/2-3/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8763450 [details]
Bug 1279403 - Part 2: Set NS_FRAME_MAY_BE_TRANSFORMED bit if the target nsIFrame has transform when setting target or keyframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59650/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba6a3c404ebb
Part 1: Force to apply corresponding change hint if there is no corresponding layer to generate display item even if animation's segment is transform:none or 100% opacity. r=birtles
https://hg.mozilla.org/integration/autoland/rev/fe5ff9a26314
Part 2: Set NS_FRAME_MAY_BE_TRANSFORMED bit if the target nsIFrame has transform when setting target or keyframes. r=birtles

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba6a3c404ebb
https://hg.mozilla.org/mozilla-central/rev/fe5ff9a26314