Bug 524545, popups with large margins appearing offscreen, r=roc
authorNeil Deakin <neil@mozilla.com>
Fri, 05 Aug 2011 15:24:24 -0400
changeset 73919 d0f0427519407c624fd3eb6fe3f9bbdd0aba2b4a
parent 73918 1c136ef5cac29d4c1f0d209ca547f6d479e217e1
child 73920 7eda7df522d1c276f41b580df7fd2d6bee756ecc
push id2
push userbsmedberg@mozilla.com
push dateFri, 19 Aug 2011 14:38:13 +0000
reviewersroc
bugs524545
milestone8.0a1
Bug 524545, popups with large margins appearing offscreen, r=roc
layout/xul/base/src/nsMenuPopupFrame.cpp
toolkit/content/tests/chrome/popup_trigger.js
toolkit/content/tests/chrome/window_popup_attribute.xul
toolkit/content/tests/chrome/window_popup_button.xul
toolkit/content/tests/widgets/popup_shared.js
toolkit/content/tests/widgets/test_arrowpanel.xul
--- a/layout/xul/base/src/nsMenuPopupFrame.cpp
+++ b/layout/xul/base/src/nsMenuPopupFrame.cpp
@@ -1061,16 +1061,17 @@ nsMenuPopupFrame::FlipOrResize(nscoord& 
 
       // check whether there is more room to the left and right (or top and
       // bottom) of the anchor and put the popup on the side with more room.
       if (aScreenEnd - endpos >= startpos - aScreenBegin) {
         if (mIsContextMenu) {
           aScreenPoint = aScreenEnd - aSize;
         }
         else {
+          aScreenPoint = endpos + aMarginBegin;
           popupSize = aScreenEnd - aScreenPoint;
         }
       }
       else {
         // flip such that the popup is to the left or top of the anchor point
         // instead.
         *aFlipSide = PR_TRUE;
         aScreenPoint = startpos - aSize - aMarginBegin - aOffsetForContextMenu;
@@ -1085,17 +1086,32 @@ nsMenuPopupFrame::FlipOrResize(nscoord& 
         }
       }
     }
     else {
       aScreenPoint = aScreenEnd - aSize;
     }
   }
 
-  return popupSize;
+  // Make sure that the point is within the screen boundaries and that the
+  // size isn't off the edge of the screen. This can happen when a large
+  // positive or negative margin is used.
+  if (aScreenPoint < aScreenBegin) {
+    aScreenPoint = aScreenBegin;
+  }
+  if (aScreenPoint > aScreenEnd) {
+    aScreenPoint = aScreenEnd - aSize;
+  }
+
+  // If popupSize ended up being negative, or the original size was actually
+  // smaller than the calculated popup size, just use the original size instead.
+  if (popupSize <= 0 || aSize < popupSize) {
+    popupSize = aSize;
+  }
+  return NS_MIN(popupSize, aScreenEnd - aScreenPoint);
 }
 
 nsresult
 nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame, PRBool aIsMove)
 {
   if (!mShouldAutoPosition)
     return NS_OK;
 
--- a/toolkit/content/tests/chrome/popup_trigger.js
+++ b/toolkit/content/tests/chrome/popup_trigger.js
@@ -59,16 +59,21 @@ var popupTests = [
     // triggered them
     is(gMenuPopup.triggerNode, gIsMenu ? null : gTrigger, testname + " triggerNode");
     is(document.popupNode, gIsMenu ? null : gTrigger, testname + " document.popupNode");
     is(document.tooltipNode, null, testname + " document.tooltipNode");
     // check to ensure the popup node for a different document isn't used
     if (window.opener)
       is(window.opener.document.popupNode, null, testname + " opener.document.popupNode");
 
+    // this will be used in some tests to ensure the size doesn't change
+    var popuprect = gMenuPopup.getBoundingClientRect();
+    gPopupWidth = Math.round(popuprect.width);
+    gPopupHeight = Math.round(popuprect.height);
+
     checkActive(gMenuPopup, "", testname);
     checkOpen("trigger", testname);
     // if a menu, the popup should be opened underneath the menu in the
     // 'after_start' position, otherwise it is opened at the mouse position
     if (gIsMenu)
       compareEdge(gTrigger, gMenuPopup, "after_start", 0, 0, testname);
   }
 },
@@ -241,16 +246,77 @@ var popupTests = [
     var rightmod = step == "before_end" || step == "after_end" ||
                    step == "start_before" || step == "start_after";
     var bottommod = step == "before_start" || step == "before_end" ||
                     step == "start_after" || step == "end_after";
     compareEdge(gTrigger, gMenuPopup, step, rightmod ? 8 : -8, bottommod ? 8 : -8, testname);
     gMenuPopup.removeAttribute("style");
   }
 },
