While the animation is pausing, GetOMTAStyle considers it's running on the compositor whereas isRunningCompositor flag indicates false. We should fix it just like LayerTransactionParent::RecvGetAnimationTransform does. Bellow list is places we use RunningOn.Either instead of RunningOn.MainThread. https://hg.mozilla.org/mozilla-central/file/3e8ee3599a67edd971770af4982ad4b0fe77f073/layout/style/test/test_animations_omta.html#l1208 https://hg.mozilla.org/mozilla-central/file/3e8ee3599a67edd971770af4982ad4b0fe77f073/layout/style/test/test_animations_omta.html#l1214 https://hg.mozilla.org/mozilla-central/file/3e8ee3599a67edd971770af4982ad4b0fe77f073/layout/style/test/test_animations_omta.html#l1231 https://hg.mozilla.org/mozilla-central/file/3e8ee3599a67edd971770af4982ad4b0fe77f073/layout/style/test/test_animations_omta.html#l1365 http://searchfox.org/mozilla-central/rev/3f096f780166bc4772199d79a79c8e3541ff99be/layout/style/test/test_animations_omta.html#1368

It turns out that we cannot return animated only values from GetOMTAStyle because of a test case in test_animations_omta_start.html[1]. [1] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/layout/style/test/test_animations_omta_start.html#154 From bug 996796 comment 40; One reason we see different behavior for opacity as opposed to transforms is that getOMTAStyle behaves differently for each one. For transforms we have a flag on the layer to indicate if the transform was set by animation or not and we only return a value if that flag is set. For opacity we don't have such a flag so we return the value on the layer whether or not it was set by animation. We might need getOMTCStyle.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > From bug 996796 comment 40; > One reason we see different behavior for opacity as opposed to transforms > is that getOMTAStyle behaves differently for each one. For transforms we > have a flag on the layer to indicate if the transform was set by animation > or not and we only return a value if that flag is set. For opacity we don't > have such a flag so we return the value on the layer whether or not it was > set by animation. > > We might need getOMTCStyle. No. The test has been originally flaky, the getOMTAStyle there could have been returning not-animated value.

Sending the transition of child element is done inside call of gUtils.advanceTimeAndRefresh(5000) at http://hg.mozilla.org/mozilla-central/file/6cf0089510fa/layout/style/test/test_animations_omta_start.html#l164 Hmm, we should fix this test first.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > Sending the transition of child element is done inside call of > gUtils.advanceTimeAndRefresh(5000) at > http://hg.mozilla.org/mozilla-central/file/6cf0089510fa/layout/style/test/ > test_animations_omta_start.html#l164 I should note here what I knew. Sending the transition of child element process is done twice. gUtils.advanceTimeAndRefresh(0) and gUtils.advanceTimeAndRefresh(5000).　This is odd. Moreover the first one seems to be done against the same layer of the parent element. I need to investigate more detail, but anyway test refresh mode seems to be broken.

From the test case; http://hg.mozilla.org/mozilla-central/file/6cf0089510fa/layout/style/test/test_animations_omta_start.html#l158 gUtils.advanceTimeAndRefresh(0); waitForAllPaints(function() { var opacity = gUtils.getOMTAStyle(child, "opacity"); is(opacity, "0.4", "transition that interrupted animation is correct"); After setting child element's opacity 1.0, we call advanceTimeAndRefresh(0), inside this call, we try to send the opacity animation to the compositor, but at this time, the animation is still pending, then we hit below condition. http://hg.mozilla.org/mozilla-central/file/6cf0089510fa/layout/base/nsDisplayList.cpp#l506 In normal refresh mode, this kind of animations will be sent in a next tick, so I tried to wait for one more frame by doing one more advanceTimeAndRefresh(0) and waitForAllPaints(). But it did not work because i) computed progress has not changed since the last composition. ii) even if i) is ignored in case of test refresh mode, composed animation style has not changed, then painting process never happens. I am thinking whether we should do advanceTimeAndRefresh(1) to solves this problem or not.

Using advanceTimeAndRefresh(1) makes expected opacity result unclear to me. (It's 0.40006 instead of 0.4) Actually it can be calculated but I've decided to add getComputedStyle(child).opacity to trigger to start the transition just before advanceTimeAndRefresh(0).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90fa93cac638 For my own reference; Without getComputedStyle(child).opacity: 1) We try to trigger pending animations at the top of advanceTimeAndRefresh(0), but at this time transition for the opacity is not created yet. 2) The transition is created in advanceTimeAndRefresh(0). 3) The transition is tried to send to the compositor but it's not sent because the transition is still pending. With getComputedStyle(child).opacity: 1) The transition is created when getComputedStyle(child).opacity is called. The transition is still pending at this time. 2) The pending transition is triggered to be started at the top of advanceTimeAndRefresh(0). 3) The transition is sent to the compositor.

