Bug 407601, use preferred size when calculating popup position and size instead of current size, fixes extraneous layouts and scrolling position changes, r+sr=bz,a=shrep

layout/xul/base/src/nsMenuFrame.cpp

@@ -748,9 +748,9 @@ nsMenuFrame::DoLayout(nsBoxLayoutState& aState)
         prefSize.width = mRect.width;
 
     // if the pref size changed then set bounds to be the pref size
-    PRBool sizeChanged = (mPopupFrame->GetRect().Size() != prefSize);
+    PRBool sizeChanged = (mPopupFrame->PreferredSize() != prefSize);
     if (sizeChanged) {
-      mPopupFrame->SetBounds(aState, nsRect(0,0,prefSize.width, prefSize.height));
+      mPopupFrame->SetPreferredBounds(aState, nsRect(0,0,prefSize.width, prefSize.height));
     }
 
     // if the menu has just been opened, or its size changed, position

layout/xul/base/src/nsMenuPopupFrame.cpp

@@ -114,7 +114,8 @@ nsMenuPopupFrame::nsMenuPopupFrame(nsIPresShell* aShell, nsStyleContext* aContex
   mMenuCanOverlapOSBar(PR_FALSE),
   mShouldAutoPosition(PR_TRUE),
   mConsumeRollupEvent(nsIPopupBoxObject::ROLLUP_DEFAULT),
-  mInContentShell(PR_TRUE)
+  mInContentShell(PR_TRUE),
+  mPrefSize(-1, -1)
 {
 } // ctor
 
@@ -299,6 +300,14 @@ nsMenuPopupFrame::IsLeaf() const
           !parentContent->HasAttr(kNameSpaceID_None, nsGkAtoms::sizetopopup));
 }
 
+void
+nsMenuPopupFrame::SetPreferredBounds(nsBoxLayoutState& aState,
+                                     const nsRect& aRect)
+{
+  nsBox::SetBounds(aState, aRect, PR_FALSE);
+  mPrefSize = aRect.Size();
+}
+
 void
 nsMenuPopupFrame::AdjustView()
 {
@@ -907,11 +916,19 @@ nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
   parentSize.width = NSToCoordCeil(parentSize.width * adj);
   parentSize.height = NSToCoordCeil(parentSize.height * adj);
 
-  // If we stick to our parent's width, set it here before we move the
-  // window around, because moving is done with respect to the width...
+  // Set the popup's size to the preferred size. Below, this size will be
+  // adjusted to fit on the screen or within the content area. If the anchor
+  // is sized to the popup, use the anchor's width instead of the preferred
+  // width. The preferred size should already be set by the parent frame.
+  NS_ASSERTION(mPrefSize.width >= 0 || mPrefSize.height >= 0,
+               "preferred size of popup not set");
   if (sizedToPopup) {
     mRect.width = parentSize.width;
   }
+  else {
+    mRect.width = mPrefSize.width;
+  }
+  mRect.height = mPrefSize.height;
 
   // |xpos| and |ypos| hold the x and y positions of where the popup will be moved to,
   // in app units, in the coordinate system of the _parent view_.

layout/xul/base/src/nsMenuPopupFrame.h

@@ -271,7 +271,15 @@ public:
   void SetConsumeRollupEvent(PRUint32 aConsumeMode);
 
   nsIScrollableView* GetScrollableView(nsIFrame* aStart);
-  
+
+  // same as SetBounds except the preferred size mPrefSize is also set.
+  void SetPreferredBounds(nsBoxLayoutState& aState, const nsRect& aRect);
+
+  // retrieve the last preferred size
+  nsSize PreferredSize() { return mPrefSize; }
+  // set the last preferred size
+  void SetPreferredSize(nsSize aSize) { mPrefSize = aSize; }
+
 protected:
   // Move without updating attributes.                                          
   void MoveToInternal(PRInt32 aLeft, PRInt32 aTop);                             
@@ -328,6 +336,15 @@ protected:
 
   nsString     mIncrementalString;  // for incremental typing navigation
 
+  // A popup's preferred size may be different than its actual size stored in
+  // mRect in the case where the popup was resized because it was too large
+  // for the screen. The preferred size mPrefSize holds the full size the popup
+  // would be before resizing. Computations are performed using this size.
+  // The parent frame is responsible for setting the preferred size using
+  // SetPreferredBounds or SetPreferredSize before positioning the popup with
+  // SetPopupPosition.
+  nsSize mPrefSize;
+
 }; // class nsMenuPopupFrame
 
 #endif

