According to a recent spec change[1] and its discussion[2], we should create a stacking context even if the transform style is not effectively applied at some point. e.g., @keyframes { 0% { transform: 'none'; } 99% { transform: 'none'; } 100% { transform: translateX(100px); } } To create a stacking context for those transform animations we should check that the nsIFrame has animation is current effect state apart from HasTransform() check in nsFrame::Init()[3]. [1] https://github.com/w3c/csswg-drafts/commit/d107d5e7b14fd944378da5664394380537b088f3 [2] https://lists.w3.org/Archives/Public/www-style/2016May/0058.html [3] https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/layout/generic/nsFrame.cpp#557

To :mattwoodrow To do such check in nsFrame::Init() makes sense to you?

To :dbaron I think nsLayoutUtils::HasCurrentAnimationOfProperty() is a suitable function to check that nsIFrame has an animation which is in current but it can't be used in nsFrame::Init() for now. That's because, EffectSet::GetEffectSet(const nsIFrame*), which is called by nsLayoutUtils::HasCurrentAnimationOfProperty(), uses nsLayoutUtils::GetStyleFrame(const nsIContent*) to check the frame is a primary frame, but in nsFrame::Init() the primary frame is not yet set at that point. In the particular case of bug 1245075, nsLayoutUtils::GetStyleFrame(nsIFrame* aFrame) seems sufficient to me. Is there any cases we can't use nsLayoutUtils::GetStyleFrame(nsIFrame* aFrame) there in your mind? If there is, we should set NS_FRAME_MAY_BE_TRANSFORMED after frame construction.

This patch has two test cases. @keyframes { from, to { transform: none; } } @keyframes { 0% { transform: none; } 99% { transform: none; } 100% { transform: translateX(0px); } } Both of them should create a stacking context.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > To :mattwoodrow > To do such check in nsFrame::Init() makes sense to you? Doing so in Init seems fine, but will we reconstruct the frame if it changes?

