Bug 1527210 - Be more consistent about only applying transforms to primary frames; r=hiro
authorBrian Birtles <birtles@gmail.com>
Tue, 05 Mar 2019 03:09:19 +0000
changeset 520213 5cf9e494f69061f8b31fde9ba7dc2108877f1650
parent 520212 71df482164471a59a54d9531e34dedec05bb9b70
child 520214 211f6573535bdc2721960b2e1339ba9069a2ae4e
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1527210, 722777, 816458
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1527210 - Be more consistent about only applying transforms to primary frames; r=hiro For display:table content we generate two frames: a table wrapper frame and an inner table frame. The styles are applied to the inner frame (referred to as the style frame), whilst the wrapper frame is the primary frame for the content. However, in order to make tables with transforms behave as a container for abspos/fixed-pos content as required by the spec, we apply the transform to the wrapper frame (bug 722777) by inheriting the transform from inner to wrapper and then ignoring the transform on the inner frame (bug 722777 and bug 816458). When handling animations on table elements we need to be careful of this distinction. in particular, css animations[1] and web animations[2] require that when we have an unfinished transform animation targetting an element, the element acts as if it had `will-change: transform` applied and therefore generates a stacking context. As a result we need to accurately detect when a frame should be considered as having transform animations applied to it or not for the purpose of creating a stacking context. Previously our handling of display:table content was quite inconsistent and contradictory. For example, `nsIFrame::HasAnimationOfTransform` would check for a primary frame AND for animations on that frame, despite the fact that we only ever store animations on the style frame. As a result it could never return true for either a table wrapper or inner table frame. This patch attempts to make this handling at least a little more consistent, producing the following result: Outer table frame (primary frame): nsIFrame::IsTransformed → true nsIFrame::IsCSSTransformed → true nsIFrame::HasAnimationOfTransform → true nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → false Inner table frame (style frame): nsIFrame::IsTransformed → false nsIFrame::IsCSSTransformed → false nsIFrame::HasAnimationOfTransform → false nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → true We maintain that the NS_FRAME_MAY_BE_TRANSFORMED bit is only set on the primary frame whilst the mMayHaveTransformAnimation flag is only set on the style frame. Note that we don't simply always put everything on the primary frame because for other property types (e.g. opacity) the default setup of putting all styles and animations on the style frame is simpler and correct. So far it is only transforms that require special handling to apply the effect to the wrapper frame. This patch adds a reftest that fails without the code changes included in this patch. [1] https://drafts.csswg.org/css-animations/#animations [2] https://drafts.csswg.org/web-animations-1/#side-effects-section Differential Revision: https://phabricator.services.mozilla.com/D21883
layout/base/RestyleManager.cpp
layout/base/nsLayoutUtils.cpp
layout/base/nsLayoutUtils.h
layout/generic/nsFrame.cpp
layout/reftests/web-animations/reftest.list
layout/reftests/web-animations/stacking-context-animation-on-table-ref.html
layout/reftests/web-animations/stacking-context-transform-changing-effect-on-table.html
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -1073,36 +1073,39 @@ static void DoApplyRenderingChangeToTree
 
       ActiveLayerTracker::NotifyRestyle(aFrame, eCSSProperty_opacity);
       if (nsSVGIntegrationUtils::UsingEffectsForFrame(aFrame)) {
         // SVG effects paints the opacity without using
         // nsDisplayOpacity. We need to invalidate manually.
         aFrame->InvalidateFrameSubtree();
       }
     }
