Bug 1253975, don't reset the scroll position of a menulist when it opens as it should scroll to its selection instead, r=mconley
authorNeil Deakin <neil@mozilla.com>
Mon, 25 Jul 2016 09:08:36 -0400
changeset 306549 59ab86516175
parent 306548 e5b7fe7de013
child 306550 6acbad99a3ff
push id30489
push usercbook@mozilla.com
push dateTue, 26 Jul 2016 09:56:19 +0000
treeherdermozilla-central@ff1ef8ec0fd8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley
bugs1253975
milestone50.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 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
layout/xul/nsMenuPopupFrame.cpp
toolkit/content/tests/chrome/test_menulist_paging.xul
toolkit/modules/SelectParentHelper.jsm
--- a/browser/base/content/test/general/browser_selectpopup.js
+++ b/browser/base/content/test/general/browser_selectpopup.js
@@ -363,16 +363,17 @@ add_task(function* test_large_popup() {
 
   yield ContentTask.spawn(tab.linkedBrowser, null, function*() {
     let doc = content.document;
     let select = doc.getElementById("one");
     for (var i = 0; i < 180; i++) {
       select.add(new content.Option("Test" + i));
     }
 
+    select.options[60].selected = true;
     select.focus();
   });
 
   let selectPopup = document.getElementById("ContentSelectDropdown").menupopup;
   let browserRect = tab.linkedBrowser.getBoundingClientRect();
 
   let positions = [
     "margin-top: 300px;",
@@ -383,16 +384,26 @@ add_task(function* test_large_popup() {
   let position;
   while (true) {
     yield openSelectPopup(selectPopup, false);
 
     let rect = selectPopup.getBoundingClientRect();
     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();
     if (!position) {
       break;
     }
 
     let contentPainted = BrowserTestUtils.contentPainted(tab.linkedBrowser);
--- a/layout/xul/nsMenuPopupFrame.cpp
+++ b/layout/xul/nsMenuPopupFrame.cpp
@@ -440,22 +440,27 @@ nsMenuPopupFrame::LayoutPopup(nsBoxLayou
     if (!shouldPosition && !aSizedToPopup) {
       RemoveStateBits(NS_FRAME_FIRST_REFLOW);
       return;
     }
   }
 
   // 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;
+        }
       }
     }
   }
 
   // get the preferred, minimum and maximum size. If the menu is sized to the
   // popup, then the popup's width is the menu's width.
   nsSize prefSize = GetXULPrefSize(aState);
   nsSize minSize = GetXULMinSize(aState); 
--- a/toolkit/content/tests/chrome/test_menulist_paging.xul
+++ b/toolkit/content/tests/chrome/test_menulist_paging.xul
@@ -50,47 +50,70 @@
     <menuitem label="Six" disabled="true"/>
     <menuitem label="Seven"/>
     <menuitem label="Eight"/>
     <menuitem label="Nine"/>
     <label value="Ten"/>
   </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[
 
 SimpleTest.waitForExplicitFinish();
 
 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()
 {
   let popup = document.getElementById("menulist-popup1");
   let menupopupHeight = popup.getBoundingClientRect().height;
   let menuitemHeight = popup.firstChild.getBoundingClientRect().height;
 
   // First, set the height of each popup to the height of four menuitems plus
   // any padding and border on the menupopup.
   let height = menuitemHeight * 4 + (menupopupHeight - menuitemHeight * 10);
   popup.height = height;
   document.getElementById("menulist-popup2").height = height;
   document.getElementById("menulist-popup3").height = height;
+  document.getElementById("menulist-popup4").height = height;
 
   runTest();
 }
 
 function runTest()
 {
   if (!tests.length) {
     SimpleTest.finish();
@@ -101,16 +124,26 @@ function runTest()
   document.getElementById(test.list).open = true;
 }
 
 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);
   }
 
   for (let i = 0; i < test.ups.length; i++) {
     sendKey("PAGE_UP");
     is(menulist.menuBoxObject.activeChild.label, menulist.getItemAtIndex(test.ups[i]).label, test.list + " page up " + i);
--- a/toolkit/modules/SelectParentHelper.jsm
+++ b/toolkit/modules/SelectParentHelper.jsm
@@ -44,17 +44,16 @@ this.SelectParentHelper = {
     }
 
     let constraintRect = browser.getBoundingClientRect();
     constraintRect = new win.DOMRect(constraintRect.left + win.mozInnerScreenX,
                                      constraintRect.top + win.mozInnerScreenY,
                                      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) {
     if (currentBrowser == browser) {
       menulist.menupopup.hidePopup();
     }
   },