Bug 984226 - Add parameter to OverflowChangedTracker::AddFrame to distingush between frames that need nsIFrame::UpdateOverflow called and frames that only have a transform that has changed. r=dbaron

- OverflowChangedTracker::AddFrame now accepts an enumerated type parameter to indicate if the overflow areas of children have changed (CHILDREN_CHANGED), the overflow areas of the children have changed and the parent have changed (CHILDREN_AND_PARENT_CHANGED), or if only the transform has changed (TRANSFORM_CHANGED). - OverflowChangedTracker::Flush no longer falls back to calling nsIFrame::UpdateOverflow when a frame lacks a PreTransformOverflowAreas property. - Added an additional change hint, nsChangeHint_ChildrenOnlyTransform, which results in TRANSFORM_CHANGED being passed in to OverflowChangedTracker::AddFrame. - In nsIFrame::FinishAndStoreOverflow, the passed in overflow is now stored as the InitialTransformProperty for elements that are IsTransformed(). - Partially corrected Bug 926155, by only calling OverflowChangedTracker::AddFrame on parents of the sticky element during StickyScrollContainer::UpdatePositions, using CHILDREN_CHANGED.
2014-03-13 17:21:25 -07:00 · 2014-03-13 17:21:25 -07:00 · c0402875df
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@ -687,7 +687,15 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
        // FinishAndStoreOverflow on it:
        hint = NS_SubtractHint(hint,
                 NS_CombineHint(nsChangeHint_UpdateOverflow,
-                                nsChangeHint_ChildrenOnlyTransform));
+                   NS_CombineHint(nsChangeHint_ChildrenOnlyTransform,
+                                  nsChangeHint_UpdatePostTransformOverflow)));
+      }
+
+      if (!(frame->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 = NS_SubtractHint(hint, nsChangeHint_UpdatePostTransformOverflow);
      }

      if (hint & nsChangeHint_UpdateEffects) {
@ -715,7 +723,10 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
      NS_ASSERTION(!(hint & nsChangeHint_ChildrenOnlyTransform) ||
                   (hint & nsChangeHint_UpdateOverflow),
                   "nsChangeHint_UpdateOverflow should be passed too");
-      if ((hint & nsChangeHint_UpdateOverflow) && !didReflowThisFrame) {
+      if (!didReflowThisFrame &&
+          (hint & (nsChangeHint_UpdateOverflow |
+                   nsChangeHint_UpdatePostTransformOverflow))) {
+        OverflowChangedTracker::ChangeKind changeKind;
        if (hint & nsChangeHint_ChildrenOnlyTransform) {
          // The overflow areas of the child frames need to be updated:
          nsIFrame* hintFrame = GetFrameForChildrenOnlyTransformHint(frame);
@ -733,7 +744,8 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
            // updating overflows since that will happen when it's reflowed.
            if (!(childFrame->GetStateBits() &
                  (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {
-              mOverflowChangedTracker.AddFrame(childFrame);
+              mOverflowChangedTracker.AddFrame(childFrame,
+                           OverflowChangedTracker::CHILDREN_AND_PARENT_CHANGED);
            }
            NS_ASSERTION(!nsLayoutUtils::GetNextContinuationOrIBSplitSibling(childFrame),
                         "SVG frames should not have continuations "
@ -746,9 +758,17 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
        // overflows since that will happen when it's reflowed.
        if (!(frame->GetStateBits() &
              (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {
+          // If we have both nsChangeHint_UpdateOverflow and
+          // nsChangeHint_UpdatePostTransformOverflow, CHILDREN_AND_PARENT_CHANGED
+          // is selected as it is stronger.
+          if (hint & nsChangeHint_UpdateOverflow) {
+            changeKind = OverflowChangedTracker::CHILDREN_AND_PARENT_CHANGED;
+          } else {
+            changeKind = OverflowChangedTracker::TRANSFORM_CHANGED;
+          }
          for (nsIFrame *cont = frame; cont; cont =
                 nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
-            mOverflowChangedTracker.AddFrame(cont);
+            mOverflowChangedTracker.AddFrame(cont, changeKind);
          }
        }
      }
--- a/layout/base/RestyleTracker.h
+++ b/layout/base/RestyleTracker.h
@ -28,6 +28,26 @@ class RestyleManager;
 class OverflowChangedTracker
 {
 public:
+  enum ChangeKind {
+    /**
+     * The frame was explicitly added as a result of
+     * nsChangeHint_UpdatePostTransformOverflow and hence may have had a style
+     * change that changes its geometry relative to parent, without reflowing.
+     */
+    TRANSFORM_CHANGED,
+    /**
+     * The overflow areas of children have changed
+     * and we need to call UpdateOverflow on the frame.
+     */
+    CHILDREN_CHANGED,
+    /**
+     * The overflow areas of children have changed
+     * and we need to call UpdateOverflow on the frame.
+     * Also call UpdateOverflow on the parent even if the
+     * overflow areas of the frame does not change.
+     */
+    CHILDREN_AND_PARENT_CHANGED
+  };

  OverflowChangedTracker() :
    mSubtreeRoot(nullptr)
@ -49,14 +69,18 @@ public:
   * If the overflow area changes, then UpdateOverflow will also
   * be called on the parent.
   */
-  void AddFrame(nsIFrame* aFrame) {
+  void AddFrame(nsIFrame* aFrame, ChangeKind aChangeKind) {
    uint32_t depth = aFrame->GetDepthInFrameTree();
-    if (mEntryList.empty() ||
-        !mEntryList.find(Entry(aFrame, depth))) {
-      // All frames in mEntryList at this stage have STYLE_CHANGED so we don't
-      // need to worry about setting the STYLE_CHANGED flag if 'find'
-      // returns true.
-      mEntryList.insert(new Entry(aFrame, depth, STYLE_CHANGED));
+    Entry *entry = nullptr;
+    if (!mEntryList.empty()) {
+      entry = mEntryList.find(Entry(aFrame, depth));
+    }
+    if (entry == nullptr) {
+      // Add new entry.
+      mEntryList.insert(new Entry(aFrame, depth, aChangeKind));
+    } else {
+      // Update the existing entry if the new value is stronger.
+      entry->mChangeKind = std::max(entry->mChangeKind, aChangeKind);
    }
  }

@ -94,38 +118,55 @@ public:
      Entry *entry = mEntryList.removeMin();
      nsIFrame *frame = entry->mFrame;

-      bool overflowChanged;
-      if (entry->mFlags & CHILDREN_CHANGED) {
+      bool overflowChanged = false;
+      if (entry->mChangeKind == CHILDREN_AND_PARENT_CHANGED) {
        // Need to union the overflow areas of the children.
+        // Always update the parent, even if the overflow does not change.
+        frame->UpdateOverflow();
+        overflowChanged = true;
+      } else if (entry->mChangeKind == CHILDREN_CHANGED) {
+        // Need to union the overflow areas of the children.
+        // Only update the parent if the overflow changes.
        overflowChanged = frame->UpdateOverflow();
      } else {
-        nsOverflowAreas* pre = static_cast<nsOverflowAreas*>
-          (frame->Properties().Get(frame->PreTransformOverflowAreasProperty()));
-        if (pre) {
-          // Since we have the pre-transform-overflow-areas, we can take a
-          // faster path that doesn't require unioning the overflow areas
-          // of our children.
+        // Take a faster path that doesn't require unioning the overflow areas
+        // of our children.
+
+#ifdef DEBUG
+        bool hasInitialOverflowPropertyApplied = false;
+        frame->Properties().Get(nsIFrame::DebugInitialOverflowPropertyApplied(),
+                                 &hasInitialOverflowPropertyApplied);
+        NS_ASSERTION(hasInitialOverflowPropertyApplied,
+                     "InitialOverflowProperty must be set first.");
+#endif
+
+        nsOverflowAreas* overflow = 
+          static_cast<nsOverflowAreas*>(frame->Properties().Get(nsIFrame::InitialOverflowProperty()));
+        if (overflow) {
          // FinishAndStoreOverflow will change the overflow areas passed in,
          // so make a copy.
-          nsOverflowAreas overflowAreas = *pre;
-          frame->FinishAndStoreOverflow(overflowAreas, frame->GetSize());
-          // We can't tell if the overflow changed, so be conservative
-          overflowChanged = true;
+          nsOverflowAreas overflowCopy = *overflow;
+          frame->FinishAndStoreOverflow(overflowCopy, frame->GetSize());
        } else {
-          // We can't take the faster path here. Do it the hard way.
-          overflowChanged = frame->UpdateOverflow();
+          nsRect bounds(nsPoint(0, 0), frame->GetSize());
+          nsOverflowAreas boundsOverflow;
+          boundsOverflow.SetAllTo(bounds);
+          frame->FinishAndStoreOverflow(boundsOverflow, bounds.Size());
        }
+
+        // We can't tell if the overflow changed, so be conservative
+        overflowChanged = true;
      }

      // If the frame style changed (e.g. positioning offsets)
      // then we need to update the parent with the overflow areas of its
      // children.
-      if (overflowChanged || (entry->mFlags & STYLE_CHANGED)) {
+      if (overflowChanged) {
        nsIFrame *parent = frame->GetParent();
        if (parent && parent != mSubtreeRoot) {
          Entry* parentEntry = mEntryList.find(Entry(parent, entry->mDepth - 1));
          if (parentEntry) {
-            parentEntry->mFlags |= CHILDREN_CHANGED;
+            parentEntry->mChangeKind = CHILDREN_CHANGED;
          } else {
            mEntryList.insert(new Entry(parent, entry->mDepth - 1, CHILDREN_CHANGED));
          }
@ -136,26 +177,12 @@ public:
  }
  
 private:
-  enum {
-    /**
-     * Set if the overflow areas of children have changed so we need to call
-     * UpdateOverflow on the frame.
-     */
-    CHILDREN_CHANGED = 0x01,
-    /**
-     * True if the frame was explicitly added and hence may have had a style
-     * change that changes its geometry relative to parent, without reflowing.
-     * In this case we must update overflow on the frame's parent even if
-     * this frame's overflow did not change.
-     */
-    STYLE_CHANGED = 0x02
-  };
  struct Entry : SplayTreeNode<Entry>
  {
-    Entry(nsIFrame* aFrame, uint32_t aDepth, uint8_t aFlags = 0)
+    Entry(nsIFrame* aFrame, uint32_t aDepth, ChangeKind aChangeKind = CHILDREN_CHANGED)
      : mFrame(aFrame)
      , mDepth(aDepth)
-      , mFlags(aFlags)
+      , mChangeKind(aChangeKind)
    {}

    bool operator==(const Entry& aOther) const
@ -189,7 +216,7 @@ private:
    nsIFrame* mFrame;
    /* Depth in the frame tree */
    uint32_t mDepth;
-    uint8_t mFlags;
+    ChangeKind mChangeKind;
  };

  /* A list of frames to process, sorted by their depth in the frame tree */
--- a/layout/base/nsChangeHint.h
+++ b/layout/base/nsChangeHint.h
@ -72,17 +72,23 @@ enum nsChangeHint {
  nsChangeHint_ReconstructFrame = 0x400,

  /**
-   * The frame's effect on its ancestors' overflow areas has changed,
-   * either through a change in its transform or a change in its position.
-   * Does not update any descendant frames.
+   * The frame's overflow area has changed, either through a change in its
+   * transform or a change in its position. Does not update any descendant
+   * frames.
   */
  nsChangeHint_UpdateOverflow = 0x800,

+  /**
+   * The frame's overflow area has changed, through a change in its transform.
+   * Does not update any descendant frames.
+   */
+  nsChangeHint_UpdatePostTransformOverflow = 0x1000,
+
  /**
   * The children-only transform of an SVG frame changed, requiring the
   * overflow rects of the frame's immediate children to be updated.
   */
-  nsChangeHint_ChildrenOnlyTransform = 0x1000,
+  nsChangeHint_ChildrenOnlyTransform = 0x2000,

  /**
   * The frame's offsets have changed, while its dimensions might have
@ -94,7 +100,7 @@ enum nsChangeHint {
   * nsChangeHint_UpdateOverflow in order to get the overflow areas of
   * the ancestors updated as well.
   */
-  nsChangeHint_RecomputePosition = 0x2000,
+  nsChangeHint_RecomputePosition = 0x4000,

  /**
   * Behaves like ReconstructFrame, but only if the frame has descendants
@ -102,7 +108,7 @@ enum nsChangeHint {
   * has changed whether the frame is a container for fixed-pos or abs-pos
   * elements, but reframing is otherwise not needed.
   */
-  nsChangeHint_AddOrRemoveTransform = 0x4000,
+  nsChangeHint_AddOrRemoveTransform = 0x8000,

  /**
   * This change hint has *no* change handling behavior.  However, it
@ -110,13 +116,13 @@ enum nsChangeHint {
   * changes, and it's inherited by a child, that might require a reflow
   * due to the border-width change on the child.
   */
-  nsChangeHint_BorderStyleNoneChange = 0x8000,
+  nsChangeHint_BorderStyleNoneChange = 0x10000,

  /**
   * SVG textPath needs to be recomputed because the path has changed.
   * This means that the glyph positions of the text need to be recomputed.
   */
-  nsChangeHint_UpdateTextPath = 0x10000
+  nsChangeHint_UpdateTextPath = 0x20000

  // IMPORTANT NOTE: When adding new hints, consider whether you need to
  // add them to NS_HintsNotHandledForDescendantsIn() below.
@ -172,6 +178,7 @@ inline bool NS_IsHintSubset(nsChangeHint aSubset, nsChangeHint aSuperSet) {
          nsChangeHint_UpdateEffects | \
          nsChangeHint_UpdateOpacityLayer | \
          nsChangeHint_UpdateOverflow | \
+          nsChangeHint_UpdatePostTransformOverflow | \
          nsChangeHint_ChildrenOnlyTransform | \
          nsChangeHint_RecomputePosition | \
          nsChangeHint_AddOrRemoveTransform | \
@ -185,6 +192,7 @@ inline nsChangeHint NS_HintsNotHandledForDescendantsIn(nsChangeHint aChangeHint)
    nsChangeHint_UpdateEffects |
    nsChangeHint_UpdateOpacityLayer |
    nsChangeHint_UpdateOverflow |
+    nsChangeHint_UpdatePostTransformOverflow |
    nsChangeHint_ChildrenOnlyTransform |
    nsChangeHint_RecomputePosition |
    nsChangeHint_AddOrRemoveTransform |
--- a/layout/generic/StickyScrollContainer.cpp
+++ b/layout/generic/StickyScrollContainer.cpp
@ -371,9 +371,12 @@ StickyScrollContainer::UpdatePositions(nsPoint aScrollPosition,
    // nsIFrame::Init.
    PositionContinuations(f);

-    for (nsIFrame* cont = f; cont;
-         cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
-      oct.AddFrame(cont);
+    f = f->GetParent();
+    if (f != aSubtreeRoot) {
+      for (nsIFrame* cont = f; cont;
+           cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
+        oct.AddFrame(cont, OverflowChangedTracker::CHILDREN_CHANGED);
+      }
    }
  }
  oct.Flush();
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@ -7199,18 +7199,28 @@ nsIFrame::FinishAndStoreOverflow(nsOverflowAreas& aOverflowAreas,
               "Don't call - overflow rects not maintained on these SVG frames");

  nsRect bounds(nsPoint(0, 0), aNewSize);
-  // Store the passed in overflow area if we are a preserve-3d frame,
-  // and it's not just the frame bounds.
-  if ((Preserves3D() || HasPerspective()) && (!aOverflowAreas.VisualOverflow().IsEqualEdges(bounds) ||
-                        !aOverflowAreas.ScrollableOverflow().IsEqualEdges(bounds))) {
-    nsOverflowAreas* initial =
-      static_cast<nsOverflowAreas*>(Properties().Get(nsIFrame::InitialOverflowProperty()));
-    if (!initial) {
-      Properties().Set(nsIFrame::InitialOverflowProperty(),
-                       new nsOverflowAreas(aOverflowAreas));
-    } else if (initial != &aOverflowAreas) {
-      *initial = aOverflowAreas;
+  // Store the passed in overflow area if we are a preserve-3d frame or we have
+  // a transform, and it's not just the frame bounds.
+  if (Preserves3D() || HasPerspective() || IsTransformed()) {
+    if (!aOverflowAreas.VisualOverflow().IsEqualEdges(bounds) ||
+        !aOverflowAreas.ScrollableOverflow().IsEqualEdges(bounds)) {
+
+      nsOverflowAreas* initial =
+        static_cast<nsOverflowAreas*>(Properties().Get(nsIFrame::InitialOverflowProperty()));
+      if (!initial) {
+        Properties().Set(nsIFrame::InitialOverflowProperty(),
+                         new nsOverflowAreas(aOverflowAreas));
+      } else if (initial != &aOverflowAreas) {
+        *initial = aOverflowAreas;
+      }
    }
+#ifdef DEBUG
+    Properties().Set(nsIFrame::DebugInitialOverflowPropertyApplied(), nullptr);
+#endif
+  } else {
+#ifdef DEBUG
+  Properties().Delete(nsIFrame::DebugInitialOverflowPropertyApplied());
+#endif
  }

  // This is now called FinishAndStoreOverflow() instead of 
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@ -884,10 +884,17 @@ public:
                            DestroyOverflowAreas)

  // The initial overflow area passed to FinishAndStoreOverflow. This is only set
-  // on frames that Preserve3D(), and when at least one of the overflow areas
-  // differs from the frame bound rect.
+  // on frames that Preserve3D() or HasPerspective() or IsTransformed(), and
+  // when at least one of the overflow areas differs from the frame bound rect.
  NS_DECLARE_FRAME_PROPERTY(InitialOverflowProperty, DestroyOverflowAreas)

+#ifdef DEBUG
+  // InitialOverflowPropertyDebug is added to the frame to indicate that either
+  // the InitialOverflowProperty has been stored or the InitialOverflowProperty
+  // has been suppressed due to being set to the default value (frame bounds)
+  NS_DECLARE_FRAME_PROPERTY(DebugInitialOverflowPropertyApplied, nullptr)
+#endif
+
  NS_DECLARE_FRAME_PROPERTY(UsedMarginProperty, DestroyMargin)
  NS_DECLARE_FRAME_PROPERTY(UsedPaddingProperty, DestroyMargin)
  NS_DECLARE_FRAME_PROPERTY(UsedBorderProperty, DestroyMargin)
--- a/layout/mathml/crashtests/crashtests.list
+++ b/layout/mathml/crashtests/crashtests.list
@ -59,4 +59,4 @@ load 655451-1.xhtml
 load 713606-1.html
 load 716349-1.html
 load 947557-1.html
-test-pref(layout.css.sticky.enabled,true) asserts(1) load 973322-1.xhtml
+test-pref(layout.css.sticky.enabled,true) load 973322-1.xhtml
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@ -2524,7 +2524,7 @@ nsChangeHint nsStyleDisplay::CalcDifference(const nsStyleDisplay& aOther) const
    if (!mSpecifiedTransform != !aOther.mSpecifiedTransform ||
        (mSpecifiedTransform &&
         *mSpecifiedTransform != *aOther.mSpecifiedTransform)) {
-      NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_UpdateOverflow,
+      NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_UpdatePostTransformOverflow,
                                         nsChangeHint_UpdateTransformLayer));
    }

--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@ -1901,6 +1901,7 @@ struct nsStyleDisplay {
                        nsChangeHint_UpdateOpacityLayer |
                        nsChangeHint_UpdateTransformLayer |
                        nsChangeHint_UpdateOverflow |
+                        nsChangeHint_UpdatePostTransformOverflow |
                        nsChangeHint_AddOrRemoveTransform);
  }
  static nsChangeHint MaxDifferenceNeverInherited() {
--- a/layout/tables/nsTableFrame.cpp
+++ b/layout/tables/nsTableFrame.cpp
@ -1955,7 +1955,8 @@ nsTableFrame::FixupPositionedTableParts(nsPresContext*           aPresContext,
    // FIXME: Unconditionally using NS_UNCONSTRAINEDSIZE for the height and
    // ignoring any change to the reflow status aren't correct. We'll never
    // paginate absolutely positioned frames.
-    overflowTracker.AddFrame(positionedPart);
+    overflowTracker.AddFrame(positionedPart,
+      OverflowChangedTracker::CHILDREN_AND_PARENT_CHANGED);
    nsFrame* positionedFrame = static_cast<nsFrame*>(positionedPart);
    positionedFrame->FinishReflowWithAbsoluteFrames(PresContext(),
                                                    desiredSize,