+    nsIFrame* primaryFrame =
+        nsLayoutUtils::GetPrimaryFrameFromStyleFrame(aFrame);
     if ((aChange & nsChangeHint_UpdateTransformLayer) &&
-        aFrame->IsTransformed()) {
+        primaryFrame->IsTransformed()) {
       // Note: All the transform-like properties should map to the same
       // layer activity index, so does the restyle count. Therefore, using
       // eCSSProperty_transform should be fine.
       ActiveLayerTracker::NotifyRestyle(aFrame, eCSSProperty_transform);
       // If we're not already going to do an invalidating paint, see
       // if we can get away with only updating the transform on a
       // layer for this frame, and not scheduling an invalidating
       // paint.
       if (!needInvalidatingPaint) {
         nsDisplayItem::Layer* layer;
-        needInvalidatingPaint |= !aFrame->TryUpdateTransformOnly(&layer);
+        needInvalidatingPaint |= !primaryFrame->TryUpdateTransformOnly(&layer);
 
         if (!needInvalidatingPaint) {
           // Since we're not going to paint, we need to resend animation
           // data to the layer.
           MOZ_ASSERT(layer, "this can't happen if there's no layer");
           nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer(
-              layer, nullptr, nullptr, aFrame, DisplayItemType::TYPE_TRANSFORM);
+              layer, nullptr, nullptr, primaryFrame,
+              DisplayItemType::TYPE_TRANSFORM);
         }
       }
     }
     if (aChange & nsChangeHint_ChildrenOnlyTransform) {
       needInvalidatingPaint = true;
       nsIFrame* childFrame = GetFrameForChildrenOnlyTransformHint(aFrame)
                                  ->PrincipalChildList()
                                  .FirstChild();
@@ -1193,17 +1196,18 @@ static bool IsPrimaryFrameOfRootOrBodyEl
 }
 
 static void ApplyRenderingChangeToTree(nsIPresShell* aPresShell,
                                        nsIFrame* aFrame, nsChangeHint aChange) {
   // We check StyleDisplay()->HasTransformStyle() in addition to checking
   // IsTransformed() since we can get here for some frames that don't support
   // CSS transforms.
   NS_ASSERTION(!(aChange & nsChangeHint_UpdateTransformLayer) ||
-                   aFrame->IsTransformed() ||
+                   nsLayoutUtils::GetPrimaryFrameFromStyleFrame(aFrame)
+                       ->IsTransformed() ||
                    aFrame->StyleDisplay()->HasTransformStyle(),
                "Unexpected UpdateTransformLayer hint");
 
   if (aPresShell->IsPaintingSuppressed()) {
     // Don't allow synchronous rendering changes when painting is turned off.
     aChange &= ~nsChangeHint_RepaintFrame;
     if (!aChange) {
       return;
@@ -1524,19 +1528,25 @@ void RestyleManager::ProcessRestyledFram
                 cont->MarkAsNotAbsoluteContainingBlock();
               }
             }
           }
         }
       }
     }
 
