Bug 1313131, don't wrap when using cursor navigation in menus onr dropdowns on Mac, or dropdowns on Windows, r=ksteuber

accessible/tests/mochitest/events/test_focus_listcontrols.xul

@@ -52,7 +52,7 @@
       gQueue.push(new synthEscapeKey("ml_marmalade", new focusChecker("menulist")));
 if (!MAC) {
       // On Windows, items get selected during navigation.
-      let expectedItem = WIN ? "ml_tangerine" : "ml_marmalade";
+      let expectedItem = WIN ? "ml_strawberry" : "ml_marmalade";
       gQueue.push(new synthDownKey("menulist", new nofocusChecker(expectedItem)));
       gQueue.push(new synthOpenComboboxKey("menulist", new focusChecker(expectedItem)));
       gQueue.push(new synthEnterKey(expectedItem, new focusChecker("menulist")));
@@ -174,6 +174,7 @@ if (!MAC) {
         <menupopup>
           <menuitem id="ml_tangerine" label="tangerine trees"/>
           <menuitem id="ml_marmalade" label="marmalade skies"/>
+          <menuitem id="ml_strawberry" label="strawberry fields"/>
         </menupopup>
       </menulist>
       <menulist id="emenulist" editable="true">

layout/xul/nsMenuBarFrame.cpp

@@ -147,7 +147,7 @@ nsMenuBarFrame::ToggleMenuActiveState()
     // Set the active menu to be the top left item (e.g., the File menu).
     // We use an attribute called "menuactive" to track the current 
     // active menu.
-    nsMenuFrame* firstFrame = nsXULPopupManager::GetNextMenuItem(this, nullptr, false);
+    nsMenuFrame* firstFrame = nsXULPopupManager::GetNextMenuItem(this, nullptr, false, false);
     if (firstFrame) {
       // Activate the menu bar
       SetActive(true);

layout/xul/nsMenuFrame.cpp

@@ -1004,7 +1004,7 @@ nsMenuFrame::UpdateMenuSpecialState()
   // get the first sibling in this menu popup. This frame may be it, and if we're
   // being called at creation time, this frame isn't yet in the parent's child list.
   // All I'm saying is that this may fail, but it's most likely alright.
-  nsIFrame* firstMenuItem = nsXULPopupManager::GetNextMenuItem(GetParent(), nullptr, true);
+  nsIFrame* firstMenuItem = nsXULPopupManager::GetNextMenuItem(GetParent(), nullptr, true, false);
   nsIFrame* sib = firstMenuItem;
   while (sib) {
     nsMenuFrame* menu = do_QueryFrame(sib);
@@ -1018,7 +1018,7 @@ nsMenuFrame::UpdateMenuSpecialState()
         return;
       }
     }
-    sib = nsXULPopupManager::GetNextMenuItem(GetParent(), menu, true);
+    sib = nsXULPopupManager::GetNextMenuItem(GetParent(), menu, true, true);
     if (sib == firstMenuItem) {
       break;
     }

layout/xul/nsMenuPopupFrame.cpp

@@ -1838,7 +1838,7 @@ void nsMenuPopupFrame::ChangeByPage(bool aIsUp)
     // just use this as the newMenu and leave currentMenu null so that no
     // check for a later element is performed. When moving down, set currentMenu
     // so that we look for one page down from the first item.
-    newMenu = nsXULPopupManager::GetNextMenuItem(this, nullptr, true);
+    newMenu = nsXULPopupManager::GetNextMenuItem(this, nullptr, true, false);
     if (!aIsUp) {
       currentMenu = newMenu;
     }
@@ -2066,7 +2066,7 @@ nsMenuPopupFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent, bool& doAction
   //       been destroyed already.  One strategy would be to 
   //       setTimeout(<func>,0) as detailed in:
   //       <http://bugzilla.mozilla.org/show_bug.cgi?id=126675#c32>
-  nsIFrame* firstMenuItem = nsXULPopupManager::GetNextMenuItem(immediateParent, nullptr, true);
+  nsIFrame* firstMenuItem = nsXULPopupManager::GetNextMenuItem(immediateParent, nullptr, true, false);
   nsIFrame* currFrame = firstMenuItem;
 
   int32_t menuAccessKey = -1;
@@ -2131,7 +2131,7 @@ nsMenuPopupFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent, bool& doAction
     }
 
     nsMenuFrame* menu = do_QueryFrame(currFrame);
-    currFrame = nsXULPopupManager::GetNextMenuItem(immediateParent, menu, true);
+    currFrame = nsXULPopupManager::GetNextMenuItem(immediateParent, menu, true, true);
     if (currFrame == firstMenuItem)
       break;
   }

