In bug 1273042 I did miss that stacking context is not created if OMTA is disabled.

I've noticed that we should create a stacking context when animation is paused on transform:none or opacity:1. I am going to add such test cases in this bug.

Oh gosh. I missed fill:forwards case at [1]. We need nsLayoutUtils::HasInEffectAnimationOfProperty here in this bug and change it to nsLayoutUtils::HasCurrentOrInEffectAnimationOfProperty (or IsRelevantAnimationOfProperty) in bug 1223658. https://hg.mozilla.org/mozilla-central/rev/02145adb5fc0#l1.13

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

We already have a wrapper named nsIFrame::BackfaceIsHidden(). Review commit: https://reviewboard.mozilla.org/r/59640/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59640/

We want to know that an nsIFrame has a transform or opacity animations regardless of its state, e.g., running on the main-thead, paused or finished but in fill:forwards state, to create a stacking context for the animation. Review commit: https://reviewboard.mozilla.org/r/59642/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59642/

We have to create stacking context for transform or opacity animations no matter what the animation state is, e.g. paused, unabled to run on the compositor or has fill:forwards. Review commit: https://reviewboard.mozilla.org/r/59644/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59644/

I will ask Brian to review part 1 and part 2 once we get back to work.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > I will ask Brian to review part 1 and part 2 once we get back to work. once *he* get back to work. I've been already back to home.

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. Welcome back, Brian!

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. https://reviewboard.mozilla.org/r/59642/#review57356 ::: layout/base/nsLayoutUtils.h:2257 (Diff revision 1) > + * Returns true if the frame has in-effect (i.e. running or finished but > + * fill:forwards) animations or transitions for the property. fill:backwards is also in-effect so perhaps we should say "i.e. running or filling" instead of "i.e. running or finished but fill:forwards"?

https://reviewboard.mozilla.org/r/59644/#review57358 I don't understand this change. We are changing from checking if we have "current" animations to "in effect" animations, but "in effect" doesn't include animations in their delay phase with fill:none or fill:forwards (whereas "current" does). Is that change intended? (Or is there no change because GetAnimationOfProperty checks mWinsInCascade and that returns false when we are in the delay phase with no backwards fill?) It seems like we want to continue returning true for animations in their delay phase.