layout/xul/base/src/nsPopupSetFrame.cpp

@@ -172,7 +172,7 @@ nsPopupSetFrame::DoLayout(nsBoxLayoutState& aState)
 
       BoundsCheck(minSize, prefSize, maxSize);
 
-      popupChild->SetBounds(aState, nsRect(0,0,prefSize.width, prefSize.height));
+      popupChild->SetPreferredBounds(aState, nsRect(0,0,prefSize.width, prefSize.height));
       popupChild->SetPopupPosition(nsnull);
 
       // is the new size too small? Make sure we handle scrollbars correctly
@@ -204,8 +204,12 @@ nsPopupSetFrame::DoLayout(nsBoxLayoutState& aState)
       // real height for its inline element, but does once it is laid out.
       // This is bug 228673 which doesn't have a simple fix.
       if (popupChild->GetRect().width > bounds.width ||
-          popupChild->GetRect().height > bounds.height)
+          popupChild->GetRect().height > bounds.height) {
+        // the size after layout was larger than the preferred size,
+        // so set the preferred size accordingly
+        popupChild->SetPreferredSize(popupChild->GetSize());
         popupChild->SetPopupPosition(nsnull);
+      }
       popupChild->AdjustView();
     }
 

toolkit/content/tests/chrome/window_largemenu.xul

@@ -21,7 +21,8 @@
 var gOverflowed = false, gUnderflowed = false;
 var gScreenY = -1;
 var gTestIndex = 0;
-var gTests = ["open normal", "open flipped position", "open with scrolling", "open small again"];
+var gTests = ["open normal", "open flipped position", "open with scrolling",
+              "open after scrolling", "open small again"];
 
 function runTests()
 {
@@ -54,7 +55,7 @@ function nextTest()
       popup.appendChild(menu);
     }
   }
-  else if (gTestIndex == 3) {
+  else if (gTestIndex == 4) {
     for (var t = 1; t <= 30; t++)
       popup.removeChild(popup.lastChild);
   }
@@ -66,6 +67,9 @@ function popupShown()
 {
   var popup = document.getElementById("popup");
   var rect = popup.getBoundingClientRect();
+  var sbo = document.getAnonymousNodes(popup)[0].scrollBoxObject;
+  var expectedScrollPos = 0;
+
   if (gTestIndex == 0) {
     // the popup should be in the center of the screen 
     is(Math.round(rect.top) + gScreenY, screen.height / 2,
@@ -88,8 +92,14 @@ function popupShown()
     ok(Math.round(rect.top) + gScreenY >= screen.top, gTests[gTestIndex] + " top");
     ok(Math.round(rect.bottom) + gScreenY < screen.height, gTests[gTestIndex] + " bottom");
     ok(gOverflowed && !gUnderflowed, gTests[gTestIndex] + " overflow")
+
+    sbo.scrollTo(0, 40);
+    expectedScrollPos = 40;
   }
   else if (gTestIndex == 3) {
+    expectedScrollPos = 40;
+  }
+  else if (gTestIndex == 4) {
     is(Math.round(rect.top) + gScreenY, screen.height / 2,
                               gTests[gTestIndex] + " top");
     ok(Math.round(rect.bottom) + gScreenY < screen.height,
@@ -97,6 +107,10 @@ function popupShown()
     ok(!gOverflowed && gUnderflowed, gTests[gTestIndex] + " overflow")
   }
 
+  var sx = { }, sy = { };
+  sbo.getPosition(sx, sy);
+  is(sy.value, expectedScrollPos, "menu scroll position");
+
   popup.hidePopup();
 }