Bug 1253975, don't reset the scroll position of a menulist when it opens as it should scroll to its selection instead, r=mconley

browser/base/content/test/general/browser_selectpopup.js

@@ -368,6 +368,7 @@ add_task(function* test_large_popup() {
       select.add(new content.Option("Test" + i));
     }
 
+    select.options[60].selected = true;
     select.focus();
   });
 
@@ -388,6 +389,16 @@ add_task(function* test_large_popup() {
     ok(rect.top >= browserRect.top, "Popup top position in within browser area");
     ok(rect.bottom <= browserRect.bottom, "Popup bottom position in within browser area");
 
+    // Don't check the scroll position for the last step as the popup will be cut off.
+    if (positions.length == 1) {
+      let cs = window.getComputedStyle(selectPopup);
+      let bpBottom = parseFloat(cs.paddingBottom) + parseFloat(cs.borderBottomWidth);
+
+      is(selectPopup.childNodes[60].getBoundingClientRect().bottom,
+         selectPopup.getBoundingClientRect().bottom - bpBottom,
+         "Popup scroll at correct position " + bpBottom);
+    }
+
     yield hideSelectPopup(selectPopup, false);
 
     position = positions.shift();

layout/xul/nsMenuPopupFrame.cpp

@@ -445,12 +445,17 @@ nsMenuPopupFrame::LayoutPopup(nsBoxLayoutState& aState, nsIFrame* aParentMenu,
 
   // if the popup has just been opened, make sure the scrolled window is at 0,0
   if (mIsOpenChanged) {
-    nsIScrollableFrame *scrollframe = do_QueryFrame(nsBox::GetChildXULBox(this));
-    if (scrollframe) {
-      nsWeakFrame weakFrame(this);
-      scrollframe->ScrollTo(nsPoint(0,0), nsIScrollableFrame::INSTANT);
-      if (!weakFrame.IsAlive()) {
-        return;
+    // Don't scroll menulists as they will scroll to their selected item on their own.
+    nsCOMPtr<nsIDOMXULMenuListElement> menulist =
+      do_QueryInterface(aParentMenu ? aParentMenu->GetContent() : nullptr);
+    if (!menulist) {
+      nsIScrollableFrame *scrollframe = do_QueryFrame(nsBox::GetChildXULBox(this));
+      if (scrollframe) {
+        nsWeakFrame weakFrame(this);
+        scrollframe->ScrollTo(nsPoint(0,0), nsIScrollableFrame::INSTANT);
+        if (!weakFrame.IsAlive()) {
+          return;
+        }
       }
     }
   }

toolkit/content/tests/chrome/test_menulist_paging.xul

@@ -55,6 +55,21 @@
   </menupopup>
 </menulist>
 
+<menulist id="menulist4">
+  <menupopup id="menulist-popup4">
+    <label value="One"/>
+    <menuitem label="Two"/>
+    <menuitem label="Three"/>
+    <menuitem label="Four"/>
+    <menuitem label="Five"/>
+    <menuitem label="Six" selected="true"/>
+    <menuitem label="Seven"/>
+    <menuitem label="Eight"/>
+    <menuitem label="Nine"/>
+    <label value="Ten"/>
+  </menupopup>
+</menulist>
+
 <script class="testbody" type="application/javascript">
 <![CDATA[
 
@@ -65,13 +80,20 @@ let test;
 // Windows allows disabled items to be selected.
 let isWindows = navigator.platform.indexOf("Win") >= 0;
 
+// Fields:
+//  list - menulist id
+//  initial - initial selected index
+//  scroll - index of item at top of the visible scrolled area, -1 to skip this test
+//  downs - array of indicies that will be selected when pressing down in sequence
+//  ups - array of indicies that will be selected when pressing up in sequence
 let tests = [
-  { list: "menulist1", initial: 0, downs: [3, 6, 9, 9],
+  { list: "menulist1", initial: 0, scroll: 0, downs: [3, 6, 9, 9],
                                    ups: [6, 3, 0, 0] },
-  { list: "menulist2", initial: 1, downs: [4, 7, isWindows ? 9 : 8, isWindows ? 9 : 8],
+  { list: "menulist2", initial: 1, scroll: 0, downs: [4, 7, isWindows ? 9 : 8, isWindows ? 9 : 8],
                                    ups: [isWindows ? 6 : 5, isWindows ? 3 : 2, isWindows ? 0 : 1] },
-  { list: "menulist3", initial: 1, downs: [isWindows ? 4 : 6, isWindows ? 7 : 8, 8],
-                                   ups: [isWindows ? 5 : 3, isWindows ? 2 : 1, 1] }
+  { list: "menulist3", initial: 1, scroll: -1, downs: [isWindows ? 4 : 6, isWindows ? 7 : 8, 8],
+                                   ups: [isWindows ? 5 : 3, isWindows ? 2 : 1, 1] },
+  { list: "menulist4", initial: 5, scroll: 2, downs: [], ups: [] }
 ];
 
 function startTest()
@@ -86,6 +108,7 @@ function startTest()
   popup.height = height;
   document.getElementById("menulist-popup2").height = height;
   document.getElementById("menulist-popup3").height = height;
+  document.getElementById("menulist-popup4").height = height;
 
   runTest();
 }
@@ -106,6 +129,16 @@ function menulistShown()
   let menulist = document.getElementById(test.list);
   is(menulist.menuBoxObject.activeChild.label, menulist.getItemAtIndex(test.initial).label, test.list + " initial selection");
 
+  let cs = window.getComputedStyle(menulist.menupopup);
+  let bpTop = parseFloat(cs.paddingTop) + parseFloat(cs.borderTopWidth);
+
+  // Skip menulist3 as it has a label that scrolling doesn't need normally deal with.
+  if (test.scroll >= 0) {
+    is(menulist.menupopup.childNodes[test.scroll].getBoundingClientRect().top,
+       menulist.menupopup.getBoundingClientRect().top + bpTop,
+       "Popup scroll at correct position");
+  }
+
   for (let i = 0; i < test.downs.length; i++) {
     sendKey("PAGE_DOWN");
     is(menulist.menuBoxObject.activeChild.label, menulist.getItemAtIndex(test.downs[i]).label, test.list + " page down " + i);

toolkit/modules/SelectParentHelper.jsm

@@ -49,7 +49,6 @@ this.SelectParentHelper = {
                                      constraintRect.width, constraintRect.height);
     menulist.menupopup.setConstraintRect(constraintRect);
     menulist.menupopup.openPopupAtScreenRect("after_start", rect.left, rect.top, rect.width, rect.height, false, false);
-    menulist.selectedItem.scrollIntoView();
   },
 
   hide: function(menulist, browser) {