Comment on attachment 8782294 [details] Bug 1279071 - Change GetOpacity to GetAnimationOpacity to return opacity value applied by animation. https://reviewboard.mozilla.org/r/72498/#review70962 Looks good to me with the following nits addressed. Matt or Markus should probably have a look though. ::: gfx/layers/composite/LayerManagerComposite.h:494 (Diff revision 1) > void SetShadowOpacity(float aOpacity) > { > mShadowOpacity = aOpacity; > } > + void SetShadowOpacitySetByAnimation(bool aSetByAnimation) > + { > + mShadowOpacitySetByAnimation = aSetByAnimation; > + } I guess we make this a separate method to be consistent with what we do for transforms. Is there any other reason we can't just make SetShadowOpacity take two parameters? Are these methods only allow to take one argument? ::: gfx/layers/ipc/PLayerTransaction.ipdl:79 (Diff revision 1) > // animations. sampleTime must not be null. > sync SetTestSampleTime(TimeStamp sampleTime); > // Leave test mode and resume normal compositing > sync LeaveTestMode(); > > - sync GetOpacity(PLayer layer) returns (float opacity); > + // Returns the value of the opacity applied to the layer by animation. We should document what |hasAnimationOpacity| means/does. ::: layout/style/test/test_animations_omta.html:1231 (Diff revision 1) > - omta_is_approx("opacity", gTF.ease_out(0.5), 0.01, RunningOn.Either, > - "animation-play-state test 2 at 4000ms"); > + omta_is_approx("opacity", gTF.ease_out(0.5), 0.01, RunningOn.MainThread, > + "animation-play-state test 2 at 4000ms"); // paused > omta_is_approx("transform", { ty: 100 * gTF.ease_in(0.375) }, 0.01, > RunningOn.MainThread, > "animation-play-state test 3 at 4000ms"); We have "// paused" after the opacity test but not the transform. I think they're both paused? We should either have the comment after both or after neither.

Comment on attachment 8782294 [details] Bug 1279071 - Change GetOpacity to GetAnimationOpacity to return opacity value applied by animation. https://reviewboard.mozilla.org/r/72498/#review71156

(In reply to Brian Birtles (:birtles) from comment #9) > Comment on attachment 8782294 [details] > Bug 1279071 - Change GetOpacity to GetAnimationOpacity to return opacity > value applied by animation. > > https://reviewboard.mozilla.org/r/72498/#review70962 > > Looks good to me with the following nits addressed. Matt or Markus should > probably have a look though. > > ::: gfx/layers/composite/LayerManagerComposite.h:494 > (Diff revision 1) > > void SetShadowOpacity(float aOpacity) > > { > > mShadowOpacity = aOpacity; > > } > > + void SetShadowOpacitySetByAnimation(bool aSetByAnimation) > > + { > > + mShadowOpacitySetByAnimation = aSetByAnimation; > > + } > > I guess we make this a separate method to be consistent with what we do for > transforms. Is there any other reason we can't just make SetShadowOpacity > take two parameters? Are these methods only allow to take one argument? No reason, I never thought that. Yes, that's a good idea, but I prefer to use enum for the second parameter instead of boolean. I will open a new bug for it and handle it this Saturday, a hackathon event for beginners.

Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/autoland/rev/7f4de87705e0 Change GetOpacity to GetAnimationOpacity to return opacity value applied by animation. r=birtles,mstange

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11) > (In reply to Brian Birtles (:birtles) from comment #9) > > Comment on attachment 8782294 [details] > > Bug 1279071 - Change GetOpacity to GetAnimationOpacity to return opacity > > value applied by animation. > > > > https://reviewboard.mozilla.org/r/72498/#review70962 > > > > Looks good to me with the following nits addressed. Matt or Markus should > > probably have a look though. > > > > ::: gfx/layers/composite/LayerManagerComposite.h:494 > > (Diff revision 1) > > > void SetShadowOpacity(float aOpacity) > > > { > > > mShadowOpacity = aOpacity; > > > } > > > + void SetShadowOpacitySetByAnimation(bool aSetByAnimation) > > > + { > > > + mShadowOpacitySetByAnimation = aSetByAnimation; > > > + } > > > > I guess we make this a separate method to be consistent with what we do for > > transforms. Is there any other reason we can't just make SetShadowOpacity > > take two parameters? Are these methods only allow to take one argument? > > No reason, I never thought that. Yes, that's a good idea, but I prefer to > use enum for the second parameter instead of boolean. I will open a new bug > for it and handle it this Saturday, a hackathon event for beginners. Filed bug 1297944.