Bug 524545, popups with large margins appearing offscreen, r=roc
authorNeil Deakin <neil@mozilla.com>
Fri, 05 Aug 2011 15:24:24 -0400
changeset 73917 d0f0427519407c624fd3eb6fe3f9bbdd0aba2b4a
parent 73916 1c136ef5cac29d4c1f0d209ca547f6d479e217e1
child 73918 7eda7df522d1c276f41b580df7fd2d6bee756ecc
push id20926
push usermak77@bonardo.net
push dateSat, 06 Aug 2011 09:36:25 +0000
treeherdermozilla-central@ba21778fcc14 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs524545
milestone8.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 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"/>