(In reply to Matt Woodrow (:mattwoodrow) from comment #4) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > > To :mattwoodrow > > To do such check in nsFrame::Init() makes sense to you? > > Doing so in Init seems fine, but will we reconstruct the frame if it changes? In case of CSS animations, I think so. I will handle web animations case in bug 1223658 or a later bug.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > I think nsLayoutUtils::HasCurrentAnimationOfProperty() is a suitable > function to check that nsIFrame has an animation which is in current but it > can't be used in nsFrame::Init() for now. That's because, > EffectSet::GetEffectSet(const nsIFrame*), which is called by > nsLayoutUtils::HasCurrentAnimationOfProperty(), uses > nsLayoutUtils::GetStyleFrame(const nsIContent*) to check the frame is a > primary frame, but in nsFrame::Init() the primary frame is not yet set at > that point. In the particular case of bug 1245075, > nsLayoutUtils::GetStyleFrame(nsIFrame* aFrame) seems sufficient to me. Is > there any cases we can't use nsLayoutUtils::GetStyleFrame(nsIFrame* aFrame) > there in your mind? If there is, we should set NS_FRAME_MAY_BE_TRANSFORMED > after frame construction. Instead, I think it probably makes more sense to change the test that EffectSet::GetEffectSet(nsIFrame*) to use aFrame->StyleContext()->GetPseudoType() instead of testing IsGeneratedContentFrame, content->NodeInfo()->NameAtom(), and nsLayoutUtils::GetStyleFrame. This matches the older code that predated it in nsTransitionManager::GetElementTransitions and nsAnimationManager::GetElementAnimations, and I think it should be simpler. Then again, I'm not sure what led to the current pattern (nor does bug 1226118 comment 1 help); perhaps you should ask birtles. This would still fix bug 1245075 since the GetPseudoType() for a table outer frame should (I think) be CSSPseudoElementType::AnonBox. With that change, there wouldn't be a problem calling it in nsFrame::Init.

Thank you, dbaron! According to bug 1226118 comment 17, the code came from GetAnimationCollection which was initially checked in https://hg.mozilla.org/mozilla-central/rev/fa343e461c43 . Hello Lee, do you remember the reason why you chose (nsIContent*)->NodeInfo()->NameAtom() there? As far as I tried a run with aFrame->StyleContext()->GetPseudoType() changes, it works very well. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b509e9323347 Brian, do you have any particular concerns in case of web animations? Oops, now Brian does not accept ni?, I will ping him later.

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #6) > Instead, I think it probably makes more sense to change the test that > EffectSet::GetEffectSet(nsIFrame*) to use > aFrame->StyleContext()->GetPseudoType() instead of testing > IsGeneratedContentFrame, content->NodeInfo()->NameAtom(), and > nsLayoutUtils::GetStyleFrame. This matches the older code that predated it > in nsTransitionManager::GetElementTransitions and > nsAnimationManager::GetElementAnimations, and I think it should be simpler. > Then again, I'm not sure what led to the current pattern (nor does bug > 1226118 comment 1 help); perhaps you should ask birtles. As hiro points out, that code is taken from the implementation of CommonAnimationManager::GetAnimationCollection present at that time which Lee will know much more about.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7) > Thank you, dbaron! > > According to bug 1226118 comment 17, the code came from > GetAnimationCollection which was initially checked in > https://hg.mozilla.org/mozilla-central/rev/fa343e461c43 . > > Hello Lee, do you remember the reason why you chose > (nsIContent*)->NodeInfo()->NameAtom() there? > > As far as I tried a run with aFrame->StyleContext()->GetPseudoType() > changes, it works very well. > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b509e9323347 > > Brian, do you have any particular concerns in case of web animations? > > Oops, now Brian does not accept ni?, I will ping him later. It's quite a while ago, so I don't remember much as far as details. I think it was simply because that's how most of the other code was doing things at the time.

Thank you, Lee. Then I will change it to the code dbaron suggested. Brian, would you mind reviewing the patch? I am going to change EffectCompositor::UpdateCascadeResults() and use it in EffectSet::GetEffect(). https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b2a4b45ce9

Before this patch, we could't use EffectSet::GetEffectSet(nsIFrame*) until the target content associated with the nsIFrame has a primary frame since nsLayoutUtils::GetStyleFrame(nsIContent*) needs the primary frame. In this patch, StyleContext()->GetPseudoType() is used for obtaining CSSPseudoElementType instread of content->NodeInfo()->NameAtom(). As a result, we don't need to care about whether the content has a primary frame or not. Review commit: https://reviewboard.mozilla.org/r/56774/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56774/

To create stacking context for transform animations whose style is transform:none, we need to check the frame has transform animations regardless of current transform style. Review commit: https://reviewboard.mozilla.org/r/56776/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56776/

This patch has two test cases for CSS animations and once test case for web animations. For CSS animations test cases: @keyframes { from, to { transform: none; } } @keyframes { 0% { transform: none; } 99% { transform: none; } 100% { transform: translateX(0px); } } Both of them should create a stacking context. For web animtions test cases, the target element is appended after animate(). Review commit: https://reviewboard.mozilla.org/r/56778/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56778/

The implementation of nsLayoutUtils::GetAnimationContent has been dropped in bug 771367. Review commit: https://reviewboard.mozilla.org/r/56780/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56780/

Comment on attachment 8758509 [details] MozReview Request: Bug 1273042 - Part 2: Create stacking context for transform animations whose style is transform:none. r?mattwoodrow https://reviewboard.mozilla.org/r/56776/#review53498

Comment on attachment 8758511 [details] MozReview Request: Bug 1273042 - Part 4: Drop nsLayoutUtils::GetAnimationContent declaration. r?birtles. https://reviewboard.mozilla.org/r/56780/#review53500

Comment on attachment 8758508 [details] MozReview Request: Bug 1273042 - Part 1: Use StyleContext()->GetPseudoType() to obtain CSSPseudoElementType for the nsIFrame. r?birtles https://reviewboard.mozilla.org/r/56774/#review53502 ::: dom/animation/EffectSet.cpp:52 (Diff revision 1) > } > > /* static */ EffectSet* > EffectSet::GetEffectSet(const nsIFrame* aFrame) > { > - nsIContent* content = aFrame->GetContent(); > + Maybe<NonOwningAnimationTarget> pseudoElement = Can we call this |target|, perhaps? pseudoElement sounds a bit like it is always a pseudo element. ::: dom/animation/EffectSet.cpp:63 (Diff revision 1) > - return static_cast<EffectSet*>(content->GetProperty(propName)); > + nsIAtom* propName = GetEffectSetPropertyAtom(pseudoElement->mPseudoType); > + > + return static_cast<EffectSet*>( > + pseudoElement->mElement->GetProperty(propName)); I guess we could even just write: return GetEffectSet(pseudoElement->mElement, pseudoElement->mPseudoType);

Comment on attachment 8758510 [details] MozReview Request: Bug 1273042 - Part 3: Reftest for checking transform:none animations create a stacking context. r?birtles https://reviewboard.mozilla.org/r/56778/#review53504 ::: layout/reftests/css-animations/stacking-context-transform-none-animation.html:2 (Diff revision 1) > +<!DOCTYPE html> > +<title>Transform animation creates a stacking context even it has only 'transform:none' keyframes</title> Nit: even though it ::: layout/reftests/css-animations/stacking-context-transform-on-none-keyframe.html:2 (Diff revision 1) > +<!DOCTYPE html> > +<title>Transform animation creates a stacking context even it's in 'transform:none' keyframe</title> Nit: ... even between 'transform:none' keyframes ::: layout/reftests/css-animations/stacking-context-transform-on-none-keyframe.html:11 (Diff revision 1) > +@keyframes TransformNone { > + 0% { transform: none; } > + 99% { transform: none; } > + 100% { transform: translateX(0px); } > +} > +#test { > + width: 100px; height: 100px; > + background: blue; > + animation: TransformNone 100s infinite; My first thought was that this is a bit unusual to have infinite iterations. If we think the test could take longer than 100s, then it could also take 99.5s and fail intermittently. So we should probably just drop the 'infinite'. However, my second thought was that I'm not really sure why this test is needed. If the first test passes, is there any reason why this test would not pass? ::: layout/reftests/web-animations/stacking-context-transform-none-animation-before-appending-element.html:3 (Diff revision 1) > +<!DOCTYPE html> > +<html class="reftest-wait"> > +<title>Transform animation which is initially not associted with any document creates a stacking context even if it has only 'transform:none' in its keyframe</title> Nit: associated Nit: Transform animation whose target is not initially...

Comment on attachment 8758508 [details] MozReview Request: Bug 1273042 - Part 1: Use StyleContext()->GetPseudoType() to obtain CSSPseudoElementType for the nsIFrame. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56774/diff/1-2/

Comment on attachment 8758509 [details] MozReview Request: Bug 1273042 - Part 2: Create stacking context for transform animations whose style is transform:none. r?mattwoodrow Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56776/diff/1-2/

Comment on attachment 8758510 [details] MozReview Request: Bug 1273042 - Part 3: Reftest for checking transform:none animations create a stacking context. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56778/diff/1-2/

https://reviewboard.mozilla.org/r/56774/#review53502 > I guess we could even just write: > > return GetEffectSet(pseudoElement->mElement, pseudoElement->mPseudoType); Thanks! I did not notice the function just above there.

https://reviewboard.mozilla.org/r/56778/#review53504 > My first thought was that this is a bit unusual to have infinite iterations. If we think the test could take longer than 100s, then it could also take 99.5s and fail intermittently. So we should probably just drop the 'infinite'. > > However, my second thought was that I'm not really sure why this test is needed. If the first test passes, is there any reason why this test would not pass? Ah, right. I just drop the test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c0af860e07e8bb805f591a4d85a25416d9cd85 Bug 1273042 - Part 1: Use StyleContext()->GetPseudoType() to obtain CSSPseudoElementType for the nsIFrame. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/df16848a29e356152b3374344a4e279a0dc05235 Bug 1273042 - Part 2: Create stacking context for transform animations whose style is transform:none. r=mattwoodrow https://hg.mozilla.org/integration/mozilla-inbound/rev/8976b91b9ad2bbb143a204adf07565d23cea1f3b Bug 1273042 - Part 3: Reftest for checking transform:none animations create a stacking context. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/958fde8511872c5fbf59c3b27a120fbb31a6f9ae Bug 1273042 - Part 4: Drop nsLayoutUtils::GetAnimationContent declaration. r=birtles.

https://hg.mozilla.org/mozilla-central/rev/d3c0af860e07 https://hg.mozilla.org/mozilla-central/rev/df16848a29e3 https://hg.mozilla.org/mozilla-central/rev/8976b91b9ad2 https://hg.mozilla.org/mozilla-central/rev/958fde851187