(In reply to Brian Birtles (:birtles) from comment #12) > https://reviewboard.mozilla.org/r/59644/#review57358 > > I don't understand this change. We are changing from checking if we have > "current" animations to "in effect" animations, but "in effect" doesn't > include animations in their delay phase with fill:none or fill:forwards > (whereas "current" does). Is that change intended? (Or is there no change > because GetAnimationOfProperty checks mWinsInCascade and that returns false > when we are in the delay phase with no backwards fill?) It seems like we > want to continue returning true for animations in their delay phase. I am sorry for the confustion. Yes, it's intentional. For delay phase, I am going change nsLayoutUtils::HasInEffectAnimationOfProperty to nsLayoutUtils::HasRelevantAnimationOfProperty in bug 1223658.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13) > (In reply to Brian Birtles (:birtles) from comment #12) > > https://reviewboard.mozilla.org/r/59644/#review57358 > > > > I don't understand this change. We are changing from checking if we have > > "current" animations to "in effect" animations, but "in effect" doesn't > > include animations in their delay phase with fill:none or fill:forwards > > (whereas "current" does). Is that change intended? (Or is there no change > > because GetAnimationOfProperty checks mWinsInCascade and that returns false > > when we are in the delay phase with no backwards fill?) It seems like we > > want to continue returning true for animations in their delay phase. > > I am sorry for the confustion. Yes, it's intentional. For delay phase, I > am going change nsLayoutUtils::HasInEffectAnimationOfProperty to > nsLayoutUtils::HasRelevantAnimationOfProperty in bug 1223658. Does this mean we're introducing a regression between these two bugs?

(In reply to Brian Birtles (:birtles) from comment #14) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #13) > > (In reply to Brian Birtles (:birtles) from comment #12) > > > https://reviewboard.mozilla.org/r/59644/#review57358 > > > > > > I don't understand this change. We are changing from checking if we have > > > "current" animations to "in effect" animations, but "in effect" doesn't > > > include animations in their delay phase with fill:none or fill:forwards > > > (whereas "current" does). Is that change intended? (Or is there no change > > > because GetAnimationOfProperty checks mWinsInCascade and that returns false > > > when we are in the delay phase with no backwards fill?) It seems like we > > > want to continue returning true for animations in their delay phase. > > > > I am sorry for the confustion. Yes, it's intentional. For delay phase, I > > am going change nsLayoutUtils::HasInEffectAnimationOfProperty to > > nsLayoutUtils::HasRelevantAnimationOfProperty in bug 1223658. > > Does this mean we're introducing a regression between these two bugs? No. This bug creates stacking context for 'in effect' animations regardless of its playing state, e.g. running on the main-thread, etc. Bug 1223658 will create stacking context in delay phase in addition to 'in effect' animations.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15) > (In reply to Brian Birtles (:birtles) from comment #14) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #13) > > > (In reply to Brian Birtles (:birtles) from comment #12) > > > > https://reviewboard.mozilla.org/r/59644/#review57358 > > > > > > > > I don't understand this change. We are changing from checking if we have > > > > "current" animations to "in effect" animations, but "in effect" doesn't > > > > include animations in their delay phase with fill:none or fill:forwards > > > > (whereas "current" does). Is that change intended? (Or is there no change > > > > because GetAnimationOfProperty checks mWinsInCascade and that returns false > > > > when we are in the delay phase with no backwards fill?) It seems like we > > > > want to continue returning true for animations in their delay phase. > > > > > > I am sorry for the confustion. Yes, it's intentional. For delay phase, I > > > am going change nsLayoutUtils::HasInEffectAnimationOfProperty to > > > nsLayoutUtils::HasRelevantAnimationOfProperty in bug 1223658. > > > > Does this mean we're introducing a regression between these two bugs? > > No. This bug creates stacking context for 'in effect' animations regardless > of its playing state, e.g. running on the main-thread, etc. Bug 1223658 > will create stacking context in delay phase in addition to 'in effect' > animations. Ok. From what I can tell, for an animation with no backwards fill that has yet to start, nsIFrame::IsTransformed() currently returns true, but with this patch it will return false. Likewise for nsIFrame::HasOpacityInternal(). Similarly, for such an animation, nsFrame::Init will currently set the NS_FRAME_MAY_BE_TRANSFORMED, but with this patch it will not. Is that right? If it is, are we sure that doesn't have other side effects?

(In reply to Brian Birtles (:birtles) from comment #16) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #15) > > (In reply to Brian Birtles (:birtles) from comment #14) > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #13) > > > > (In reply to Brian Birtles (:birtles) from comment #12) > > > > > https://reviewboard.mozilla.org/r/59644/#review57358 > > > > > > > > > > I don't understand this change. We are changing from checking if we have > > > > > "current" animations to "in effect" animations, but "in effect" doesn't > > > > > include animations in their delay phase with fill:none or fill:forwards > > > > > (whereas "current" does). Is that change intended? (Or is there no change > > > > > because GetAnimationOfProperty checks mWinsInCascade and that returns false > > > > > when we are in the delay phase with no backwards fill?) It seems like we > > > > > want to continue returning true for animations in their delay phase. > > > > > > > > I am sorry for the confustion. Yes, it's intentional. For delay phase, I > > > > am going change nsLayoutUtils::HasInEffectAnimationOfProperty to > > > > nsLayoutUtils::HasRelevantAnimationOfProperty in bug 1223658. > > > > > > Does this mean we're introducing a regression between these two bugs? > > > > No. This bug creates stacking context for 'in effect' animations regardless > > of its playing state, e.g. running on the main-thread, etc. Bug 1223658 > > will create stacking context in delay phase in addition to 'in effect' > > animations. > > Ok. From what I can tell, for an animation with no backwards fill that has > yet to start, nsIFrame::IsTransformed() currently returns true, but with > this patch it will return false. Likewise for > nsIFrame::HasOpacityInternal(). Similarly, for such an animation, > nsFrame::Init will currently set the NS_FRAME_MAY_BE_TRANSFORMED, but with > this patch it will not. Is that right? Ah, right. I did totally miss the case. Thanks. Then, we should create stacking context for animations in delay phase as well in this bug, and send the animations to the compositor in bug 1223658. Though I did not realize at all, fill:backwards animations which can run on the compositor consume the main thread until it's active. This behavior also fixes in bug 1223658.

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

Comment on attachment 8763444 [details] Bug 1278136 - Part 0: Clean up frame->StyleDisplay()->BackfaceIsHidden() usage. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59640/diff/1-2/

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59642/diff/1-2/

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59644/diff/1-2/

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. Brian, I am sorry for sending review requests repeatedly, and thank you for taking your time in your busy work. Now I've changed nsLayoutUtils::HasInEffectAnimationOfProperty to HasRelevantAnimationOfProperty to check both of in-effect and current for creation of stacking context in delay phase. And also I've added additional test cases for stacking contest in delay phase in part 2 patch, and a test case to check animation in delay phase is not running on the compositor in part 3 patch. Thank you!

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. Forgot to add r?birtles in commit message.

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. https://reviewboard.mozilla.org/r/59642/#review57576 ::: layout/base/nsLayoutUtils.h:2257 (Diff revision 2) > + * Returns true if the frame has current or in-effect (i.e. in before phase, > + * running or filling) animations or transitions for the > + * property. Returns true if the frame has any current or in-effect animations or transitions for the specified property (i.e. any animations that haven't yet finished, or which have finished but are applying a forwards fill) ?

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. https://reviewboard.mozilla.org/r/59644/#review57582 ::: dom/animation/EffectCompositor.cpp:670 (Diff revision 2) > - bool inEffect = effect->IsInEffect(); > + bool isCurrentOrInEffect = effect->IsCurrent() || effect->IsInEffect(); > for (AnimationProperty& prop : effect->Properties()) { > > bool winsInCascade = !animatedProperties.HasProperty(prop.mProperty) && > - inEffect; > + isCurrentOrInEffect; I don't understand how this addresses bug 1223658 comment 149. Why is it ok to set winsInCascade to true for an animation in the delay phase that does not fill backwards? I think winsInCascade is used for a few different purposes like: * Deciding which animations to send to the compositor [for which this change is partly ok? But won't in mean we fail to send later animations?] * Deciding which animation/transition to apply [for which this change is probably *not* ok?] Is that right?

(In reply to Brian Birtles (:birtles) from comment #25) > Comment on attachment 8763446 [details] > Bug 1278136 - Part 2: We should not check whether the animation can run on > the compositor or it's paused. > > https://reviewboard.mozilla.org/r/59644/#review57582 > > ::: dom/animation/EffectCompositor.cpp:670 > (Diff revision 2) > > - bool inEffect = effect->IsInEffect(); > > + bool isCurrentOrInEffect = effect->IsCurrent() || effect->IsInEffect(); > > for (AnimationProperty& prop : effect->Properties()) { > > > > bool winsInCascade = !animatedProperties.HasProperty(prop.mProperty) && > > - inEffect; > > + isCurrentOrInEffect; > > I don't understand how this addresses bug 1223658 comment 149. Why is it ok > to set winsInCascade to true for an animation in the delay phase that does > not fill backwards? > > I think winsInCascade is used for a few different purposes like: > > * Deciding which animations to send to the compositor [for which this change > is partly ok? But won't in mean we fail to send later animations?] > * Deciding which animation/transition to apply [for which this change is > probably *not* ok?] > > Is that right? Yes, right. A problem there is that winsInCascade is initially false until the effect is in current even if there is no competitor. Should we add another variant of KeyframeEffectReadOnly::GetAnimationOfPropery()?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26) > Yes, right. A problem there is that winsInCascade is initially false until > the effect is in current even if there is no competitor. Should we add > another variant of KeyframeEffectReadOnly::GetAnimationOfPropery()? To be more clear, current KeyframeEffectReadOnly::GetAnimationOfPropery() implies that we have an effect which is not in delay phase.

> (In reply to Hiroyuki Ikezoe (:hiro) from comment #26) > > > Yes, right. A problem there is that winsInCascade is initially false until > > the effect is in current even if there is no competitor. Should we add > > another variant of KeyframeEffectReadOnly::GetAnimationOfPropery()? > > To be more clear, current KeyframeEffectReadOnly::GetAnimationOfPropery() > implies that we have an effect which is not in delay phase. Maybe. What are the call sites of GetAnimationOfProperty? Can we just drop the check for mWinsInCascade from GetAnimationOfProperty? It seems like AddAnimationsForProperty would be ok with this change? At least once we ship animations to the compositor during their delay phase. I don't know about all the uses of HasAnimationOfProperty. We should probably go through all the call sites of GetAnimationOfProperty and see what makes sense for each both before and after bug 1223658. (And, on that matter, how does this bug relate to bug 1223658? Does it block it or depend on it?)

(In reply to Brian Birtles (:birtles) from comment #28) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #26) > > > > > Yes, right. A problem there is that winsInCascade is initially false until > > > the effect is in current even if there is no competitor. Should we add > > > another variant of KeyframeEffectReadOnly::GetAnimationOfPropery()? > > > > To be more clear, current KeyframeEffectReadOnly::GetAnimationOfPropery() > > implies that we have an effect which is not in delay phase. > > Maybe. What are the call sites of GetAnimationOfProperty? Can we just drop > the check for mWinsInCascade from GetAnimationOfProperty? I don't think we can drop the check in GetAnimationOfProperty. If we dropped the check, we will try to send animations which do not actually win in the cascades to the compositor. An example is a transform animation in our test[1]. [1] http://hg.mozilla.org/mozilla-central/file/c2da34d96746/dom/animation/test/chrome/test_running_on_compositor.html#l284 > It seems like AddAnimationsForProperty would be ok with this change? > At least once we ship animations to the compositor during their delay phase. If 'this change' means dropping the check, then no as I commented above about a test case. If 'this change' means about attachment 8763446 [details], then yes. :-) > I don't know about all the uses of HasAnimationOfProperty. * In FindAnimationsForCompositor() http://hg.mozilla.org/mozilla-central/file/c2da34d96746/dom/animation/EffectCompositor.cpp#l126 If we check also mWinsInCascade here, then GetAnimationOfProperty in AddAnimationsForProperty should not be a problem. Yes, once FindAnimationsForCompositor() returns true even if the animation is in delay phase, it gets something weird situation until bug 1223658 is landed. * In KeyframeEffectReadOnly::CanThrottle() http://hg.mozilla.org/mozilla-central/file/c2da34d96746/dom/animation/KeyframeEffect.cpp#l1088 I think this HasAnimationOfProperty does not matter whether it includes delay phase or not. > We should probably go through all the call sites of GetAnimationOfProperty > and see what makes sense for each both before and after bug 1223658. (And, > on that matter, how does this bug relate to bug 1223658? Does it block it or > depend on it?) Before bug 1223658, Animation::IsPlay() stops sending animations in delay phase to the compositor. IsPlay() is the last barrier not to send animations in delay phase to the compositor. Yes, bug 1223658 depends on this bug. I thought I did set the dependency. Hmm, I am now inclined to this part 2 patch should be integrated in bug 1223658 if we handle delay phase in this bug. Does it sound reasonable?

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. Clearing review request since I've decided that this patch is integrated in bug 1223658.

Comment on attachment 8763444 [details] Bug 1278136 - Part 0: Clean up frame->StyleDisplay()->BackfaceIsHidden() usage. https://reviewboard.mozilla.org/r/59640/#review57634

Comment on attachment 8765321 [details] Bug 1278136 - Part 3: Test that animations with fill:backwards consumes the main-thread while it's in delay phase. https://reviewboard.mozilla.org/r/60762/#review57992 This seems fine although I wonder if we will end up modifying it an including it in bug 1223658.

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. https://reviewboard.mozilla.org/r/59644/#review58416

It turns out that creating stacking context for animations in delay phase needs part 1 patch for bug 1279403 and bug 1279403 needs part 2 patch here. So I am going to drop the change against EffectCompositor::UpdateCascadeResults and mark reftests for delay phase as "fails" here. Then, after landing bug 1279403, I will re-visit this bug to work animations in delay phase correctly. Oops! Bugzilla does not allow mutual dependency. So I am going to mark bug 1279403 depending on this bug.

Comment on attachment 8763444 [details] Bug 1278136 - Part 0: Clean up frame->StyleDisplay()->BackfaceIsHidden() usage. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59640/diff/2-3/

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59642/diff/2-3/

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59644/diff/2-3/

Comment on attachment 8765321 [details] Bug 1278136 - Part 3: Test that animations with fill:backwards consumes the main-thread while it's in delay phase. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60762/diff/1-2/

(In reply to Hiroyuki Ikezoe (:hiro) from comment #37) > Comment on attachment 8763446 [details] > Bug 1278136 - Part 2: We should not check whether the animation can run on > the compositor or it's paused. > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/59644/diff/2-3/ Added two reftests to check for animated property losing in the cascade too.

https://reviewboard.mozilla.org/r/59644/#review59648 ::: layout/reftests/css-animations/reftest.list:11 (Diff revision 3) > -== stacking-context-transform-none-animation.html stacking-context-transform-animation-ref.html > -== stacking-context-transform-none-animation-on-svg.html stacking-context-transform-animation-ref.html > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-opacity-animation.html stacking-context-animation-ref.html > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-transform-animation.html stacking-context-animation-ref.html Why do these need the pref to be cleared?

https://reviewboard.mozilla.org/r/59644/#review59650 In the commit message: > We have to create stacking context for transform or opacity animations > no matter what the animation state is, e.g. paused, unabled to run on the > compositor or has fill:forwards. Perhaps: We should create a stacking context for any transform or opacity animations that are either "in effect" (what we currently do) OR "current", i.e. scheduled to run or running. Does that seem accurate?

(In reply to Brian Birtles (:birtles) from comment #41) > https://reviewboard.mozilla.org/r/59644/#review59650 > > In the commit message: > > > We have to create stacking context for transform or opacity animations > > no matter what the animation state is, e.g. paused, unabled to run on the > > compositor or has fill:forwards. > > Perhaps: > > We should create a stacking context for any transform or opacity animations > that are either "in effect" (what we currently do) OR "current", i.e. > scheduled to run or running. > > Does that seem accurate? Oops, that's back to front. Should be: We should create a stacking context for any transform or opacity animations that are either "current" (what we currently do, i.e. running or scheduled to run) OR "in effect", i.e. including animations that are filling forwards.

(In reply to Brian Birtles (:birtles) from comment #40) > https://reviewboard.mozilla.org/r/59644/#review59648 > > ::: layout/reftests/css-animations/reftest.list:11 > (Diff revision 3) > > -== stacking-context-transform-none-animation.html stacking-context-transform-animation-ref.html > > -== stacking-context-transform-none-animation-on-svg.html stacking-context-transform-animation-ref.html > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-opacity-animation.html stacking-context-animation-ref.html > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-transform-animation.html stacking-context-animation-ref.html > > Why do these need the pref to be cleared? That's because we need to test that opacity or transform animations create stacking context even if OMTA is disabled. And I think it's good enough just to run normal cases there with the disabled pref. (In reply to Brian Birtles (:birtles) from comment #42) > (In reply to Brian Birtles (:birtles) from comment #41) > > https://reviewboard.mozilla.org/r/59644/#review59650 > > > > In the commit message: > > > > > We have to create stacking context for transform or opacity animations > > > no matter what the animation state is, e.g. paused, unabled to run on the > > > compositor or has fill:forwards. > > > > Perhaps: > > > > We should create a stacking context for any transform or opacity animations > > that are either "in effect" (what we currently do) OR "current", i.e. > > scheduled to run or running. > > > > Does that seem accurate? > > Oops, that's back to front. Should be: > > We should create a stacking context for any transform or opacity animations > that are either "current" (what we currently do, i.e. running or scheduled > to run) OR "in effect", i.e. including animations that are filling forwards. It's slightly wrong just for now because we don't create any stacking context in delay phase without fill:backwards.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > (In reply to Brian Birtles (:birtles) from comment #40) > > https://reviewboard.mozilla.org/r/59644/#review59648 > > > > ::: layout/reftests/css-animations/reftest.list:11 > > (Diff revision 3) > > > -== stacking-context-transform-none-animation.html stacking-context-transform-animation-ref.html > > > -== stacking-context-transform-none-animation-on-svg.html stacking-context-transform-animation-ref.html > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-opacity-animation.html stacking-context-animation-ref.html > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-transform-animation.html stacking-context-animation-ref.html > > > > Why do these need the pref to be cleared? > > That's because we need to test that opacity or transform animations create > stacking context even if OMTA is disabled. And I think it's good enough > just to run normal cases there with the disabled pref. Can you explain a bit more? What specifically are we trying to test? > (In reply to Brian Birtles (:birtles) from comment #42) > > (In reply to Brian Birtles (:birtles) from comment #41) > > > https://reviewboard.mozilla.org/r/59644/#review59650 > > > > > > In the commit message: > > > > > > > We have to create stacking context for transform or opacity animations > > > > no matter what the animation state is, e.g. paused, unabled to run on the > > > > compositor or has fill:forwards. > > > > > > Perhaps: > > > > > > We should create a stacking context for any transform or opacity animations > > > that are either "in effect" (what we currently do) OR "current", i.e. > > > scheduled to run or running. > > > > > > Does that seem accurate? > > > > Oops, that's back to front. Should be: > > > > We should create a stacking context for any transform or opacity animations > > that are either "current" (what we currently do, i.e. running or scheduled > > to run) OR "in effect", i.e. including animations that are filling forwards. > > It's slightly wrong just for now because we don't create any stacking > context in delay phase without fill:backwards. Ok. We need a comment to say what this patch is doing. It's not obvious to me what's happenning and I'm sure if someone looks at this in a year's time, they will have the same question.

(In reply to Brian Birtles (:birtles) from comment #44) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > > (In reply to Brian Birtles (:birtles) from comment #40) > > > https://reviewboard.mozilla.org/r/59644/#review59648 > > > > > > ::: layout/reftests/css-animations/reftest.list:11 > > > (Diff revision 3) > > > > -== stacking-context-transform-none-animation.html stacking-context-transform-animation-ref.html > > > > -== stacking-context-transform-none-animation-on-svg.html stacking-context-transform-animation-ref.html > > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-opacity-animation.html stacking-context-animation-ref.html > > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-transform-animation.html stacking-context-animation-ref.html > > > > > > Why do these need the pref to be cleared? > > > > That's because we need to test that opacity or transform animations create > > stacking context even if OMTA is disabled. And I think it's good enough > > just to run normal cases there with the disabled pref. > > Can you explain a bit more? What specifically are we trying to test? 'transform: none' or 'opacity:1' animation when EffectCompositor::HasAnimationsForCompositor() returns false. Before part 2 patch, we used EffectCompositor::HasAnimationsForCompositor() in nsIFrame::IsTransformed() or nsIFrame::HasOpacityInternal(), so the test failed without the change. > > (In reply to Brian Birtles (:birtles) from comment #42) > > > (In reply to Brian Birtles (:birtles) from comment #41) > > > > https://reviewboard.mozilla.org/r/59644/#review59650 > > > > > > > > In the commit message: > > > > > > > > > We have to create stacking context for transform or opacity animations > > > > > no matter what the animation state is, e.g. paused, unabled to run on the > > > > > compositor or has fill:forwards. > > > > > > > > Perhaps: > > > > > > > > We should create a stacking context for any transform or opacity animations > > > > that are either "in effect" (what we currently do) OR "current", i.e. > > > > scheduled to run or running. > > > > > > > > Does that seem accurate? > > > > > > Oops, that's back to front. Should be: > > > > > > We should create a stacking context for any transform or opacity animations > > > that are either "current" (what we currently do, i.e. running or scheduled > > > to run) OR "in effect", i.e. including animations that are filling forwards. > > > > It's slightly wrong just for now because we don't create any stacking > > context in delay phase without fill:backwards. > > Ok. We need a comment to say what this patch is doing. It's not obvious to > me what's happenning and I'm sure if someone looks at this in a year's time, > they will have the same question. Okay, I will update the commit message.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #45) > (In reply to Brian Birtles (:birtles) from comment #44) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > > > (In reply to Brian Birtles (:birtles) from comment #40) > > > > https://reviewboard.mozilla.org/r/59644/#review59648 > > > > > > > > ::: layout/reftests/css-animations/reftest.list:11 > > > > (Diff revision 3) > > > > > -== stacking-context-transform-none-animation.html stacking-context-transform-animation-ref.html > > > > > -== stacking-context-transform-none-animation-on-svg.html stacking-context-transform-animation-ref.html > > > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-opacity-animation.html stacking-context-animation-ref.html > > > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-transform-animation.html stacking-context-animation-ref.html > > > > > > > > Why do these need the pref to be cleared? > > > > > > That's because we need to test that opacity or transform animations create > > > stacking context even if OMTA is disabled. And I think it's good enough > > > just to run normal cases there with the disabled pref. > > > > Can you explain a bit more? What specifically are we trying to test? > > 'transform: none' or 'opacity:1' animation when > EffectCompositor::HasAnimationsForCompositor() returns false. Before part 2 > patch, we used EffectCompositor::HasAnimationsForCompositor() in > nsIFrame::IsTransformed() or nsIFrame::HasOpacityInternal(), so the test > failed without the change. So let me check, for the case of nsIFrame::IsTransformed: Prior to part 2 we used EffectCompositor::HasAnimationsForCompositor -> FindAnimationsForCompositor(aFrame, aProperty, nullptr) which returns false if: * there are no effects, or * the frame was refused async animation, or * OMTA is disabled, or * there is no playing animation of |aProperty|, or * |aProperty| is transform but we have an animation of left/top etc. (i.e. something that block running it on the compositor) As of part 2 we use nsLayoutUtils::HasRelevantAnimationOfProperty which is added in part 1 and returns false if: * there are no effects, or * there is no current or in-effect effect for |aProperty|. As a result IsTransformed will now return true where it previously returned false if: * the frame was refused async animation, or * OMTA is disabled, or ** the animation is paused, or ** the animation is in the before phase, ** the animation is filling forwards, or * |aProperty| is transform but we have an animation of left/top etc. Therefore, in theory we will now create a stacking context in these cases. However, the ** cases require some extra work as described in comment 4. As a result stacking-context-opacity-animation.html will now pass even if we turn off OMTA. Is that right? When you say, "the test failed without the change" (comment 45), does that mean that, "Before part 2, if we turn off OMTA the test would fail, but now it will pass whether we turn OMTA on or off?"

(In reply to Brian Birtles (:birtles) from comment #46) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #45) > > (In reply to Brian Birtles (:birtles) from comment #44) > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > > > > (In reply to Brian Birtles (:birtles) from comment #40) > > > > > https://reviewboard.mozilla.org/r/59644/#review59648 > > > > > > > > > > ::: layout/reftests/css-animations/reftest.list:11 > > > > > (Diff revision 3) > > > > > > -== stacking-context-transform-none-animation.html stacking-context-transform-animation-ref.html > > > > > > -== stacking-context-transform-none-animation-on-svg.html stacking-context-transform-animation-ref.html > > > > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-opacity-animation.html stacking-context-animation-ref.html > > > > > > +test-pref(layers.offmainthreadcomposition.async-animations,false) == stacking-context-transform-animation.html stacking-context-animation-ref.html > > > > > > > > > > Why do these need the pref to be cleared? > > > > > > > > That's because we need to test that opacity or transform animations create > > > > stacking context even if OMTA is disabled. And I think it's good enough > > > > just to run normal cases there with the disabled pref. > > > > > > Can you explain a bit more? What specifically are we trying to test? > > > > 'transform: none' or 'opacity:1' animation when > > EffectCompositor::HasAnimationsForCompositor() returns false. Before part 2 > > patch, we used EffectCompositor::HasAnimationsForCompositor() in > > nsIFrame::IsTransformed() or nsIFrame::HasOpacityInternal(), so the test > > failed without the change. > > So let me check, for the case of nsIFrame::IsTransformed: > > Prior to part 2 we used EffectCompositor::HasAnimationsForCompositor > -> FindAnimationsForCompositor(aFrame, aProperty, nullptr) > which returns false if: > * there are no effects, or > * the frame was refused async animation, or > * OMTA is disabled, or > * there is no playing animation of |aProperty|, or > * |aProperty| is transform but we have an animation of left/top etc. (i.e. > something that block running it on the compositor) > > As of part 2 we use nsLayoutUtils::HasRelevantAnimationOfProperty > which is added in part 1 and returns false if: > * there are no effects, or > * there is no current or in-effect effect for |aProperty|. > > As a result IsTransformed will now return true where it previously returned > false if: > * the frame was refused async animation, or > * OMTA is disabled, or > ** the animation is paused, or > ** the animation is in the before phase, IsTransformed still returns false in the before phase, because HasReleventAnimationOfProperty() uses KeyframeEffectReadOnly::GetAnimationOfProperty which checks mWinsInCascade, the flag will never be true until the effect is in effect phase. > Therefore, in theory we will now create a stacking context in these cases. > However, the ** cases require some extra work as described in comment 4. > > As a result stacking-context-opacity-animation.html will now pass even if we > turn off OMTA. Is that right? > > When you say, "the test failed without the change" (comment 45), does that > mean that, "Before part 2, if we turn off OMTA the test would fail, but now > it will pass whether we turn OMTA on or off?" That's right. For the test case, I am going to re-use the same test file, i.e. stacking-context-opacity-1-animation.html, and add a comment in reftest.list.

Comment on attachment 8763444 [details] Bug 1278136 - Part 0: Clean up frame->StyleDisplay()->BackfaceIsHidden() usage. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59640/diff/3-4/

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59642/diff/3-4/

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59644/diff/3-4/

Comment on attachment 8765321 [details] Bug 1278136 - Part 3: Test that animations with fill:backwards consumes the main-thread while it's in delay phase. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60762/diff/2-3/

(In reply to Hiroyuki Ikezoe (:hiro) from comment #47) > (In reply to Brian Birtles (:birtles) from comment #46) > > Therefore, in theory we will now create a stacking context in these cases. > > However, the ** cases require some extra work as described in comment 4. > > > > As a result stacking-context-opacity-animation.html will now pass even if we > > turn off OMTA. Is that right? > > > > When you say, "the test failed without the change" (comment 45), does that > > mean that, "Before part 2, if we turn off OMTA the test would fail, but now > > it will pass whether we turn OMTA on or off?" > > That's right. > For the test case, I am going to re-use the same test file, i.e. > stacking-context-opacity-1-animation.html, and add a comment in reftest.list. Ok, I'll wait for your updated patch before reviewing. I agree we at least need a comment in reftest.list. I'd slightly prefer that we either (a) test both with/without OMTA, (b) rename the test to make it clear it is only expected to be testing OMTA-off case, or (c) test the other case where IsTransformed now returns true, namely "|aProperty| is transform but we have an animation of left/top etc.".

https://reviewboard.mozilla.org/r/59644/#review59922 In the commit message, "We should not check whether the animation can run on the compositor or it's paused" we should mention that this is with regards to creating a stacking context. e.g. add "... when determining if we should create a stacking context" to the end of the message. ::: layout/reftests/css-animations/no-stacking-context-animation-ref.html:3 (Diff revision 4) > +<!DOCTYPE html> > +<title> > +Reference of testcases which don't create stacking context for bug 1278136 Nit: 'a stacking context' ::: layout/reftests/css-animations/reftest.list:11 (Diff revision 4) > == animate-preserves3d.html animate-preserves3d-ref.html > == in-visibility-hidden-animation.html in-visibility-hidden-animation-ref.html > == in-visibility-hidden-animation-pseudo-element.html in-visibility-hidden-animation-pseudo-element-ref.html > == partially-out-of-view-animation.html partially-out-of-view-animation-ref.html > == animate-display-table-opacity.html animate-display-table-opacity-ref.html > -== stacking-context-transform-none-animation.html stacking-context-transform-animation-ref.html > +# We need to run 100% opacity test case when OMTA is disabled to check that the animation surely creates a stacking context even if the animation is not running on the compositor Nit: Remove 'surely' here and in the next comment. ::: layout/reftests/css-animations/reftest.list:15 (Diff revision 4) > +== stacking-context-lose-opacity-1.html no-stacking-context-animation-ref.html > +== stacking-context-lose-transform-none.html no-stacking-context-animation-ref.html > +== stacking-context-opacity-1-animation.html stacking-context-animation-ref.html > +== stacking-context-opacity-1-with-fill-backwards.html stacking-context-animation-ref.html > +== stacking-context-opacity-1-with-fill-forwards.html stacking-context-animation-ref.html > +== stacking-context-paused-on-opacity-1.html stacking-context-animation-ref.html > +== stacking-context-paused-on-transform-none.html stacking-context-animation-ref.html > +== stacking-context-transform-none-animation.html stacking-context-animation-ref.html > +== stacking-context-transform-none-animation-on-svg.html stacking-context-animation-ref.html > +== stacking-context-transform-none-animation-with-backface-visibility.html stacking-context-animation-ref.html > +== stacking-context-transform-none-animation-with-preserve-3d.html stacking-context-animation-ref.html > +== stacking-context-transform-none-with-fill-backwards.html stacking-context-animation-ref.html > +== stacking-context-transform-none-with-fill-forwards.html stacking-context-animation-ref.html For all of these, is the intention that they should create a stacking context, but don't yet and that we will fix them in another bug? If so, can we add a comment to that? ::: layout/reftests/css-animations/reftest.list:28 (Diff revision 4) > +fails == stacking-context-opacity-1-in-delay.html stacking-context-animation-ref.html > +fails == stacking-context-transform-none-in-delay.html stacking-context-animation-ref.html Is there a bug number we can annotate these failures with? Is that the plan--that we make these pass in another bug?

(In reply to Brian Birtles (:birtles) from comment #53) > (Diff revision 4) > > +== stacking-context-lose-opacity-1.html no-stacking-context-animation-ref.html > > +== stacking-context-lose-transform-none.html no-stacking-context-animation-ref.html > > +== stacking-context-opacity-1-animation.html stacking-context-animation-ref.html > > +== stacking-context-opacity-1-with-fill-backwards.html stacking-context-animation-ref.html > > +== stacking-context-opacity-1-with-fill-forwards.html stacking-context-animation-ref.html > > +== stacking-context-paused-on-opacity-1.html stacking-context-animation-ref.html > > +== stacking-context-paused-on-transform-none.html stacking-context-animation-ref.html > > +== stacking-context-transform-none-animation.html stacking-context-animation-ref.html > > +== stacking-context-transform-none-animation-on-svg.html stacking-context-animation-ref.html > > +== stacking-context-transform-none-animation-with-backface-visibility.html stacking-context-animation-ref.html > > +== stacking-context-transform-none-animation-with-preserve-3d.html stacking-context-animation-ref.html > > +== stacking-context-transform-none-with-fill-backwards.html stacking-context-animation-ref.html > > +== stacking-context-transform-none-with-fill-forwards.html stacking-context-animation-ref.html > > For all of these, is the intention that they should create a stacking > context, but don't yet and that we will fix them in another bug? If so, can > we add a comment to that? These tests can pass now. > ::: layout/reftests/css-animations/reftest.list:28 > (Diff revision 4) > > +fails == stacking-context-opacity-1-in-delay.html stacking-context-animation-ref.html > > +fails == stacking-context-transform-none-in-delay.html stacking-context-animation-ref.html > > Is there a bug number we can annotate these failures with? Is that the > plan--that we make these pass in another bug? Thanks. I added 'bug 1278136 and bug 1279403' locally. *bug 1278136* is actually this bug, the test will be passed with a subsequent patch in this bug.

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. https://reviewboard.mozilla.org/r/59644/#review59928 r=me with comments addressed

Comment on attachment 8763444 [details] Bug 1278136 - Part 0: Clean up frame->StyleDisplay()->BackfaceIsHidden() usage. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59640/diff/4-5/

Comment on attachment 8763445 [details] Bug 1278136 - Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59642/diff/4-5/

Comment on attachment 8763446 [details] Bug 1278136 - Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59644/diff/4-5/

Comment on attachment 8765321 [details] Bug 1278136 - Part 3: Test that animations with fill:backwards consumes the main-thread while it's in delay phase. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60762/diff/3-4/

Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/autoland/rev/114f71154f00 Part 0: Clean up frame->StyleDisplay()->BackfaceIsHidden() usage. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/8d9e9c2f25e8 Part 1: Add nsLayoutUtils::HasRelevantAnimationOfProperty. r=birtles https://hg.mozilla.org/integration/autoland/rev/ef21ce58421e Part 2: We should not check whether the animation can run on the compositor or it's paused when determining if we should create a stacking context. r=birtles,mattwoodrow https://hg.mozilla.org/integration/autoland/rev/18cbc19266b4 Part 3: Test that animations with fill:backwards consumes the main-thread while it's in delay phase. r=birtles

https://hg.mozilla.org/mozilla-central/rev/114f71154f00 https://hg.mozilla.org/mozilla-central/rev/8d9e9c2f25e8 https://hg.mozilla.org/mozilla-central/rev/ef21ce58421e https://hg.mozilla.org/mozilla-central/rev/18cbc19266b4

This is the patch to create a stacking context in delay phase. To create a stacking context in delay phase, we have to know whether the animation style wins in cascade in delay phase.

Comment on attachment 8769519 [details] [diff] [review] Part 4: Drop IsInEffect() check in UpdateCascadeResults Review of attachment 8769519 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand this patch. What are we actually using the wins in cascade member for at this point? When should it be false, when should it be true? Is it only used for blocking transitions? We have code like KeyframeEffectReadOnly::NotifyAnimationTimingUpdated that detects changes to the "in effect" status so I don't understand how this change is sufficient. ::: dom/animation/EffectCompositor.h @@ +155,5 @@ > nsCSSProperty aProperty); > > // Update animation cascade results for the specified (pseudo-)element > // but only if we have marked the cascade as needing an update due a > + // the change in the set of effects. We should fix this comment at the same time to read '... an update to due a change in ...'

Comment on attachment 8774536 [details] [diff] [review] Bug 1278136 - Part 5: Create a stacking context for opacity/transform animations even if it's in delay phase Review of attachment 8774536 [details] [diff] [review]: ----------------------------------------------------------------- This might be fine, but I really don't understand what "visually effective" is supposed to mean. In what sort of situations should it be false? ::: dom/animation/EffectCompositor.cpp @@ +680,5 @@ > + // Unlike winsInCascade flag, mIsVisuallyEffective is true even if > + // the animation is not in effect because we should create a stacking > + // context for the property in delay phase. > + prop.mIsVisuallyEffective = > + !animatedProperties.HasProperty(prop.mProperty) && !isOverridden; I don't really understand what "visually effective" means. In particular, why do we need the check for !animatedProperties.HasProperty()? And what does overriding have to do with it? In what situations, other than when the animation has finished (and is not filling) do we want this to return false? ::: dom/animation/KeyframeEffect.cpp @@ +507,5 @@ > return nullptr; > } > > +bool > +KeyframeEffectReadOnly::HasVisuallyEffectiveAnimationOfPropertyForCompositor( This is a pretty long function name--but I guess this relates to my previous question, i.e. once I understand what this is doing I can probably better understand what we should call this. ::: dom/animation/KeyframeEffect.h @@ +155,5 @@ > bool mWinsInCascade = false; > > + // This flag is similar to mWinsInCascade. Unlike mWinsInCascade this flag > + // does not consider whether each animation is in effect or not since we > + // create a stacking context for opacity or transform animations even if This description doesn't say what the member does--just how it differs from mWinsInCascade. What is its purpose? What does it mean? @@ +160,5 @@ > + // it's in delay phase. > + // > + // **NOTE**: This member is not included when comparing AnimationProperty > + // objects for equality. > + bool mIsVisuallyEffective = false; Nit: Space after this. ::: layout/base/nsLayoutUtils.cpp @@ +502,5 @@ > return HasMatchingAnimations(aFrame, > [&aProperty](KeyframeEffectReadOnly& aEffect) > { > return (aEffect.IsInEffect() || aEffect.IsCurrent()) && > + aEffect.HasVisuallyEffectiveAnimationOfPropertyForCompositor(aProperty); This function's name no longer matches what it does. "Relevant" has the specific meaning of "in effect or current". With this change, this function no longer matches that meaning. It seems like we need to rename this method.

Comment on attachment 8774535 [details] [diff] [review] Bug 1278136 - Part 4: Tests to check stacking context for correct effect in cascading rule Review of attachment 8774535 [details] [diff] [review]: ----------------------------------------------------------------- I went through these tests and I thought they were good but now I wonder if this is actually the correct behavior. The spec says, "While an animation is applied but has not finished, or has finished but has an animation-fill-mode of forwards or both, the user agent must act as if the will-change property ([css-will-change-1]) on the element additionally includes all the properties animated by the animation."[1] That suggests that even if an important rule is applying 'transform: none' or 'opacity: 1', we should still create a stacking context. This is consistent with the fact that if we have an animation that animates from opacity '1' to '0', and has a 10s delay and no backwards fill, even during the delay phase where the opacity is '1' we still create a stacking context. Is there any reason to not create a stacking context when we have an important rule? [1] https://drafts.csswg.org/css-animations/#animations ::: layout/reftests/css-animations/stacking-context-opacity-win-in-delay-on-main-thread.html @@ +1,3 @@ > +<!DOCTYPE html> > +<title> > +Opacity animation winning to another opacity animation in delay phase Nit: winning over @@ +23,5 @@ > +} > +#test { > + width: 100px; height: 100px; > + background: blue; > + animation: Opacity0 100s 100s, Opacity1 100s, Width 100s; I don't quite understand what this is testing. Wouldn't animation: Opacity1 100s 100s, Width 100s still work? Why would another animation winning change the result? That said, it's probably fine. ::: layout/reftests/css-animations/stacking-context-opacity-win-in-delay.html @@ +1,3 @@ > +<!DOCTYPE html> > +<title> > +Opacity animation winning to another opacity animation in delay phase winning over ::: layout/reftests/css-animations/stacking-context-opacity-win-to-style.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html class="reftest-wait"> > +<title> > +Opacity animation winning to opacity style creates a stacking context winning over Likewise in the filename (s/win-to-style/wins-over-style/) @@ +28,5 @@ > +window.addEventListener("load", () => { > + var target = document.getElementById("test"); > + getComputedStyle(target).opacity; > + > + // CSS animation should win style, so transition won't happen during the win over style s/won't happen/won't be visible/ ::: layout/reftests/css-animations/stacking-context-transform-win-in-delay-on-main-thread.html @@ +1,3 @@ > +<!DOCTYPE html> > +<title> > +Transform animation winning to another transform animation in delay phase winning over ::: layout/reftests/css-animations/stacking-context-transform-win-in-delay.html @@ +1,3 @@ > +<!DOCTYPE html> > +<title> > +Transform animation winning to another transform animation in delay phase winning over ::: layout/reftests/css-animations/stacking-context-transform-win-to-style.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html class="reftest-wait"> > +<title> > +Transform animation winning to transform style creates a stacking context winning over. Likewise in filename ::: layout/reftests/css-transitions/stacking-context-opacity-lose-to-animation.html @@ +34,5 @@ > + target.style.setProperty("opacity", "1", "important"); > + // Important style is changed but there is a CSS animation, > + // so transition does not start and the CSS animation is not effective > + // due to the important style. As a result opacity style here should > + // be 100%, so we should not create any stacking context for it. Wow, that's a complicated test! Do we have a test that checks that if we remove the important style during the delay phase we update layers accordingly? ::: layout/reftests/web-animations/stacking-context-transform-losing-css-animation-in-delay.html @@ +11,5 @@ > + background: green; > + top: 50px; > +} > +@keyframes Transform100px { > + from, to { transform: none; } This doesn't seem like the right name for this animation.

(In reply to Brian Birtles (:birtles) from comment #67) > Comment on attachment 8774535 [details] [diff] [review] > Bug 1278136 - Part 4: Tests to check stacking context for correct effect in > cascading rule > > Review of attachment 8774535 [details] [diff] [review]: > ----------------------------------------------------------------- > > I went through these tests and I thought they were good but now I wonder if > this is actually the correct behavior. The spec says, > > "While an animation is applied but has not finished, or has finished but > has an animation-fill-mode of forwards or both, the user agent must act as > if the will-change property ([css-will-change-1]) on the element > additionally includes all the properties animated by the animation."[1] > > That suggests that even if an important rule is applying 'transform: none' > or 'opacity: 1', we should still create a stacking context. This is > consistent with the fact that if we have an animation that animates from > opacity '1' to '0', and has a 10s delay and no backwards fill, even during > the delay phase where the opacity is '1' we still create a stacking context. > > Is there any reason to not create a stacking context when we have an > important rule? I was misunderstanding that 'will-change' does not create a stacking context if there is an important rule. I will revise test cases here, and should revise part 5 patch. > @@ +23,5 @@ > > +} > > +#test { > > + width: 100px; height: 100px; > > + background: blue; > > + animation: Opacity0 100s 100s, Opacity1 100s, Width 100s; > > I don't quite understand what this is testing. Wouldn't animation: Opacity1 > 100s 100s, Width 100s still work? Why would another animation winning change > the result? Correct. Hmm, I don't remember why I added the another animation there... > layout/reftests/css-transitions/stacking-context-opacity-lose-to-animation. > html > @@ +34,5 @@ > > + target.style.setProperty("opacity", "1", "important"); > > + // Important style is changed but there is a CSS animation, > > + // so transition does not start and the CSS animation is not effective > > + // due to the important style. As a result opacity style here should > > + // be 100%, so we should not create any stacking context for it. > > Wow, that's a complicated test! > > Do we have a test that checks that if we remove the important style during > the delay phase we update layers accordingly? Great idea! I will add the case, but once we create a stacking context even if we have important style, this kind of tests might be changed. We might need opposite cases, i.e. removing animations.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #68) > > @@ +23,5 @@ > > > +} > > > +#test { > > > + width: 100px; height: 100px; > > > + background: blue; > > > + animation: Opacity0 100s 100s, Opacity1 100s, Width 100s; > > > > I don't quite understand what this is testing. Wouldn't animation: Opacity1 > > 100s 100s, Width 100s still work? Why would another animation winning change > > the result? > > Correct. Hmm, I don't remember why I added the another animation there... I remembered that I did want to check the Opacity0, which is in delay phase, is not painted accidentally.

Unfortunately this patch can not be reviewed yet, because this patch causes a failure in test_animations_omta.html. https://treeherder.mozilla.org/logviewer.html#?job_id=24845186&repo=try#L0-L22301 The failure case is here; http://hg.mozilla.org/mozilla-central/file/4a18b5cacb1b/layout/style/test/test_animations_omta.html#l2249 The animation with fill:forwards in the test case was considered being running on the compositor. I am suspecting getOMTAStyle() returned incorrect state because, if the animation were really running on the compositor, an assertion to check mWinsInCascade in AddAnimationsForProperty() should have hit there.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #71) > Created attachment 8776453 [details] [diff] [review] > Bug 1278136 - Part 5: Create a stacking context for opacity/transform > animations even if it's in delay phase > > Unfortunately this patch can not be reviewed yet, because this patch causes > a failure in test_animations_omta.html. > https://treeherder.mozilla.org/logviewer.html#?job_id=24845186&repo=try#L0- > L22301 > > The failure case is here; > http://hg.mozilla.org/mozilla-central/file/4a18b5cacb1b/layout/style/test/ > test_animations_omta.html#l2249 > > The animation with fill:forwards in the test case was considered being > running on the compositor. I am suspecting getOMTAStyle() returned > incorrect state because, if the animation were really running on the > compositor, an assertion to check mWinsInCascade in > AddAnimationsForProperty() should have hit there. Note that the style obtained by getOMTAStyle() is not animated one. So, getOMTAStyle() is definitely wrong.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #72) > > The animation with fill:forwards in the test case was considered being > > running on the compositor. I am suspecting getOMTAStyle() returned > > incorrect state because, if the animation were really running on the > > compositor, an assertion to check mWinsInCascade in > > AddAnimationsForProperty() should have hit there. > > Note that the style obtained by getOMTAStyle() is not animated one. So, > getOMTAStyle() is definitely wrong. Another note that if I changed the test case to use transform instead of opacity, the test passed. I think we need GetShadowOpacityByAnimation() in LayerComposite as well as SetShadowTransformSetByAnimation().

(In reply to Hiroyuki Ikezoe (:hiro) from comment #73) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #72) > > > > The animation with fill:forwards in the test case was considered being > > > running on the compositor. I am suspecting getOMTAStyle() returned > > > incorrect state because, if the animation were really running on the > > > compositor, an assertion to check mWinsInCascade in > > > AddAnimationsForProperty() should have hit there. > > > > Note that the style obtained by getOMTAStyle() is not animated one. So, > > getOMTAStyle() is definitely wrong. > > Another note that if I changed the test case to use transform instead of > opacity, the test passed. I think we need GetShadowOpacityByAnimation() in > LayerComposite as well as SetShadowTransformSetByAnimation(). I was wrong (though getOMTAStyle is wrong either). We need two different functions in KeyframeEffectReadOnly; 1) HasAnimationOfPropertyForStackingContext() which returns true even if there is an important style 2) HasAnimationOfPropertyForCompositor() which does not return true if there is an important style

his patch introduces two new function named HasAnimationOfPropertyForStackingContext and HasAnimationOfPropertyForCompositor. The former does not check the property is overridden by important style or other animations because we need to create a stacking context for the property even if there is an important style such as 'opacity: 1 ! important', whereas the latter checks the property is overridden because we should not send effects which are overridden by important style.

stacking-context-(transform|opacity)-removing-animation-in-delay.html cause intermittent failures. A rAF callback might be insufficient. https://treeherder.mozilla.org/#/jobs?repo=try&revision=918eb037053e&selectedJob=24930854

Comment on attachment 8776743 [details] [diff] [review] Bug 1278136 - Part 5: Create a stacking context for opacity/transform animations even if it's in delay phase Review of attachment 8776743 [details] [diff] [review]: ----------------------------------------------------------------- I think this is nearly there. However, I'm concerned that the distinction between mWinsInCascade and mIsOverridden and some of the different HasAnimation.../GetAnimation... methods is not clear. I realize this will all be rewritten when we implement additive animations but that might take quite a while so we should try to make this clear and consistent where possible. ::: dom/animation/EffectCompositor.cpp @@ +687,5 @@ > > bool winsInCascade = !animatedProperties.HasProperty(prop.mProperty) && > inEffect; > > + // FIXME: Bug 1216844: One we have effect composition, we should not Nit: s/One/Once/ @@ +689,5 @@ > inEffect; > > + // FIXME: Bug 1216844: One we have effect composition, we should not > + // check animatedProperties if the effect composition is additive or > + // accumulate. Actually I wonder if we should be checking |animatedProperties| at all. If we have three animations A, B, C that all animate transform and run immediately after one another (offset by a delay) then it seems like we should send them all to the compositor--otherwise there might be a gap between when A stops and B starts. That will mean more work on the compositor side but we need to do that anyway. Feel free to make that a separate bug and update the comment here, however. ::: dom/animation/KeyframeEffect.cpp @@ +512,5 @@ > + const nsCSSProperty aProperty) const > +{ > + MOZ_ASSERT(nsCSSProps::PropHasFlags(aProperty, > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > + "The property should be able to run on the compositor."); Why is this a requirement? @@ +515,5 @@ > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > + "The property should be able to run on the compositor."); > + for (const AnimationProperty& property : mProperties) { > + if (nsCSSProps::PropHasFlags(property.mProperty, > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR) && Why do we need this check? If we don't need this check, then it seems like we could just: * Rename GetAnimationOfProperty to GetEffectiveAnimationOfProperty * Rename HasAnimationOfProperty to HasEffectiveAnimationOfProperty * Call this HasAnimationOfProperty and add a comment explaining how it differs from HasEffectiveAnimationOfProperty Or, probably better, add an enum parameter along the lines of enum class OverriddenAnimations { Ignore, Include }; I'm not sure if "overridden" is the correct term here. Looking into it, it's a bit odd that: * HasAnimationOfPropertyForCompositor checks for !property.mIsOverridden * GetAnimationOfProperty checks for property.mWinsInCascade and is used in AddAnimationsForProperty[1] to check for animations that run on the compositor. So, there's a mismatch there? i.e. if an animation is current but not in-effect, HasAnimationOfPropertyForCompositor will return true but GetAnimationOfProperty will return false? Is that right? Would it work to have just: * GetAnimationOfProperty(OverriddenAnimations aOverridenAnimations) -> Checks mIsOverridden * HasAnimationOfProperty(OverriddenAnimations aOverridenAnimations) -> Calls GetAnimationOfProperty as it currently does Would that work for all call sites of HasAnimationOfProperty. [1] http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/layout/base/nsDisplayList.cpp#478 ::: dom/animation/KeyframeEffect.h @@ +151,5 @@ > // **NOTE**: This member is not included when comparing AnimationProperty > // objects for equality. > bool mWinsInCascade = false; > > + // True if the property is overridden by important style or other animations. So, if I understand this correctly: mWinsInCascade = "in effect" && !mIsOverridden Is that right? Can we just drop mWinsInCascade and check for InEffect() && !mIsOverridden or will that cause us to recalculate timing unnecessarily? My concern is that the difference between mWinsInCascade and mIsOverridden is really hard to understand. I *think* we have three things we check: A. Is the animation in effect? B. Is the property already animated by something higher priority? (This includes where we block CSS transitions when CSS animations run on the same property.) C. Is the property overridden by something higher in the cascade? (e.g. !important rules -- we only do this for the animations level of the cascade although I don't remember why) Currently: mWinsInCascade = A && !B && !C In this patch: mIsOverridden = B || C Although, in my comment above, I'm suggesting we should (eventually) just check 'C' and then test for 'B' on the compositor. I wonder how we can make the distinction between mWinsInCascade and mIsOverridden more clear. I think we will rewrite mWinsInCascade significantly when we implement additive animation so perhaps this is ok for now and we just need to add a comment here describing this difference. @@ +155,5 @@ > + // True if the property is overridden by important style or other animations. > + // > + // **NOTE**: This member is not included when comparing AnimationProperty > + // objects for equality. > + // Nit: Drop this blank line. @@ +301,5 @@ > bool HasAnimationOfProperty(nsCSSProperty aProperty) const { > return GetAnimationOfProperty(aProperty) != nullptr; > } > + // Returns true if the effect has a property which needs to create a stacking > + // context. Returns true if the effect includes a property which should trigger the creation of a stacking context if when the effect is current or in-effect (i.e. relevant). @@ +303,5 @@ > } > + // Returns true if the effect has a property which needs to create a stacking > + // context. > + // NOTE: This function does not care about the effect state, i.e. the effect > + // is current or in-effect. We need to update the comment for operator== too, to mention that this member is not compared. @@ +305,5 @@ > + // context. > + // NOTE: This function does not care about the effect state, i.e. the effect > + // is current or in-effect. > + bool HasAnimationOfPropertyForStackingContext( > + const nsCSSProperty aProperty) const; I don't think we need 'const' when passing a parameter per-value. ::: layout/base/nsLayoutUtils.cpp @@ +502,5 @@ > return HasMatchingAnimations(aFrame, > [&aProperty](KeyframeEffectReadOnly& aEffect) > { > + // We don't need to check that the effect is current or in-effect > + // because EffectSet has only current or in-effect animations. Is it worth adding an assertion to check this in case it changes? @@ +516,5 @@ > + return HasMatchingAnimations(aFrame, > + [&aProperty](KeyframeEffectReadOnly& aEffect) > + { > + // We don't need to check that the effect is current or in-effect > + // because EffectSet has only current or in-effect animations. Likewise here? ::: layout/base/nsLayoutUtils.h @@ +2246,5 @@ > static bool HasCurrentTransitions(const nsIFrame* aFrame); > > /** > + * Returns true if the frame has animations which need to create a stacking > + * context. Nit: Returns true if |aFrame| has an animation of |aProperty| that should trigger the creation of a stacking context. This includes animations that are in their delay phase (even if they do not have a backwards fill) since this is the behavior defined by CSS Animations and Web Animations. @@ +2253,5 @@ > nsCSSProperty aProperty); > > /** > + * Returns true if the frame has animations which could be sent the > + * compositor. Nit: Returns true if |aFrame| has an animation of |aProperty| that can be sent to the compositor.

Comment on attachment 8776450 [details] [diff] [review] Bug 1278136 - Part 4: Tests to check stacking context for correct effect in cascading rule v2 Review of attachment 8776450 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I don't understand the comment in layout/reftests/css-animations/stacking-context-opacity-removing-important-in-delay.html / stacking-context-transform-removing-important-in-delay.html. Also, I'm confused by layout/reftests/css-transitions/stacking-context-opacity-lose-to-animation.html and stacking-context-transform-lose-to-animation.html. Didn't we decide that CSS animations overridden by !important rules still create a stacking context? That's what the specs currently say but I'm happy to try to change them if you think that behavior's wrong. Or maybe I'm misunderstanding something? ::: layout/reftests/css-animations/stacking-context-opacity-removing-important-in-delay.html @@ +1,5 @@ > +<!DOCTYPE html> > +<html class="reftest-wait"> > +<title> > +Removing important directive during delay phase of animation creates > +a stack context for correct style Nit: stacking @@ +2,5 @@ > +<html class="reftest-wait"> > +<title> > +Removing important directive during delay phase of animation creates > +a stack context for correct style > +style Unnecessary "style" @@ +35,5 @@ > + // Apply 100% opacity without important directive. > + target.style.setProperty("opacity", "1", ""); > + requestAnimationFrame(() => { > + // Now CSS animation is effective but it's still in delay phase, > + // so we should create a stacking context for 100% opacity style. I think we decided we create a stacking context even if we have an important style rule in place? Is this test still needed? The comment and test title don't seem right. ::: layout/reftests/css-animations/stacking-context-opacity-wins-over-style.html @@ +28,5 @@ > +window.addEventListener("load", () => { > + var target = document.getElementById("test"); > + getComputedStyle(target).opacity; > + > + // CSS animation should win over style, so transition won't be visible during It's not so much that the animation wins over style, but that transitions don't take effect when there is a CSS animation running on the same property, right? ::: layout/reftests/css-animations/stacking-context-transform-removing-important-in-delay.html @@ +2,5 @@ > +<html class="reftest-wait"> > +<title> > +Removing important directive during delay phase of animation creates > +a stack context for correct style > +style Unnecessary "style" @@ +36,5 @@ > + // Apply transform:none without important directive. > + target.style.setProperty("transform", "none", ""); > + requestAnimationFrame(() => { > + // Now CSS animation is effective but it's still in delay phase, > + // so we should create a stacking context for transform:none style. Same question as for stacking-context-opacity-removing-important-in-delay.html. ::: layout/reftests/css-animations/stacking-context-transform-win-in-delay.html @@ +1,5 @@ > +<!DOCTYPE html> > +<title> > +Transform animation winning over another transform animation in delay phase > +creates a stacking context > +phase Nit: unnecessary "phase" ::: layout/reftests/css-animations/stacking-context-transform-wins-over-style.html @@ +28,5 @@ > +window.addEventListener("load", () => { > + var target = document.getElementById("test"); > + getComputedStyle(target).transform; > + > + // CSS animation should win over style, so transition won't be visible during As with the comment on the similar test for opacity, I don't think this is really about winning over style. ::: layout/reftests/css-transitions/stacking-context-opacity-lose-to-animation.html @@ +34,5 @@ > + target.style.setProperty("opacity", "1", "important"); > + // Important style is changed but there is a CSS animation, > + // so transition does not start and the CSS animation is not effective > + // due to the important style. As a result opacity style here should > + // be 100%, so we should not create any stacking context for it. I thought we decided that even if a CSS animation is overridden by an !important rule, it should still create a stacking context while it is relevant? ::: layout/reftests/web-animations/stacking-context-transform-losing-css-animation-in-delay.html @@ +1,4 @@ > +<!DOCTYPE html> > +<title> > +CSS transform animation winning over web animation in delay phase creates > +a stacking context I don't understand, wouldn't either trigger stacking context creation? Or is the point that a CSS transform animation with transform:none that wins creates a stacking context? I'm not sure why we would expect the script animation to have any effect here.

(In reply to Brian Birtles (:birtles) from comment #78) > Also, I'm confused by > layout/reftests/css-transitions/stacking-context-opacity-lose-to-animation. > html and stacking-context-transform-lose-to-animation.html. Didn't we decide > that CSS animations overridden by !important rules still create a stacking > context? That's what the specs currently say but I'm happy to try to change > them if you think that behavior's wrong. Or maybe I'm misunderstanding > something? I am sorry, I forgot to fix comments in the test file. Actually those test files no longer compares with no-stacking-context-ref.html. Now I realized a confusing point is that some of these test cases check that *correct* style is painted with a stacking context. For example, a CSS animation and a web animation with delay. I should rename them to proper names. > @@ +35,5 @@ > > + // Apply 100% opacity without important directive. > > + target.style.setProperty("opacity", "1", ""); > > + requestAnimationFrame(() => { > > + // Now CSS animation is effective but it's still in delay phase, > > + // so we should create a stacking context for 100% opacity style. > > I think we decided we create a stacking context even if we have an important > style rule in place? Is this test still needed? The comment and test title > don't seem right. The purpose of this test is to check that we don't paint incorrect style accidentally when removing important style in delay phase. If there is an important style for a property, an animation of the same property is not running, after removing the important style, then the animation starts running. I think this case will be important when we send delayed animations to the compositor. It might be passed trivially, but it's not trivial for me. > layout/reftests/css-animations/stacking-context-opacity-wins-over-style.html > @@ +28,5 @@ > > +window.addEventListener("load", () => { > > + var target = document.getElementById("test"); > > + getComputedStyle(target).opacity; > > + > > + // CSS animation should win over style, so transition won't be visible during > > It's not so much that the animation wins over style, but that transitions > don't take effect when there is a CSS animation running on the same > property, right? Right. > ::: > layout/reftests/css-animations/stacking-context-transform-wins-over-style. > html > @@ +28,5 @@ > > +window.addEventListener("load", () => { > > + var target = document.getElementById("test"); > > + getComputedStyle(target).transform; > > + > > + // CSS animation should win over style, so transition won't be visible during > > As with the comment on the similar test for opacity, I don't think this is > really about winning over style. I am wondering we don't say CSS animations win normal style? Because CSS animations are always effective even if there are any style other than "important"?

(In reply to Brian Birtles (:birtles) from comment #77) > @@ +689,5 @@ > > inEffect; > > > > + // FIXME: Bug 1216844: One we have effect composition, we should not > > + // check animatedProperties if the effect composition is additive or > > + // accumulate. > > Actually I wonder if we should be checking |animatedProperties| at all. If > we have three animations A, B, C that all animate transform and run > immediately after one another (offset by a delay) then it seems like we > should send them all to the compositor--otherwise there might be a gap > between when A stops and B starts. > > That will mean more work on the compositor side but we need to do that > anyway. > > Feel free to make that a separate bug and update the comment here, however. Ah, right. I will open a new bug for it. I guess it will be fixed after bug 1216844 or during working bug 1216844. > ::: dom/animation/KeyframeEffect.cpp > @@ +512,5 @@ > > + const nsCSSProperty aProperty) const > > +{ > > + MOZ_ASSERT(nsCSSProps::PropHasFlags(aProperty, > > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > > + "The property should be able to run on the compositor."); > > Why is this a requirement? I want to prevent from being the function called for other purpose/other properties. Actually KeyframeEffectReadOnly::HasAnimationOfProperty() is called for background position properties, it actually does not matter, but HasAnimationOfProperty depends on mWinsInCascade flag even the purpose of the flag is only for OMTA. I don't know yet how mWinsInCascade flag affects background position animations. This is a reason why I can't drop/change mWinsInCase and related functions, i.e. HasAnimationOfProperty or GetAnimationOfProperty. I'd like to leave them as it is until I really understand layerization of background position animations. > @@ +515,5 @@ > > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > > + "The property should be able to run on the compositor."); > > + for (const AnimationProperty& property : mProperties) { > > + if (nsCSSProps::PropHasFlags(property.mProperty, > > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR) && > > Why do we need this check? The same reason as I commented above. > If we don't need this check, then it seems like we could just: > > * Rename GetAnimationOfProperty to GetEffectiveAnimationOfProperty > * Rename HasAnimationOfProperty to HasEffectiveAnimationOfProperty > * Call this HasAnimationOfProperty and add a comment explaining how it > differs from HasEffectiveAnimationOfProperty > > Or, probably better, add an enum parameter along the lines of > > enum class OverriddenAnimations { > Ignore, > Include > }; > > I'm not sure if "overridden" is the correct term here. Looking into it, it's > a bit odd that: > > * HasAnimationOfPropertyForCompositor checks for !property.mIsOverridden > * GetAnimationOfProperty checks for property.mWinsInCascade and is used in > AddAnimationsForProperty[1] to check for animations that run on the > compositor. > > So, there's a mismatch there? i.e. if an animation is current but not > in-effect, HasAnimationOfPropertyForCompositor will return true but > GetAnimationOfProperty will return false? Is that right? Right. The mismatch blocks that we send the animation to the compositor. It will be changed in bug 1223658. Honestly I have not touched bug 1223658 for a white, I'd like to re-visit bug 1223658 after this bug is *completely* fixed. This bug has changed a lot from the beginning and is long enough, I might need a new glue bug between them. > ::: dom/animation/KeyframeEffect.h > @@ +151,5 @@ > > // **NOTE**: This member is not included when comparing AnimationProperty > > // objects for equality. > > bool mWinsInCascade = false; > > > > + // True if the property is overridden by important style or other animations. > > So, if I understand this correctly: > > mWinsInCascade = "in effect" && !mIsOverridden > > Is that right? Can we just drop mWinsInCascade and check for InEffect() && > !mIsOverridden or will that cause us to recalculate timing unnecessarily? > > My concern is that the difference between mWinsInCascade and mIsOverridden > is really hard to understand. The difference is exactly what I don't understand about layerization of background position animations. I think it just checks *in-play* animations but I am not convinced yet. > I *think* we have three things we check: > > A. Is the animation in effect? > B. Is the property already animated by something higher priority? (This > includes where we block CSS transitions when CSS animations run on the same > property.) > C. Is the property overridden by something higher in the cascade? (e.g. > !important rules -- we only do this for the animations level of the cascade > although I don't remember why) > > Currently: > > mWinsInCascade = A && !B && !C > > In this patch: > > mIsOverridden = B || C > > Although, in my comment above, I'm suggesting we should (eventually) just > check 'C' and then test for 'B' on the compositor. > > I wonder how we can make the distinction between mWinsInCascade and > mIsOverridden more clear. I think we will rewrite mWinsInCascade > significantly when we implement additive animation so perhaps this is ok for > now and we just need to add a comment here describing this difference. Thanks. I will add comments there. Actually I want to drop mWinsInCascade, but before do that, I think we should clarify how nsLayoutUtils::HasCurrentAnimationOfProperty in ActiveLayerTracker::IsStyleAnimated affects layerization. That is the last place the caller of nsLayoutUtils::HasCurrentAnimationOfProperty. The background position animation was checked in this June. Yes it's while I've been working on this bug. I'd want to avoid similar situations, so I'd like to finish this bug anyway.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #80) > > ::: dom/animation/KeyframeEffect.cpp > > @@ +512,5 @@ > > > + const nsCSSProperty aProperty) const > > > +{ > > > + MOZ_ASSERT(nsCSSProps::PropHasFlags(aProperty, > > > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > > > + "The property should be able to run on the compositor."); > > > > Why is this a requirement? > > I want to prevent from being the function called for other purpose/other > properties. Actually KeyframeEffectReadOnly::HasAnimationOfProperty() is > called for background position properties, it actually does not matter, but > HasAnimationOfProperty depends on mWinsInCascade flag even the purpose of > the flag is only for OMTA. I don't know yet how mWinsInCascade flag affects > background position animations. This is a reason why I can't drop/change > mWinsInCase and related functions, i.e. HasAnimationOfProperty or > GetAnimationOfProperty. I'd like to leave them as it is until I really > understand layerization of background position animations. I think we should investigate that. I don't want to make this code *more* confusing by adding extra flags and methods that are subtly different just because we don't know how existing code works.

I am stuck on removing winsInCascade flag. After landing bug 1049975, it makes KeyFrameEffectReadOnly::CanThrottle() being called from Animation::SetEffectNoUpdate() without creating a new EffectSet. As a result I am hit by an assertion in CanThrottle()[1]. [1] http://hg.mozilla.org/mozilla-central/file/1a5b53a831e5/dom/animation/KeyframeEffect.cpp#l921 Apparently the assertion on current our code depends on a fact that winsInCascade flag is initially false (we bail out the loop in CanThrottle() just before the assertion is winsInCascade is false).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #82) > I am stuck on removing winsInCascade flag. After landing bug 1049975, it > makes KeyFrameEffectReadOnly::CanThrottle() being called from > Animation::SetEffectNoUpdate() without creating a new EffectSet. As a > result I am hit by an assertion in CanThrottle()[1]. > > [1] > http://hg.mozilla.org/mozilla-central/file/1a5b53a831e5/dom/animation/ > KeyframeEffect.cpp#l921 > > Apparently the assertion on current our code depends on a fact that > winsInCascade flag is initially false (we bail out the loop in CanThrottle() > just before the assertion is winsInCascade is false). Oops, sorry. I did not check the failure case. The assertion was hit in case when setting null effect. So we should add a null effect check somewhere.

Thank you, Boris, for taking care of both of those bugs! I need your help here too. I'd like to drop mWinsInCascade flag in this bug, (to be precise it will be replaced by another flag), but before doing it, I'd like to clarify the effect of ResetWinsInCascade() in KeyframeEffect::SetTarget(). Do you know we have a test case which fails if we don't call ResetWinsInCascade() in SetTarget()? If there is no such test cases, I am going to write it first.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #84) > Thank you, Boris, for taking care of both of those bugs! > > I need your help here too. I'd like to drop mWinsInCascade flag in this > bug, (to be precise it will be replaced by another flag), but before doing > it, I'd like to clarify the effect of ResetWinsInCascade() in > KeyframeEffect::SetTarget(). Do you know we have a test case which fails if > we don't call ResetWinsInCascade() in SetTarget()? If there is no such test > cases, I am going to write it first. I think we don't have any test for ResetWinsInCascade() because I only added tests for basic cases, ResetOnRunningCompositor(), and mutation observers. If you have time to write the tests for ResetWinsInCascade(), it'd be great! Thank you!

Thank you, Boris! I will figure out how to write the case.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #82) > I am stuck on removing winsInCascade flag. After landing bug 1049975, it > makes KeyFrameEffectReadOnly::CanThrottle() being called from > Animation::SetEffectNoUpdate() without creating a new EffectSet. As a > result I am hit by an assertion in CanThrottle()[1]. Hi, Hiro, Could you please give me your patch which can trigger this assertion because I cannot reproduce the empty EffectSet assertion on current m-c. (You can put it on bug 1298742, bug 1298739, or others). Thank you very much.

(In reply to Boris Chiou [:boris] from comment #87) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #82) > > I am stuck on removing winsInCascade flag. After landing bug 1049975, it > > makes KeyFrameEffectReadOnly::CanThrottle() being called from > > Animation::SetEffectNoUpdate() without creating a new EffectSet. As a > > result I am hit by an assertion in CanThrottle()[1]. > > Hi, Hiro, > Could you please give me your patch which can trigger this assertion because > I cannot reproduce the empty EffectSet assertion on current m-c. (You can > put it on bug 1298742, bug 1298739, or others). Thank you very much. This is it. Though this is still broken something (background-position-in-delay.html test fails with this patch), but it can be used to check that symptom. A point is that, on our current code, HasAnimationOfProperty() in CanThrottle() checks mWinsInCascade flag so the assertion is not hit at all. I guess you can also see the assertion if you move the assertion just before HasAnimationOfProperty().

(In reply to Hiroyuki Ikezoe (:hiro) from comment #88) > Created attachment 8785833 [details] [diff] [review] > a WIP patch > > This is it. Though this is still broken something > (background-position-in-delay.html test fails with this patch), but it can > be used to check that symptom. A point is that, on our current code, > HasAnimationOfProperty() in CanThrottle() checks mWinsInCascade flag so the > assertion is not hit at all. I guess you can also see the assertion if you > move the assertion just before HasAnimationOfProperty(). Thanks! I can reproduce it now.

Now I can come back here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=299e44459160

Fixed the result in case of animations overridden by !important rule, it should create a stacking context now. Also fixed couple of comments.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #90) > Now I can come back here. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=299e44459160 I am sorry, this try included patches for bug 1305325, but it should not affect of this.

Comment on attachment 8798317 [details] [diff] [review] Part 4: Tests to check stacking context for correct effect in cascading rule v3 Review of attachment 8798317 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/css-animations/stacking-context-opacity-removing-important-in-delay.html @@ +11,5 @@ > + position: fixed; > + background: green; > + top: 50px; > +} > +@keyframes Opaque0 { Call this Opacity0 like we do in no-stacking-context-opacity-removing-animation-in-delay.html ?

Comment on attachment 8798318 [details] [diff] [review] Bug 1278136 - Part 5: Create a stacking context for opacity/transform animations even if it's in delay phase v2 Review of attachment 8798318 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: dom/animation/KeyframeEffectReadOnly.cpp @@ +1020,5 @@ > // occasionally unthrottle these animations even if the animations are > // already running on compositor. > for (const LayerAnimationInfo::Record& record : > LayerAnimationInfo::sRecords) { > + // Skip properties that are overridden by important rules. Nit: s/important/!important/ ::: dom/animation/KeyframeEffectReadOnly.h @@ +241,5 @@ > void SetKeyframes(JSContext* aContext, JS::Handle<JSObject*> aKeyframes, > ErrorResult& aRv); > void SetKeyframes(nsTArray<Keyframe>&& aKeyframes, > nsStyleContext* aStyleContext); > + // Returns AnimationProperty corresponding to a given CSS property Nit: Add a space before this line. @@ +256,5 @@ > + > + // Returns true if the effect includes |aProperty| which is not > + // overridden by !important rules. > + // NOTE: We don't currently check for !important rules for properties that > + // can't run on the compositor. We could probably combine the comments for GetEffectiveAnimationOfProperty and HasEffectAnimationOfProperty. @@ +257,5 @@ > + // Returns true if the effect includes |aProperty| which is not > + // overridden by !important rules. > + // NOTE: We don't currently check for !important rules for properties that > + // can't run on the compositor. > + bool HasEffectiveAnimationOfProperty(nsCSSPropertyID aProperty) const This should probably go together with GetEffectiveAnimationOfProperty. Perhaps an order like: * HasAnimationOfProperty * HasEffectiveAnimationOfProperty * GetEffectiveAnimationOfProperty

Part 5 patch makes layout/reftests/bugs/395107-2.html perma fail. https://treeherder.mozilla.org/logviewer.html#?job_id=28765981&repo=try Attaching image is the difference between test and reference images. The difference is a pixel on both right corners. Oddly part5 should affect only animations but there is no animation in neither the test file nor the reference one. I don't really understand what happened.

bz, you are the author of the test case (395107-2.html), I'd like to add fuzzy-if for the test case if it's OK with you.

That's OK in general, but I agree that it's really odd that part 5 affects this. Is Fennec doing some opacity/transform animations on all <select> elements or something? Might be good to get that figured out...

Thank you Boris for the advises. I'd been tracking down why the test fails for a while I could reproduce the failure locally on emulator, and also confirmed there is no animations on the select element while running the test case. What I am really surprised is that the key changeset which causes the failure is part 4 actually, not part 5. Here is a try with part 4 without part 5. The test failed! https://treeherder.mozilla.org/#/jobs?repo=try&revision=dabea9b50f42 I really really don't understand what's going on there. Part 4 is just adding bunch of reftests and those of tests does not belong to the same group of the failure test. Filed bug 1309151 for the issue. So, although I don't understand what happens here, I am going to add fuzzy-if here.

Comment on attachment 8798824 [details] [diff] [review] Part 6: Add fuzzy-if on Android Brian, can you please stamp r+ on behalf of bz? bz currently does not accept any review request. Here is a try with this patch (only run the reftest group including the failure case). https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b894e0cd40c

> the key changeset which causes the failure is part 4 actually, not part 5 That's much more plausible, actually! This could be down to the harness reusing canvases, or GPU or graphics driver issues with reusing memory or textures or something. At least I can see plausible mechanisms for causing a slight color difference! I agree that for now the fuzzy-if is the way to go.

Comment on attachment 8798824 [details] [diff] [review] Part 6: Add fuzzy-if on Android r=me, but please include this in part 4, since that's where it would be needed, and add the commit message to that one accordingly (probably with some of the info from comment 99).

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #101) > > the key changeset which causes the failure is part 4 actually, not part 5 > > That's much more plausible, actually! > > This could be down to the harness reusing canvases, or GPU or graphics > driver issues with reusing memory or textures or something. At least I can > see plausible mechanisms for causing a slight color difference! Wow! What a great insight! Final try just in case. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff8ec9ed4ff2151e6c99e9484b39ae8a62e94f14 Thank you so much Boris!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f2db29e67ca5e8542675bf41b795eb107fda262 Bug 1278136 - Part 4: Tests to check stacking context for correct effect in cascading rule. r=birtles, bz https://hg.mozilla.org/integration/mozilla-inbound/rev/62cf4a7d6007e3f0b1da0f0590c81a17791ec898 Bug 1278136 - Part 5: Create a stacking context for opacity/transform animations even if it's in delay phase and even if the property is overridden by !important rules. r=birtles

Backed out part 4 and part 5 in https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf29e912685ecd840014334094ba5be7d9ef274 for Windows debug (and, if you're willing to look at hidden jobs, Linux64 debug) failures in no-stacking-context-opacity-removing-animation-in-delay.html like https://treeherder.mozilla.org/logviewer.html#?job_id=37428678&repo=mozilla-inbound

Oh! I did not notice the failure was hidden on the try result.

Yes, lots of failure in what we actually let you see on try, since on non-try trees we are running Windows reftests on VMs, and on try we run them on hardware by default, and only give you the actual runs that you need to pass if you specifically ask for, e.g., reftest[Windows 7], bug 1303006, plus for both Windows XP and Windows 8 we intentionally don't run them unless you specifically ask for them.

Thanks Phil. I will be careful hidden failures from now on. As for the failure test, it seems that there is a race condition that reftest harness takes a snapshot before removal animation is painted, as a result stacking context for the animation is still there. I will figure out how to avoid it.

Brian, could you please re-review changes for no-stacking-context-(opacity|transform)-removing-animation-in-delay.html? We needed to wait MozAfterPaint instead of rAF callbacks for these tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3348a4c97647&exclusion_profile=false

The test case is included in 'R4' on the try.

Comment on attachment 8800530 [details] [diff] [review] Bug 1278136 - Part 5: Create a stacking context for opacity/transform animations even if it's in delay phase v3 Review of attachment 8800530 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/css-animations/no-stacking-context-opacity-removing-animation-in-delay.html @@ +28,5 @@ > target.style.animation = "Opacity0 100s 100s"; > + > + // We need to wait MozAfterPaint instead of requestAnimationFrame because > + // with rAF callbacks sometimes we recieve unexpected result in the second > + // rAF callback due to compositor delay. "We need to wait for MozAfterPaint instead of requestAnimationFrame to ensure the stacking context has been updated (removed) on the compositor before we snapshot." ? ::: layout/reftests/css-animations/no-stacking-context-transform-removing-animation-in-delay.html @@ +28,5 @@ > target.style.animation = "TransformNone 100s 100s"; > > + // We need to wait MozAfterPaint instead of requestAnimationFrame because > + // with rAF callbacks sometimes we recieve unexpected result in the second > + // rAF callback due to compositor delay. Likewise here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2782494a3f16f3bdeebf77e46ee12243696a9bbd Bug 1278136 - Part 4: Tests to check stacking context for correct effect in cascading rule. r=birtles, bz https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0801b220f3b57363b5c7ed2946295433dd125d Bug 1278136 - Part 5: Create a stacking context for opacity/transform animations even if it's in delay phase and even if the property is overridden by !important rules. r=birtles

Thank you Brian for quick review. I should note here about the failed test on Android. Surprisingly, though I did not notice at all, but the test case begun to fail after backing part 4 out. See bug 1309533.

https://hg.mozilla.org/mozilla-central/rev/2782494a3f16 https://hg.mozilla.org/mozilla-central/rev/fd0801b220f3