-    if ((hint & nsChangeHint_AddOrRemoveTransform) && frame &&
+    // Transforms are applied to the primary frame only but transform-related
+    // change hints (e.g. those due to animation) are generally only applied to
+    // the style frame. As a result, we need to look up the primary frame so we
+    // can apply the appropriate frame bits there.
+    nsIFrame* primaryFrame = content ? content->GetPrimaryFrame() : nullptr;
+
+    if (primaryFrame && (hint & nsChangeHint_AddOrRemoveTransform) &&
         !(hint & nsChangeHint_ReconstructFrame)) {
-      for (nsIFrame* cont = frame; cont;
+      for (nsIFrame* cont = primaryFrame; cont;
            cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
         if (cont->StyleDisplay()->HasTransform(cont)) {
           cont->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
         }
         // Don't remove NS_FRAME_MAY_BE_TRANSFORMED since it may still be
         // transformed by other means. It's OK to have the bit even if it's
         // not needed.
       }
@@ -1606,36 +1616,34 @@ void RestyleManager::ProcessRestyledFram
         // frame does not maintain overflow rects, so avoid calling
         // FinishAndStoreOverflow on it:
         hint &=
             ~(nsChangeHint_UpdateOverflow | nsChangeHint_ChildrenOnlyTransform |
               nsChangeHint_UpdatePostTransformOverflow |
               nsChangeHint_UpdateParentOverflow);
       }
 
-      if (!(frame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED)) {
+      if (primaryFrame &&
+          !(primaryFrame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED)) {
         // Frame can not be transformed, and thus a change in transform will
         // have no effect and we should not use the
         // nsChangeHint_UpdatePostTransformOverflow hint.
         hint &= ~nsChangeHint_UpdatePostTransformOverflow;
       }
 
-      if ((hint & nsChangeHint_UpdateTransformLayer) &&
-          !(frame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED) &&
-          frame->HasAnimationOfTransform()) {
+      if ((hint & nsChangeHint_UpdateTransformLayer) && primaryFrame &&
+          !(primaryFrame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED) &&
+          primaryFrame->HasAnimationOfTransform()) {
         // If we have an nsChangeHint_UpdateTransformLayer hint but no
         // corresponding frame bit, it's possible we have a transform animation
         // with transform style 'none' that was initialized independently from
         // this frame and associated after the fact.
         //
         // In that case we should set the frame bit.
-        //
-        // FIXME: Bug 1527210 - Use the primary frame here instead so that
-        // we handle display: table correctly.
-        for (nsIFrame* cont = frame; cont;
+        for (nsIFrame* cont = primaryFrame; cont;
              cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
           cont->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
         }
       }
 
       if (hint & nsChangeHint_AddOrRemoveTransform) {
         // When dropping a running transform animation we will first add an
         // nsChangeHint_UpdateTransformLayer hint as part of the animation-only
@@ -1736,17 +1744,17 @@ void RestyleManager::ProcessRestyledFram
                        (hint & nsChangeHint_UpdateOverflow),
                    "nsChangeHint_UpdateOverflow should be passed too");
       if (!didReflowThisFrame &&
           (hint & (nsChangeHint_UpdateOverflow |
                    nsChangeHint_UpdatePostTransformOverflow |
                    nsChangeHint_UpdateParentOverflow |
                    nsChangeHint_UpdateSubtreeOverflow))) {
         if (hint & nsChangeHint_UpdateSubtreeOverflow) {
-          for (nsIFrame* cont = frame; cont;
+          for (nsIFrame* cont = primaryFrame; cont;
                cont =
                    nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
             AddSubtreeToOverflowTracker(cont, mOverflowChangedTracker);
           }
           // The work we just did in AddSubtreeToOverflowTracker
           // subsumes some of the other hints:
           hint &= ~(nsChangeHint_UpdateOverflow |
                     nsChangeHint_UpdatePostTransformOverflow);
@@ -1796,44 +1804,45 @@ void RestyleManager::ProcessRestyledFram
               NS_ASSERTION(
                   childFrame->GetParent() == hintFrame,
                   "SVG child frame not expected to have different parent");
             }
           }
         }
         // If |frame| is dirty or has dirty children, we don't bother updating
         // overflows since that will happen when it's reflowed.
-        if (!(frame->GetStateBits() &
+        if (primaryFrame &&
+            !(primaryFrame->GetStateBits() &
               (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {
           if (hint & (nsChangeHint_UpdateOverflow |
                       nsChangeHint_UpdatePostTransformOverflow)) {
             OverflowChangedTracker::ChangeKind changeKind;
             // If we have both nsChangeHint_UpdateOverflow and
             // nsChangeHint_UpdatePostTransformOverflow,
             // CHILDREN_CHANGED is selected as it is
             // strictly stronger.
             if (hint & nsChangeHint_UpdateOverflow) {
               changeKind = OverflowChangedTracker::CHILDREN_CHANGED;
             } else {
               changeKind = OverflowChangedTracker::TRANSFORM_CHANGED;
             }
-            for (nsIFrame* cont = frame; cont;
+            for (nsIFrame* cont = primaryFrame; cont;
                  cont =
                      nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
               mOverflowChangedTracker.AddFrame(cont, changeKind);
             }
           }
           // UpdateParentOverflow hints need to be processed in addition
           // to the above, since if the processing of the above hints
           // yields no change, the update will not propagate to the
           // parent.
           if (hint & nsChangeHint_UpdateParentOverflow) {
             MOZ_ASSERT(frame->GetParent(),
                        "shouldn't get style hints for the root frame");
-            for (nsIFrame* cont = frame; cont;
+            for (nsIFrame* cont = primaryFrame; cont;
                  cont =
                      nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
               mOverflowChangedTracker.AddFrame(
                   cont->GetParent(), OverflowChangedTracker::CHILDREN_CHANGED);
             }
           }
         }
       }
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -1414,37 +1414,47 @@ nsIFrame* nsLayoutUtils::GetClosestFrame
   return nullptr;
 }
 
 /* static */
 nsIFrame* nsLayoutUtils::GetPageFrame(nsIFrame* aFrame) {
   return GetClosestFrameOfType(aFrame, LayoutFrameType::Page);
 }
 
-// static
-nsIFrame* nsLayoutUtils::GetStyleFrame(nsIFrame* aFrame) {
-  if (aFrame->IsTableWrapperFrame()) {
-    nsIFrame* inner = aFrame->PrincipalChildList().FirstChild();
-    // inner may be null, if aFrame is mid-destruction
+/* static */
+nsIFrame* nsLayoutUtils::GetStyleFrame(nsIFrame* aPrimaryFrame) {
+  if (aPrimaryFrame->IsTableWrapperFrame()) {
+    nsIFrame* inner = aPrimaryFrame->PrincipalChildList().FirstChild();
+    // inner may be null, if aPrimaryFrame is mid-destruction
     return inner;
   }
 
-  return aFrame;
+  return aPrimaryFrame;
+}
+
+const nsIFrame* nsLayoutUtils::GetStyleFrame(const nsIFrame* aPrimaryFrame) {
+  return nsLayoutUtils::GetStyleFrame(const_cast<nsIFrame*>(aPrimaryFrame));
 }
 
 nsIFrame* nsLayoutUtils::GetStyleFrame(const nsIContent* aContent) {
   nsIFrame* frame = aContent->GetPrimaryFrame();
   if (!frame) {
     return nullptr;
   }
 
   return nsLayoutUtils::GetStyleFrame(frame);
 }
 
 /* static */
+nsIFrame* nsLayoutUtils::GetPrimaryFrameFromStyleFrame(nsIFrame* aStyleFrame) {
+  nsIFrame* parent = aStyleFrame->GetParent();
+  return parent && parent->IsTableWrapperFrame() ? parent : aStyleFrame;
+}
+
+/* static */
 nsIFrame* nsLayoutUtils::GetRealPrimaryFrameFor(const nsIContent* aContent) {
   nsIFrame* frame = aContent->GetPrimaryFrame();
   if (!frame) {
     return nullptr;
   }
 
   return nsPlaceholderFrame::GetRealFrameFor(frame);
 }
--- a/layout/base/nsLayoutUtils.h
+++ b/layout/base/nsLayoutUtils.h
@@ -378,26 +378,33 @@ class nsLayoutUtils {
    * the content.
    * This is aPrimaryFrame itself except for tableWrapper frames.
    *
    * Given a non-null input, this will return null if and only if its
    * argument is a table wrapper frame that is mid-destruction (and its
    * table frame has been destroyed).
    */
   static nsIFrame* GetStyleFrame(nsIFrame* aPrimaryFrame);
+  static const nsIFrame* GetStyleFrame(const nsIFrame* aPrimaryFrame);
 
   /**
    * Given a content node,
    * return the frame that has the non-pseudoelement ComputedStyle for
    * the content.  May return null.
    * This is aContent->GetPrimaryFrame() except for tableWrapper frames.
    */
   static nsIFrame* GetStyleFrame(const nsIContent* aContent);
 
   /**
+   * The inverse of GetStyleFrame. Returns |aStyleFrame| unless it is an inner
+   * table frame, in which case the table wrapper frame is returned.
+   */
+  static nsIFrame* GetPrimaryFrameFromStyleFrame(nsIFrame* aStyleFrame);
+
+  /**
    * Gets the real primary frame associated with the content object.
    *
    * In the case of absolutely positioned elements and floated elements,
    * the real primary frame is the frame that is out of the flow and not the
    * placeholder frame.
    */
   static nsIFrame* GetRealPrimaryFrameFor(const nsIContent* aContent);
 
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -1546,19 +1546,20 @@ bool nsIFrame::IsTransformed(const nsSty
 
 bool nsIFrame::IsCSSTransformed(const nsStyleDisplay* aStyleDisplay) const {
   MOZ_ASSERT(aStyleDisplay == StyleDisplay());
   return ((mState & NS_FRAME_MAY_BE_TRANSFORMED) &&
           (aStyleDisplay->HasTransform(this) || HasAnimationOfTransform()));
 }
 
 bool nsIFrame::HasAnimationOfTransform() const {
-  return IsPrimaryFrame() &&
+  const nsIFrame* styleFrame = nsLayoutUtils::GetStyleFrame(this);
+  return IsPrimaryFrame() && styleFrame &&
          nsLayoutUtils::HasAnimationOfPropertySet(
-             this, nsCSSPropertyIDSet::TransformLikeProperties()) &&
+             styleFrame, nsCSSPropertyIDSet::TransformLikeProperties()) &&
          IsFrameOfType(eSupportsCSSTransforms);
 }
 
 bool nsIFrame::ChildrenHavePerspective(
     const nsStyleDisplay* aStyleDisplay) const {
   MOZ_ASSERT(aStyleDisplay == StyleDisplay());
   return aStyleDisplay->HasPerspective(this);
 }
--- a/layout/reftests/web-animations/reftest.list
+++ b/layout/reftests/web-animations/reftest.list
@@ -11,15 +11,16 @@ test-pref(dom.animations-api.core.enable
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-opacity-changing-effect.html stacking-context-animation-ref.html
 == stacking-context-opacity-losing-css-animation-in-delay.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-keyframe.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-keyframe-in-delay.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-target.html stacking-context-animation-changing-target-ref.html
 
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-target-in-delay.html stacking-context-animation-changing-target-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-effect.html stacking-context-animation-ref.html
+test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-effect-on-table.html stacking-context-animation-on-table-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-display-property.html stacking-context-animation-ref.html
 == stacking-context-transform-losing-css-animation-in-delay.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) test-pref(dom.animations-api.compositing.enabled,true) == style-updates-on-iteration-composition-changed-from-accumulate-to-replace.html style-updates-for-iteration-composite-ref.html
 test-pref(dom.animations-api.core.enabled,true) test-pref(dom.animations-api.compositing.enabled,true) == style-updates-on-iteration-composition-changed-from-replace-to-accumulate.html style-updates-for-iteration-composite-ref.html
 test-pref(dom.animations-api.core.enabled,true) test-pref(dom.animations-api.compositing.enabled,true) == style-updates-on-current-iteration-changed.html style-updates-for-iteration-composite-ref.html
 test-pref(dom.animations-api.core.enabled,true) == cancel-animation-with-selector-matching.html about:blank
 == child-in-animating-element-display-none.html child-in-animating-element-display-none-ref.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/web-animations/stacking-context-animation-on-table-ref.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<title>Reference of testcases for bug 1273042</title>
+<style>
+span {
+  height: 100px;
+  width: 100px;
+  background: green;
+  position: fixed;
+  top: 50px;
+  z-index: -1;
+}
+#test {
+  display: table;
+  height: 100px;
+  width: 100px;
+  background: blue;
+}
+</style>
+<span></span>
+<div id="test"></div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/web-animations/stacking-context-transform-changing-effect-on-table.html
@@ -0,0 +1,37 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<title>
+  Transform animation on a display:table element creates a stacking context
+  after changing effects
+</title>
+<style>
+span {
+  height: 100px;
+  width: 100px;
+  position: fixed;
+  background: green;
+  top: 50px;
+}
+div {
+  display: table;
+  width: 100px;
+  height: 100px;
+  background: blue;
+}
+</style>
+<span></span>
+<div id="test"></div>
+<script>
+  var elem = document.getElementById("test");
+  var anim = elem.animate(null, { duration: 100000 });
+  var newEffect = new KeyframeEffect(elem,
+                                     { transform: ['none', 'none']},
+                                     100000);
+  anim.ready.then(function() {
+    anim.effect = newEffect;
+    requestAnimationFrame(function() {
+      document.documentElement.classList.remove("reftest-wait");
+    });
+  });
+</script>
+</html>