layout/xul/nsXULPopupManager.cpp

@@ -945,7 +945,7 @@ nsXULPopupManager::ShowPopupCallback(nsIContent* aPopup,
   }
 
   if (aSelectFirstItem) {
-    nsMenuFrame* next = GetNextMenuItem(aPopupFrame, nullptr, true);
+    nsMenuFrame* next = GetNextMenuItem(aPopupFrame, nullptr, true, false);
     aPopupFrame->SetCurrentMenuItem(next);
   }
 
@@ -2207,8 +2207,8 @@ nsXULPopupManager::HandleKeyboardNavigation(uint32_t aKeyCode)
   
     if (NS_DIRECTION_IS_INLINE(theDirection)) {
       nsMenuFrame* nextItem = (theDirection == eNavigationDirection_End) ?
-                              GetNextMenuItem(mActiveMenuBar, currentMenu, false) : 
-                              GetPreviousMenuItem(mActiveMenuBar, currentMenu, false);
+                              GetNextMenuItem(mActiveMenuBar, currentMenu, false, true) : 
+                              GetPreviousMenuItem(mActiveMenuBar, currentMenu, false, true);
       mActiveMenuBar->ChangeMenuItem(nextItem, true, true);
       return true;
     }
@@ -2243,7 +2243,7 @@ nsXULPopupManager::HandleKeyboardNavigationInPopup(nsMenuChainItem* item,
     // We've been opened, but we haven't had anything selected.
     // We can handle End, but our parent handles Start.
     if (aDir == eNavigationDirection_End) {
-      nsMenuFrame* nextItem = GetNextMenuItem(aFrame, nullptr, true);
+      nsMenuFrame* nextItem = GetNextMenuItem(aFrame, nullptr, true, false);
       if (nextItem) {
         aFrame->ChangeMenuItem(nextItem, false, true);
         return true;
@@ -2277,14 +2277,28 @@ nsXULPopupManager::HandleKeyboardNavigationInPopup(nsMenuChainItem* item,
       NS_DIRECTION_IS_BLOCK_TO_EDGE(aDir)) {
     nsMenuFrame* nextItem;
 
-    if (aDir == eNavigationDirection_Before)
-      nextItem = GetPreviousMenuItem(aFrame, currentMenu, true);
-    else if (aDir == eNavigationDirection_After)
-      nextItem = GetNextMenuItem(aFrame, currentMenu, true);
-    else if (aDir == eNavigationDirection_First)
-      nextItem = GetNextMenuItem(aFrame, nullptr, true);
-    else
-      nextItem = GetPreviousMenuItem(aFrame, nullptr, true);
+    if (aDir == eNavigationDirection_Before ||
+        aDir == eNavigationDirection_After) {
+      // Cursor navigation does not wrap on Mac or for menulists on Windows.
+      bool wrap =
+#ifdef XP_WIN
+        aFrame->IsMenuList() ? false : true;
+#elif defined XP_MACOSX
+        false;
+#else
+        true;
+#endif
+
+      if (aDir == eNavigationDirection_Before) {
+        nextItem = GetPreviousMenuItem(aFrame, currentMenu, true, wrap);
+      } else {
+        nextItem = GetNextMenuItem(aFrame, currentMenu, true, wrap);
+      }
+    } else if (aDir == eNavigationDirection_First) {
+      nextItem = GetNextMenuItem(aFrame, nullptr, true, false);
+    } else {
+      nextItem = GetPreviousMenuItem(aFrame, nullptr, true, false);
+    }
 
     if (nextItem) {
       aFrame->ChangeMenuItem(nextItem, false, true);
@@ -2406,7 +2420,8 @@ nsXULPopupManager::HandleKeyboardEventWithKeyCode(
 nsMenuFrame*
 nsXULPopupManager::GetNextMenuItem(nsContainerFrame* aParent,
                                    nsMenuFrame* aStart,
-                                   bool aIsPopup)
+                                   bool aIsPopup,
+                                   bool aWrap)
 {
   nsPresContext* presContext = aParent->PresContext();
   auto insertion = presContext->PresShell()->
@@ -2441,6 +2456,10 @@ nsXULPopupManager::GetNextMenuItem(nsContainerFrame* aParent,
       currFrame = currFrame->GetNextSibling();
   }
 
+  if (!aWrap) {
+    return aStart;
+  }
+
   currFrame = immediateParent->PrincipalChildList().FirstChild();
 
   // Still don't have anything. Try cycling from the beginning.
@@ -2467,7 +2486,8 @@ nsXULPopupManager::GetNextMenuItem(nsContainerFrame* aParent,
 nsMenuFrame*
 nsXULPopupManager::GetPreviousMenuItem(nsContainerFrame* aParent,
                                        nsMenuFrame* aStart,
-                                       bool aIsPopup)
+                                       bool aIsPopup,
+                                       bool aWrap)
 {
   nsPresContext* presContext = aParent->PresContext();
   auto insertion = presContext->PresShell()->
@@ -2506,6 +2526,10 @@ nsXULPopupManager::GetPreviousMenuItem(nsContainerFrame* aParent,
       currFrame = currFrame->GetPrevSibling();
   }
 
+  if (!aWrap) {
+    return aStart;
+  }
+
   currFrame = frames.LastChild();
 
   // Still don't have anything. Try cycling from the end.

layout/xul/nsXULPopupManager.h

@@ -368,12 +368,16 @@ public:
   //          returns the item before it, while GetNextMenuItem returns the
   //          item after it.
   // aIsPopup - true for menupopups, false for menubars
+  // aWrap - true to wrap around to the beginning and continue searching if not
+  //         found. False to end at the beginning or end of the menu.
   static nsMenuFrame* GetPreviousMenuItem(nsContainerFrame* aParent,
                                           nsMenuFrame* aStart,
-                                          bool aIsPopup);
+                                          bool aIsPopup,
+                                          bool aWrap);
   static nsMenuFrame* GetNextMenuItem(nsContainerFrame* aParent,
                                       nsMenuFrame* aStart,
-                                      bool aIsPopup);
+                                      bool aIsPopup,
+                                      bool aWrap);
 
   // returns true if the menu item aContent is a valid menuitem which may
   // be navigated to. aIsPopup should be true for items on a popup, or false

toolkit/content/tests/chrome/popup_trigger.js

@@ -88,16 +88,20 @@ var popupTests = [
 {
   // check that pressing cursor up wraps and highlights the last item
   testname: "cursor up wrap",
-  events: [ "DOMMenuItemInactive item1", "DOMMenuItemActive last" ],
+  events: function () {
+    // No wrapping on menus on Mac
+    return platformIsMac() ? [] : [ "DOMMenuItemInactive item1", "DOMMenuItemActive last" ]
+  },
   test: function() { synthesizeKey("VK_UP", { }); },
   result: function(testname) {
-    checkActive(gMenuPopup, "last", testname);
+    checkActive(gMenuPopup, platformIsMac() ? "item1" : "last", testname);
   }
 },
 {
   // check that pressing cursor down wraps and highlights the first item
   testname: "cursor down wrap",
-  events: [ "DOMMenuItemInactive last", "DOMMenuItemActive item1" ],
+  condition: function () { return !platformIsMac() },
+  events: ["DOMMenuItemInactive last", "DOMMenuItemActive item1" ],
   test: function() { synthesizeKey("VK_DOWN", { }); },
   result: function(testname) { checkActive(gMenuPopup, "item1", testname); }
 },

toolkit/content/tests/chrome/test_menulist_keynav.xul

@@ -41,6 +41,7 @@ var list = $("list");
 let expectCommandEvent;
 
 var iswin = (navigator.platform.indexOf("Win") == 0);
+var ismac = (navigator.platform.indexOf("Mac") == 0);
 
 function runTests()
 {
@@ -48,20 +49,28 @@ function runTests()
 
   // on Mac, up and cursor keys open the menu, but on other platforms, the
   // cursor keys navigate between items without opening the menu
-  if (navigator.platform.indexOf("Mac") == -1) {
+  if (!ismac) {
     expectCommandEvent = true;
-    keyCheck(list, "VK_DOWN", 2, "cursor down");
-    keyCheck(list, "VK_DOWN", 3, "cursor down skip disabled");
-    keyCheck(list, "VK_UP", 2, "cursor up skip disabled");
-    keyCheck(list, "VK_UP", 1, "cursor up");
-    keyCheck(list, "VK_UP", 4, "cursor up wrap");
-    keyCheck(list, "VK_DOWN", 1, "cursor down wrap");
+    keyCheck(list, "VK_DOWN", 2, 1, "cursor down");
+    keyCheck(list, "VK_DOWN", 3, 1, "cursor down skip disabled");
+    keyCheck(list, "VK_UP", 2, 1, "cursor up skip disabled");
+    keyCheck(list, "VK_UP", 1, 1, "cursor up");
+
+    // On Windows, wrapping doesn't occur.
+    keyCheck(list, "VK_UP", iswin ? 1 : 4, 1, "cursor up wrap");
+
+    list.selectedIndex = 4;
+    list.menuBoxObject.activeChild = list.selectedItem;
+    keyCheck(list, "VK_DOWN", iswin ? 4 : 1, 4, "cursor down wrap");
+
+    list.selectedIndex = 0;
+    list.menuBoxObject.activeChild = list.selectedItem;
   }
 
   // check that attempting to open the menulist does not change the selection
-  synthesizeKey("VK_DOWN", { altKey: navigator.platform.indexOf("Mac") == -1 });
+  synthesizeKey("VK_DOWN", { altKey: !ismac });
   is(list.selectedItem, $("i1"), "open menulist down selectedItem");
-  synthesizeKey("VK_UP", { altKey: navigator.platform.indexOf("Mac") == -1 });
+  synthesizeKey("VK_UP", { altKey: !ismac });
   is(list.selectedItem, $("i1"), "open menulist up selectedItem");
 
   list.selectedItem = $("i1");
@@ -78,11 +87,11 @@ function pressLetter()
   synthesizeKey("G", { });
   is(list.selectedItem, $("i1"), "letter pressed not found selectedItem");
 
-  keyCheck(list, "T", 2, "letter pressed");
+  keyCheck(list, "T", 2, 1, "letter pressed");
 
   if (!gOpenPhase) {
     SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // prevent to timeout
-    keyCheck(list, "T", 2, "same letter pressed");
+    keyCheck(list, "T", 2, 1, "same letter pressed");
     SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");
   }
 
@@ -91,16 +100,16 @@ function pressLetter()
 
 function pressedAgain()
 {
-  keyCheck(list, "T", 3, "letter pressed again");
+  keyCheck(list, "T", 3, 1, "letter pressed again");
   SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // prevent to timeout
-  keyCheck(list, "W", 2, "second letter pressed");
+  keyCheck(list, "W", 2, 1, "second letter pressed");
   SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");
   setTimeout(differentPressed, 1200); 
 }
 
 function differentPressed()
 {
-  keyCheck(list, "O", 1, "different letter pressed");
+  keyCheck(list, "O", 1, 1, "different letter pressed");
 
   if (gOpenPhase) {
     list.open = false;
@@ -133,7 +142,7 @@ function tabAndScroll()
 {
   list = $("list");
 
-  if (navigator.platform.indexOf("Mac") == -1) {
+  if (!ismac) {
     $("button1").focus();
     synthesizeKeyExpectEvent("VK_TAB", { }, list, "focus", "focus to menulist");
     synthesizeKeyExpectEvent("VK_TAB", { }, $("button2"), "focus", "focus to button");
@@ -176,11 +185,13 @@ function tabAndScroll()
   checkEnter();
 }
 
-function keyCheck(list, key, index, testname)
+function keyCheck(list, key, index, defaultindex, testname)
 {
   var item = $("i" + index);
+  var defaultitem = $("i" + defaultindex || 1);
+
   synthesizeKeyExpectEvent(key, { }, item, expectCommandEvent ? "command" : "!command", testname);
-  is(list.selectedItem, expectCommandEvent ? item : $("i1"), testname + " selectedItem");
+  is(list.selectedItem, expectCommandEvent ? item : defaultitem, testname + " selectedItem" + "----" + list.selectedItem.id);
 }
 
 function checkModifiers(event)
@@ -243,6 +254,20 @@ function checkCursorNavigation()
   is(list.selectedIndex, iswin ? 3 : 1, "selectedIndex after page down");
   is(commandEventsCount, iswin ? 2 : 0, "selectedIndex after page down command event");
   is(list.menupopup.state, "open", "page down popup state");
+
+  // Check whether cursor up and down wraps.
+  list.selectedIndex = 0;
+  list.menuBoxObject.activeChild = list.selectedItem;
+  synthesizeKey("VK_UP", { });
+  is(list.menuBoxObject.activeChild,
+     document.getElementById(iswin || ismac ? "b1" : "b4"), "cursor up wrap while open");
+
+  list.selectedIndex = 3;
+  list.menuBoxObject.activeChild = list.selectedItem;
+  synthesizeKey("VK_DOWN", { });
+  is(list.menuBoxObject.activeChild,
+     document.getElementById(iswin || ismac ? "b4" : "b1"), "cursor down wrap while open");
+
   list.open = false;
 }
 

toolkit/content/tests/widgets/popup_shared.js

@@ -247,8 +247,11 @@ function goNextStepSync()
     test.test(test.testname, step);
 
     // no events to check for so just check the result
-    if (!("events" in test))
+    if (!("events" in test)) {
       checkResult();
+    } else if (typeof test.events == "function" && !test.events().length) {
+      checkResult();
+    }
   }
   else {
     finish();