Bug 827577 - Be a lot more careful about saving and restoring frame constructor state for fixed-pos descendants of CSS-transformed elements. r=bz, a=akeybl
authorRobert O'Callahan <robert@ocallahan.org>
Fri, 11 Jan 2013 00:47:33 +1300
changeset 127365 b6269b3e8eefa01f09608b899689f293cffe3d24
parent 127364 13dc236d0fd868cca1809123f0c8df1318eae34b
child 127366 b615064c99280f638663d40c28a6849e980b2777
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz, akeybl
bugs827577
milestone20.0a2
Bug 827577 - Be a lot more careful about saving and restoring frame constructor state for fixed-pos descendants of CSS-transformed elements. r=bz, a=akeybl
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
layout/reftests/bugs/827577-1-ref.html
layout/reftests/bugs/827577-1a.html
layout/reftests/bugs/827577-1b.html
layout/reftests/bugs/827577-2.html
layout/reftests/bugs/reftest.list
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -678,26 +678,29 @@ nsAbsoluteItems::AddChild(nsIFrame* aChi
 // blocks. The destructor restores the state to its previous state
 class NS_STACK_CLASS nsFrameConstructorSaveState {
 public:
   typedef nsIFrame::ChildListID ChildListID;
   nsFrameConstructorSaveState();
   ~nsFrameConstructorSaveState();
 
 private:
-  nsAbsoluteItems* mItems;                // pointer to struct whose data we save/restore
-  bool*    mFixedPosIsAbsPos;
-
-  nsAbsoluteItems  mSavedItems;           // copy of original data
-  bool             mSavedFixedPosIsAbsPos;
+  nsAbsoluteItems* mItems;      // pointer to struct whose data we save/restore
+  nsAbsoluteItems  mSavedItems; // copy of original data
 
   // The name of the child list in which our frames would belong
   ChildListID mChildListID;
   nsFrameConstructorState* mState;
 
+  // State used only when we're saving the abs-pos state for a transformed
+  // element.
+  nsAbsoluteItems mSavedFixedItems;
+
+  bool mSavedFixedPosIsAbsPos;
+
   friend class nsFrameConstructorState;
 };
 
 // Structure used to keep track of a list of bindings we need to call
 // AddToAttachedQueue on.  These should be in post-order depth-first
 // flattened tree traversal order.
 struct PendingBinding : public LinkedListElement<PendingBinding>
 {
@@ -776,16 +779,18 @@ public:
                           nsIFrame*              aAbsoluteContainingBlock,
                           nsIFrame*              aFloatContainingBlock);
 
   ~nsFrameConstructorState();
   
   // Function to push the existing absolute containing block state and
   // create a new scope. Code that uses this function should get matching
   // logic in GetAbsoluteContainingBlock.
+  // Also makes aNewAbsoluteContainingBlock the containing block for
+  // fixed-pos elements if necessary.
   void PushAbsoluteContainingBlock(nsIFrame* aNewAbsoluteContainingBlock,
                                    nsFrameConstructorSaveState& aSaveState);
 
   // Function to push the existing float containing block state and
   // create a new scope. Code that uses this function should get matching
   // logic in GetFloatContainingBlock.
   // Pushing a null float containing block forbids any frames from being
   // floated until a new float containing block is pushed.
@@ -1022,21 +1027,25 @@ AdjustAbsoluteContainingBlock(nsIFrame* 
 void
 nsFrameConstructorState::PushAbsoluteContainingBlock(nsIFrame* aNewAbsoluteContainingBlock,
                                                      nsFrameConstructorSaveState& aSaveState)
 {
   aSaveState.mItems = &mAbsoluteItems;
   aSaveState.mSavedItems = mAbsoluteItems;
   aSaveState.mChildListID = nsIFrame::kAbsoluteList;
   aSaveState.mState = this;
-
-  /* Store whether we're wiring the abs-pos and fixed-pos lists together. */
-  aSaveState.mFixedPosIsAbsPos = &mFixedPosIsAbsPos;
   aSaveState.mSavedFixedPosIsAbsPos = mFixedPosIsAbsPos;
 
+  if (mFixedPosIsAbsPos) {
+    // Since we're going to replace mAbsoluteItems, we need to save it into
+    // mFixedItems now (and save the current value of mFixedItems).
+    aSaveState.mSavedFixedItems = mFixedItems;
+    mFixedItems = mAbsoluteItems;
+  }
+
   mAbsoluteItems = 
     nsAbsoluteItems(AdjustAbsoluteContainingBlock(aNewAbsoluteContainingBlock));
 
   /* See if we're wiring the fixed-pos and abs-pos lists together.  This happens iff
    * we're a transformed element.
    */
   mFixedPosIsAbsPos = aNewAbsoluteContainingBlock &&
     aNewAbsoluteContainingBlock->GetStyleDisplay()->HasTransform(aNewAbsoluteContainingBlock);
@@ -1230,17 +1239,23 @@ nsFrameConstructorState::ProcessFrameIns
   if (aFrameItems.IsEmpty()) {
     return;
   }
   
   nsIFrame* containingBlock = aFrameItems.containingBlock;
 
   NS_ASSERTION(containingBlock,
                "Child list without containing block?");
-  
+
+  if (aChildListID == nsIFrame::kFixedList &&
+      containingBlock->GetStyleDisplay()->HasTransform(containingBlock)) {
+    // Put this frame on the transformed-frame's abs-pos list instead.
+    aChildListID = nsIFrame::kAbsoluteList;
+  }
+
   // Insert the frames hanging out in aItems.  We can use SetInitialChildList()
   // if the containing block hasn't been reflowed yet (so NS_FRAME_FIRST_REFLOW
   // is set) and doesn't have any frames in the aChildListID child list yet.
   const nsFrameList& childList = containingBlock->GetChildList(aChildListID);
   DebugOnly<nsresult> rv = NS_OK;
   if (childList.IsEmpty() &&
       (containingBlock->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
     // If we're injecting absolutely positioned frames, inject them on the
@@ -1294,39 +1309,50 @@ nsFrameConstructorState::ProcessFrameIns
   // and deal with all the placeholders... but what if the placeholders aren't
   // in the document yet?  Could that happen?
   NS_ASSERTION(NS_SUCCEEDED(rv), "Frames getting lost!");
 }
 
 
 nsFrameConstructorSaveState::nsFrameConstructorSaveState()
   : mItems(nullptr),
-    mFixedPosIsAbsPos(nullptr),
     mSavedItems(nullptr),
-    mSavedFixedPosIsAbsPos(false),
     mChildListID(kPrincipalList),
-    mState(nullptr)
+    mState(nullptr),
+    mSavedFixedItems(nullptr),
+    mSavedFixedPosIsAbsPos(false)
 {
 }
 
 nsFrameConstructorSaveState::~nsFrameConstructorSaveState()
 {
   // Restore the state
   if (mItems) {
     NS_ASSERTION(mState, "Can't have mItems set without having a state!");
     mState->ProcessFrameInsertions(*mItems, mChildListID);
     *mItems = mSavedItems;
 #ifdef DEBUG
     // We've transferred the child list, so drop the pointer we held to it.
     // Note that this only matters for the assert in ~nsAbsoluteItems.
     mSavedItems.Clear();
 #endif
-  }
-  if (mFixedPosIsAbsPos) {
-    *mFixedPosIsAbsPos = mSavedFixedPosIsAbsPos;
+    if (mItems == &mState->mAbsoluteItems) {
+      mState->mFixedPosIsAbsPos = mSavedFixedPosIsAbsPos;
+      if (mSavedFixedPosIsAbsPos) {
+        // mAbsoluteItems was moved to mFixedItems, so move mFixedItems back
+        // and repair the old mFixedItems now.
+        mState->mAbsoluteItems = mState->mFixedItems;
+        mState->mFixedItems = mSavedFixedItems;
+#ifdef DEBUG
+        mSavedFixedItems.Clear();
+#endif
+      }
+    }
+    NS_ASSERTION(!mItems->LastChild() || !mItems->LastChild()->GetNextSibling(),
+                 "Something corrupted our list");
   }
 }
 
 static 
 bool IsBorderCollapse(nsIFrame* aFrame)
 {
   for (nsIFrame* frame = aFrame; frame; frame = frame->GetParent()) {
     if (nsGkAtoms::tableFrame == frame->GetType()) {
@@ -5612,16 +5638,31 @@ nsCSSFrameConstructor::GetAbsoluteContai
 
   // It is possible for the search for the containing block to fail, because
   // no absolute container can be found in the parent chain.  In those cases,
   // we fall back to the document element's containing block.
   return mHasRootAbsPosContainingBlock ? mDocElementContainingBlock : nullptr;
 }
 
 nsIFrame*
+nsCSSFrameConstructor::GetFixedContainingBlock(nsIFrame* aFrame)
+{
+  NS_PRECONDITION(nullptr != mRootElementFrame, "no root element frame");
+
+  // Starting with aFrame, look for a frame that is CSS-transformed
+  for (nsIFrame* frame = aFrame; frame; frame = frame->GetParent()) {
+    if (frame->GetStyleDisplay()->HasTransform(frame)) {
+      return frame;
+    }
+  }
+
+  return mFixedContainingBlock;
+}
+
+nsIFrame*
 nsCSSFrameConstructor::GetFloatContainingBlock(nsIFrame* aFrame)
 {
   // Starting with aFrame, look for a frame that is a float containing block.
   // IF we hit a mathml frame, bail out; we don't allow floating out of mathml
   // frames, because they don't seem to be able to deal.
   // The logic here needs to match the logic in ProcessChildren()
   for (nsIFrame* containingBlock = aFrame;
        containingBlock &&
@@ -6605,17 +6646,17 @@ nsCSSFrameConstructor::ContentAppended(n
   // Deal with possible :after generated content on the parent
   nsIFrame* parentAfterFrame;
   parentFrame =
     ::AdjustAppendParentForAfterContent(mPresShell->GetPresContext(),
                                         aContainer, parentFrame,
                                         &parentAfterFrame);
   
   // Create some new frames
-  nsFrameConstructorState state(mPresShell, mFixedContainingBlock,
+  nsFrameConstructorState state(mPresShell, GetFixedContainingBlock(parentFrame),
                                 GetAbsoluteContainingBlock(parentFrame),
                                 GetFloatContainingBlock(parentFrame));
   state.mTreeMatchContext.mAncestorFilter.Init(aContainer->AsElement());
 
   // See if the containing block has :first-letter style applied.
   bool haveFirstLetterStyle = false, haveFirstLineStyle = false;
   nsIFrame* containingBlock = state.mFloatedItems.containingBlock;
   if (containingBlock) {
@@ -7040,17 +7081,17 @@ nsCSSFrameConstructor::ContentRangeInser
 
   if (parentFrame->IsFrameOfType(nsIFrame::eMathML)) {
     LAYOUT_PHASE_TEMP_EXIT();
     nsresult rv = RecreateFramesForContent(parentFrame->GetContent(), false);
     LAYOUT_PHASE_TEMP_REENTER();
     return rv;
   }
 
-  nsFrameConstructorState state(mPresShell, mFixedContainingBlock,
+  nsFrameConstructorState state(mPresShell, GetFixedContainingBlock(parentFrame),
                                 GetAbsoluteContainingBlock(parentFrame),
                                 GetFloatContainingBlock(parentFrame),
                                 aFrameState);
   state.mTreeMatchContext.mAncestorFilter.Init(aContainer ?
                                                  aContainer->AsElement() :
                                                  nullptr);
 
   // Recover state for the containing block - we need to know if
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -1468,16 +1468,17 @@ private:
    * element. They should recreate the same state that we would have
    * arrived at if we had built frames from the root frame to aFrame.
    * Therefore, any calls to PushFloatContainingBlock and
    * PushAbsoluteContainingBlock during frame construction should get
    * corresponding logic in these functions.
    */
 public:
   nsIFrame* GetAbsoluteContainingBlock(nsIFrame* aFrame);
+  nsIFrame* GetFixedContainingBlock(nsIFrame* aFrame);
 private:
   nsIFrame* GetFloatContainingBlock(nsIFrame* aFrame);
 
   nsIContent* PropagateScrollToViewport();
 
   // Build a scroll frame: 
   //  Calls BeginBuildingScrollFrame, InitAndRestoreFrame, and then FinishBuildingScrollFrame.
   // @param aNewFrame the created scrollframe --- output only
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/827577-1-ref.html
@@ -0,0 +1,7 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<div style="margin-left:100px; width:200px; height:200px; border:1px solid black">
+  <div style="margin-left:150px; width:50px; height:80px; background:yellow;">
+  </div>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/827577-1a.html
@@ -0,0 +1,11 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<div style="transform:translateX(100px); width:200px; height:200px; border:1px solid black">
+  <div style="position:absolute;">
+  </div>
+  <div style="position:absolute;">
+    <div style="position:fixed; right:0; width:50px; height:80px; background:yellow;">
+    </div>
+  </div>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/827577-1b.html
@@ -0,0 +1,18 @@
+<!DOCTYPE HTML>
+<html class="reftest-wait">
+<body>
+<div style="transform:translateX(100px); width:200px; height:200px; border:1px solid black">
+  <div style="position:absolute;">
+  </div>
+  <div style="position:absolute;">
+    <div id="d" style="display:none; position:fixed; right:0; width:50px; height:80px; background:yellow;">
+    </div>
+  </div>
+</div>
+<script>
+function doTest() {
+  document.getElementById("d").style.display = "";
+  document.documentElement.removeAttribute("class");
+}
+window.addEventListener("MozReftestInvalidate", doTest, false);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/827577-2.html
@@ -0,0 +1,11 @@
+<!DOCTYPE HTML>
+<html class="reftest-wait>
+<body>
+<div style="transform:translateX(100px); width:200px; height:200px; border:1px solid black">
+  <div style="position:absolute;">
+  </div>
+  <div style="position:absolute;">
+    <div style="position:fixed; right:0; width:50px; height:80px; background:yellow;">
+    </div>
+  </div>
+</div>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1733,8 +1733,10 @@ fuzzy(40,800) == 797797-2.html 797797-2-
 == 811301-1.html 811301-1-ref.html
 == 812824-1.html 812824-1-ref.html
 == 814677.html 814677-ref.html
 skip-if(B2G) == 815593-1.html 815593-1-ref.html
 skip-if(B2G) == 814952-1.html 814952-1-ref.html
 == 816458-1.html 816458-1-ref.html
 == 816359-1.html 816359-1-ref.html
 skip-if(B2G) == 818276-1.html 818276-1-ref.html
+== 827577-1a.html 827577-1-ref.html
+== 827577-1b.html 827577-1-ref.html