+ {
+  testname: "open popup with large positive margin",
+  events: [ "popupshowing thepopup", "popupshown thepopup" ],
+  autohide: "thepopup",
+  steps: ["before_start", "before_end", "after_start", "after_end",
+          "start_before", "start_after", "end_before", "end_after", "after_pointer", "overlap"],
+  test: function(testname, step) {
+    gMenuPopup.setAttribute("style", "margin: 1000px;");
+    gMenuPopup.openPopup(gTrigger, step, 0, 0, false, false);
+  },
+  result: function(testname, step) {
+    var popuprect = gMenuPopup.getBoundingClientRect();
+    // as there is more room on the 'end' or 'after' side, popups will always
+    // appear on the right or bottom corners, depending on which side they are
+    // allowed to be flipped by.
+    var expectedleft = step == "before_end" || step == "after_end" ?
+                       0 : Math.round(window.innerWidth - gPopupWidth);
+    var expectedtop = step == "start_after" || step == "end_after" ?
+                      0 : Math.round(window.innerHeight - gPopupHeight);
+    is(Math.round(popuprect.left), expectedleft, testname + " x position " + step);
+    is(Math.round(popuprect.top), expectedtop, testname + " y position " + step);
+    gMenuPopup.removeAttribute("style");
+  }
+},
+{
+  testname: "open popup with large negative margin",
+  events: [ "popupshowing thepopup", "popupshown thepopup" ],
+  autohide: "thepopup",
+  steps: ["before_start", "before_end", "after_start", "after_end",
+          "start_before", "start_after", "end_before", "end_after", "after_pointer", "overlap"],
+  test: function(testname, step) {
+    gMenuPopup.setAttribute("style", "margin: -1000px;");
+    gMenuPopup.openPopup(gTrigger, step, 0, 0, false, false);
+  },
+  result: function(testname, step) {
+    var popuprect = gMenuPopup.getBoundingClientRect();
+    // using negative margins causes the reverse of positive margins, and
+    // popups will appear on the left or top corners.
+    var expectedleft = step == "before_end" || step == "after_end" ?
+                       Math.round(window.innerWidth - gPopupWidth) : 0;
+    var expectedtop = step == "start_after" || step == "end_after" ?
+                      Math.round(window.innerHeight - gPopupHeight) : 0;
+    is(Math.round(popuprect.left), expectedleft, testname + " x position " + step);
+    is(Math.round(popuprect.top), expectedtop, testname + " y position " + step);
+    gMenuPopup.removeAttribute("style");
+  }
+},
+{
+  testname: "popup with unknown step",
+  events: [ "popupshowing thepopup", "popupshown thepopup" ],
+  autohide: "thepopup",
+  test: function() {
+    gMenuPopup.openPopup(gTrigger, "other", 0, 0, false, false);
+  },
+  result: function (testname) {
+    var triggerrect = gMenuPopup.getBoundingClientRect();
+    var popuprect = gMenuPopup.getBoundingClientRect();
+    is(Math.round(popuprect.left), triggerrect.left, testname + " x position ");
+    is(Math.round(popuprect.top), triggerrect.top, testname + " y position ");
+  }
+},
 {
   // these tests check to ensure that the position attribute can be used
   // to set the position of a popup instead of passing it as an argument
   testname: "open popup anchored with attribute",
   events: [ "popupshowing thepopup", "popupshown thepopup" ],
   autohide: "thepopup",
   steps: ["before_start", "before_end", "after_start", "after_end",
           "start_before", "start_after", "end_before", "end_after", "after_pointer", "overlap",
--- a/toolkit/content/tests/chrome/window_popup_attribute.xul
+++ b/toolkit/content/tests/chrome/window_popup_attribute.xul
@@ -8,18 +8,18 @@
   <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>      
   <script type="application/javascript" src="popup_shared.js"></script>      
   <script type="application/javascript" src="popup_trigger.js"></script>      
 
 <script>
 window.opener.SimpleTest.waitForFocus(runTests, window);
 </script>
 
-<hbox style="margin-left: 325px; margin-top: 325px;">
-  <label id="trigger" popup="thepopup" value="Popup"/>
+<hbox style="margin-left: 200px; margin-top: 270px;">
+  <label id="trigger" popup="thepopup" value="Popup" height="60"/>
 </hbox>
 <!-- this frame is used to check that document.popupNode
      is inaccessible from different sources -->
 <iframe id="childframe" type="content" width="10" height="10"
         src="http://sectest2.example.org:80/chrome/toolkit/content/tests/chrome/popup_childframe_node.xul"/>
 
 <menupopup id="thepopup">
   <menuitem id="item1" label="First"/>
--- a/toolkit/content/tests/chrome/window_popup_button.xul
+++ b/toolkit/content/tests/chrome/window_popup_button.xul
@@ -8,18 +8,18 @@
   <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>      
   <script type="application/javascript" src="popup_shared.js"></script>      
   <script type="application/javascript" src="popup_trigger.js"></script>
 
 <script>
 window.opener.SimpleTest.waitForFocus(runTests, window);
 </script>
 
-<hbox style="margin-left: 325px; margin-top: 325px;">
-  <button id="trigger" type="menu" label="Popup">
+<hbox style="margin-left: 200px; margin-top: 270px;">
+  <button id="trigger" type="menu" label="Popup" width="100" height="50">
     <menupopup id="thepopup">
       <menuitem id="item1" label="First"/>
       <menuitem id="item2" label="Main Item"/>
       <menuitem id="amenu" label="A Menu" accesskey="M"/>
       <menuitem id="item3" label="Third"/>
       <menuitem id="one" label="One"/>
       <menuitem id="fancier" label="Fancier Menu"/>
       <menu id="submenu" label="Only Menu">
--- a/toolkit/content/tests/widgets/popup_shared.js
+++ b/toolkit/content/tests/widgets/popup_shared.js
@@ -31,16 +31,17 @@ const menuactiveAttribute = "_moz-menuac
 var gPopupTests = null;
 var gTestIndex = -1;
 var gTestStepIndex = 0;
 var gTestEventIndex = 0;
 var gAutoHide = false;
 var gExpectedEventDetails = null;
 var gExpectedTriggerNode = null;
 var gWindowUtils;
+var gPopupWidth = -1, gPopupHeight = -1;
 
 function startPopupTests(tests)
 {
   document.addEventListener("popupshowing", eventOccurred, false);
   document.addEventListener("popupshown", eventOccurred, false);
   document.addEventListener("popuphiding", eventOccurred, false);
   document.addEventListener("popuphidden", eventOccurred, false);
   document.addEventListener("command", eventOccurred, false);
@@ -340,19 +341,25 @@ function compareEdge(anchor, popup, edge
   testname += " " + edge;
 
   checkOpen(anchor.id, testname);
 
   var anchorrect = anchor.getBoundingClientRect();
   var popuprect = popup.getBoundingClientRect();
   var check1 = false, check2 = false;
 
-  ok((Math.round(popuprect.right) - Math.round(popuprect.left)) &&
-     (Math.round(popuprect.bottom) - Math.round(popuprect.top)),
-     testname + " size");
+  if (gPopupWidth == -1) {
+    ok((Math.round(popuprect.right) - Math.round(popuprect.left)) &&
+       (Math.round(popuprect.bottom) - Math.round(popuprect.top)),
+       testname + " size");
+  }
+  else {
+    is(Math.round(popuprect.width), gPopupWidth, testname + " width");
+    is(Math.round(popuprect.height), gPopupHeight, testname + " height");
+  }
 
   var spaceIdx = edge.indexOf(" ");
   if (spaceIdx > 0) {
     let cornerX, cornerY;
     let [anchor, align] = edge.split(" ");
     switch (anchor) {
       case "topleft": cornerX = anchorrect.left; cornerY = anchorrect.top; break;
       case "topcenter": cornerX = anchorrect.left + anchorrect.width / 2; cornerY = anchorrect.top; break;
--- a/toolkit/content/tests/widgets/test_arrowpanel.xul
+++ b/toolkit/content/tests/widgets/test_arrowpanel.xul
@@ -5,20 +5,20 @@
 <window title="Arrow Panels"
         style="padding: 10px;"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
 
   <script type="application/javascript" src="/MochiKit/packed.js"></script>
   <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>      
 
 <stack flex="1">
-  <label id="topleft" value="Top Left" left="0" top="0"/>
-  <label id="topright" value="Top Right" right="0" top="0"/>
-  <label id="bottomleft" value="Bottom Left" left="0" bottom="0"/>
-  <label id="bottomright" value="Bottom Right" right="0" bottom="0"/>
+  <label id="topleft" value="Top Left" left="15" top="15"/>
+  <label id="topright" value="Top Right" right="15" top="15"/>
+  <label id="bottomleft" value="Bottom Left" left="15" bottom="15"/>
+  <label id="bottomright" value="Bottom Right" right="15" bottom="15"/>
   <!-- Our SimpleTest/TestRunner.js runs tests inside an iframe which sizes are W=500 H=300.
        'left' and 'top' values need to be set so that the panel (popup) has enough room to display on its 4 sides. -->
   <label id="middle" value="+/- Centered" left="225" top="135"/>
   <iframe id="frame" src="data:text/html,&lt;input id='input'&gt;" width="100" height="100" left="225" top="180"/>
 </stack>
 
 <panel id="panel" type="arrow" onpopupshown="checkPanelPosition(this)" onpopuphidden="runNextTest.next()">
   <label id="panellabel" value="This is some text" height="80"/>