Bug 1149745, on Windows, menulist should select the value when the cursor keys are used to navigate items, r=neil
authorNeil Deakin <neil@mozilla.com>
Fri, 26 Jun 2015 09:32:25 -0700
changeset 250331 11373e6e556f02838d8e037c13d4b4c4b3597f9b
parent 250330 a03152790896f9304cf6b79e48de91cc0388cc64
child 250332 9798c03e390991fd2d2e50f089d90bd78b7ceeae
push id61511
push userneil@mozilla.com
push dateFri, 26 Jun 2015 16:33:01 +0000
treeherdermozilla-inbound@11373e6e556f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersneil
bugs1149745
milestone41.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 1149745, on Windows, menulist should select the value when the cursor keys are used to navigate items, r=neil
accessible/tests/mochitest/events/test_focus_listcontrols.xul
browser/base/content/test/general/browser_selectpopup.js
layout/xul/nsMenuBarFrame.cpp
layout/xul/nsMenuBarFrame.h
layout/xul/nsMenuFrame.cpp
layout/xul/nsMenuParent.h
layout/xul/nsMenuPopupFrame.cpp
layout/xul/nsMenuPopupFrame.h
layout/xul/nsXULPopupManager.cpp
toolkit/content/tests/chrome/test_menulist.xul
toolkit/content/tests/chrome/test_menulist_keynav.xul
toolkit/modules/SelectContentHelper.jsm
toolkit/modules/SelectParentHelper.jsm
--- a/accessible/tests/mochitest/events/test_focus_listcontrols.xul
+++ b/accessible/tests/mochitest/events/test_focus_listcontrols.xul
@@ -46,19 +46,21 @@
       gQueue.push(new synthDownKey("msrlb_item1", new focusChecker("msrlb_item2"), { shiftKey: true }));
       gQueue.push(new synthFocus("emptyrichlistbox", new focusChecker("emptyrichlistbox")));
 
       gQueue.push(new synthFocus("menulist"));
       gQueue.push(new synthClick("menulist", new focusChecker("ml_tangerine")));
       gQueue.push(new synthDownKey("ml_tangerine", new focusChecker("ml_marmalade")));
       gQueue.push(new synthEscapeKey("ml_marmalade", new focusChecker("menulist")));
 if (!MAC) {
-      gQueue.push(new synthDownKey("menulist", new nofocusChecker("ml_marmalade")));
-      gQueue.push(new synthOpenComboboxKey("menulist", new focusChecker("ml_marmalade")));
-      gQueue.push(new synthEnterKey("ml_marmalade", new focusChecker("menulist")));
+      // On Windows, items get selected during navigation.
+      let expectedItem = WIN ? "ml_tangerine" : "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")));
 } else {
       todo(false, "Bug 746531 - timeouts of last three menulist tests on OS X");
 }
 
       var textentry = getAccessible("emenulist").firstChild;
       gQueue.push(new synthFocus("emenulist", new focusChecker(textentry)));
       gQueue.push(new synthDownKey(textentry, new nofocusChecker("eml_tangerine")));
       gQueue.push(new synthUpKey(textentry, new focusChecker("eml_marmalade")));
@@ -68,17 +70,17 @@ if (!MAC) {
 
       // no focus events for unfocused list controls when current item is
       // changed.
       gQueue.push(new synthFocus("emptylistbox"));
 
       gQueue.push(new changeCurrentItem("listbox", "lb_item1"));
       gQueue.push(new changeCurrentItem("richlistbox", "rlb_item1"));
 if (!MAC) {
-      gQueue.push(new changeCurrentItem("menulist", "ml_tangerine"));
+      gQueue.push(new changeCurrentItem("menulist", WIN ? "ml_marmalade" : "ml_tangerine"));
 }
       gQueue.push(new changeCurrentItem("emenulist", "eml_tangerine"));
 
       gQueue.invoke(); // Will call SimpleTest.finish();
     }
 
     SimpleTest.waitForExplicitFinish();
     addA11yLoadEvent(doTests);
--- a/browser/base/content/test/general/browser_selectpopup.js
+++ b/browser/base/content/test/general/browser_selectpopup.js
@@ -52,42 +52,48 @@ add_task(function*() {
 
   yield SimpleTest.promiseFocus(browser.contentWindow);
 
   let menulist = document.getElementById("ContentSelectDropdown");
   let selectPopup = menulist.menupopup;
 
   yield openSelectPopup(selectPopup);
 
+  let isWindows = navigator.platform.indexOf("Win") >= 0;
+
   is(menulist.selectedIndex, 1, "Initial selection");
   is(selectPopup.firstChild.localName, "menucaption", "optgroup is caption");
   is(selectPopup.firstChild.getAttribute("label"), "First Group", "optgroup label");
   is(selectPopup.childNodes[1].localName, "menuitem", "option is menuitem");
   is(selectPopup.childNodes[1].getAttribute("label"), "One", "option label");
 
   EventUtils.synthesizeKey("KEY_ArrowDown", { code: "ArrowDown" });
   is(menulist.menuBoxObject.activeChild, menulist.getItemAtIndex(2), "Select item 2");
+  is(menulist.selectedIndex, isWindows ? 2 : 1, "Select item 2 selectedIndex");
 
   EventUtils.synthesizeKey("KEY_ArrowDown", { code: "ArrowDown" });
   is(menulist.menuBoxObject.activeChild, menulist.getItemAtIndex(3), "Select item 3");
+  is(menulist.selectedIndex, isWindows ? 3 : 1, "Select item 3 selectedIndex");
 
   EventUtils.synthesizeKey("KEY_ArrowDown", { code: "ArrowDown" });
 
   // On Windows, one can navigate on disabled menuitems
-  let expectedIndex = (navigator.platform.indexOf("Win") >= 0) ? 5 : 9;
+  let expectedIndex = isWindows ? 5 : 9;
      
   is(menulist.menuBoxObject.activeChild, menulist.getItemAtIndex(expectedIndex),
      "Skip optgroup header and disabled items select item 7");
+  is(menulist.selectedIndex, isWindows ? 5 : 1, "Select or skip disabled item selectedIndex");
 
   for (let i = 0; i < 10; i++) {
     is(menulist.getItemAtIndex(i).disabled, i >= 4 && i <= 7, "item " + i + " disabled")
   }
 
   EventUtils.synthesizeKey("KEY_ArrowUp", { code: "ArrowUp" });
   is(menulist.menuBoxObject.activeChild, menulist.getItemAtIndex(3), "Select item 3 again");
+  is(menulist.selectedIndex, isWindows ? 3 : 1, "Select item 3 selectedIndex");
 
   yield hideSelectPopup(selectPopup);
 
   is(menulist.selectedIndex, 3, "Item 3 still selected");
 
   gBrowser.removeCurrentTab();
 });
 
--- a/layout/xul/nsMenuBarFrame.cpp
+++ b/layout/xul/nsMenuBarFrame.cpp
@@ -312,17 +312,18 @@ private:
   nsCOMPtr<nsIContent> mMenuBar;
   nsCOMPtr<nsIContent> mOldMenu;
   nsCOMPtr<nsIContent> mNewMenu;
   bool mSelectFirstItem;
 };
 
 NS_IMETHODIMP
 nsMenuBarFrame::ChangeMenuItem(nsMenuFrame* aMenuItem,
-                               bool aSelectFirstItem)
+                               bool aSelectFirstItem,
+                               bool aFromKey)
 {
   if (mCurrentMenu == aMenuItem)
     return NS_OK;
 
   // check if there's an open context menu, we ignore this
   nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
   if (pm && pm->HasContextMenu(nullptr))
     return NS_OK;
--- a/layout/xul/nsMenuBarFrame.h
+++ b/layout/xul/nsMenuBarFrame.h
@@ -30,17 +30,19 @@ public:
   NS_DECL_FRAMEARENA_HELPERS
 
   explicit nsMenuBarFrame(nsStyleContext* aContext);
 
   // nsMenuParent interface
   virtual nsMenuFrame* GetCurrentMenuItem() override;
   NS_IMETHOD SetCurrentMenuItem(nsMenuFrame* aMenuItem) override;
   virtual void CurrentMenuIsBeingDestroyed() override;
-  NS_IMETHOD ChangeMenuItem(nsMenuFrame* aMenuItem, bool aSelectFirstItem) override;
+  NS_IMETHOD ChangeMenuItem(nsMenuFrame* aMenuItem,
+                            bool aSelectFirstItem,
+                            bool aFromKey) override;
 
   NS_IMETHOD SetActive(bool aActiveFlag) override; 
 
   virtual bool IsMenuBar() override { return true; }
   virtual bool IsContextMenu() override { return false; }
   virtual bool IsActive() override { return mIsActive; }
   virtual bool IsMenu() override { return false; }
   virtual bool IsOpen() override { return true; } // menubars are considered always open
--- a/layout/xul/nsMenuFrame.cpp
+++ b/layout/xul/nsMenuFrame.cpp
@@ -472,30 +472,30 @@ nsMenuFrame::HandleEvent(nsPresContext* 
     // Deactivate the menu.
     if (menuParent) {
       bool onmenubar = menuParent->IsMenuBar();
       if (!(onmenubar && menuParent->IsActive())) {
         if (IsMenu() && !onmenubar && IsOpen()) {
           // Submenus don't get closed up immediately.
         }
         else if (this == menuParent->GetCurrentMenuItem()) {
-          menuParent->ChangeMenuItem(nullptr, false);
+          menuParent->ChangeMenuItem(nullptr, false, false);
         }
       }
     }
   }
   else if (aEvent->message == NS_MOUSE_MOVE &&
            (onmenu || (menuParent && menuParent->IsMenuBar()))) {
     if (gEatMouseMove) {
       gEatMouseMove = false;
       return NS_OK;
     }
 
     // Let the menu parent know we're the new item.
-    menuParent->ChangeMenuItem(this, false);
+    menuParent->ChangeMenuItem(this, false, false);
     NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
     NS_ENSURE_TRUE(menuParent, NS_OK);
 
     // we need to check if we really became the current menu
     // item or not
     nsMenuFrame *realCurrentItem = menuParent->GetCurrentMenuItem();
     if (realCurrentItem != this) {
       // we didn't (presumably because a context menu was active)
@@ -1428,25 +1428,25 @@ NS_IMETHODIMP
 nsMenuFrame::SetActiveChild(nsIDOMElement* aChild)
 {
   nsMenuPopupFrame* popupFrame = GetPopup();
   if (!popupFrame)
     return NS_ERROR_FAILURE;
 
   if (!aChild) {
     // Remove the current selection
-    popupFrame->ChangeMenuItem(nullptr, false);
+    popupFrame->ChangeMenuItem(nullptr, false, false);
     return NS_OK;
   }
 
   nsCOMPtr<nsIContent> child(do_QueryInterface(aChild));
 
   nsMenuFrame* menu = do_QueryFrame(child->GetPrimaryFrame());
   if (menu)
-    popupFrame->ChangeMenuItem(menu, false);
+    popupFrame->ChangeMenuItem(menu, false, false);
   return NS_OK;
 }
 
 nsIScrollableFrame* nsMenuFrame::GetScrollTargetFrame()
 {
   nsMenuPopupFrame* popupFrame = GetPopup();
   if (!popupFrame)
     return nullptr;
--- a/layout/xul/nsMenuParent.h
+++ b/layout/xul/nsMenuParent.h
@@ -25,18 +25,21 @@ public:
   NS_IMETHOD SetCurrentMenuItem(nsMenuFrame* aMenuItem) = 0;
   // indicate that the current menu frame is being destroyed, so clear the
   // current menu item
   virtual void CurrentMenuIsBeingDestroyed() = 0;
   // deselects the current item and closes its popup if any, then selects the
   // new item aMenuItem. For a menubar, if another menu is already open, the
   // new menu aMenuItem is opened. In this case, if aSelectFirstItem is true,
   // select the first item in it. For menupopups, the menu is not opened and
-  // the aSelectFirstItem argument is not used.
-  NS_IMETHOD ChangeMenuItem(nsMenuFrame* aMenuItem, bool aSelectFirstItem) = 0;
+  // the aSelectFirstItem argument is not used. The aFromKey argument indicates
+  // that the keyboard was used to navigate to the new menu item.
+  NS_IMETHOD ChangeMenuItem(nsMenuFrame* aMenuItem,
+                            bool aSelectFirstItem,
+                            bool aFromKey) = 0;
 
   // returns true if the menupopup is open. For menubars, returns false.
   virtual bool IsOpen() = 0;
   // returns true if the menubar is currently active. For menupopups, returns false.
   virtual bool IsActive() = 0;
   // returns true if this is a menubar. If false, it is a popup
   virtual bool IsMenuBar() = 0;
   // returns true if this is a menu, which has a tag of menupopup or popup.
--- a/layout/xul/nsMenuPopupFrame.cpp
+++ b/layout/xul/nsMenuPopupFrame.cpp
@@ -40,16 +40,17 @@
 #include "nsIDocShellTreeOwner.h"
 #include "nsIBaseWindow.h"
 #include "nsISound.h"
 #include "nsIScreenManager.h"
 #include "nsIServiceManager.h"
 #include "nsThemeConstants.h"
 #include "nsTransitionManager.h"
 #include "nsDisplayList.h"
+#include "nsIDOMXULSelectCntrlItemEl.h"
 #include "mozilla/EventDispatcher.h"
 #include "mozilla/EventStateManager.h"
 #include "mozilla/EventStates.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/LookAndFeel.h"
 #include "mozilla/MouseEvents.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/PopupBoxObject.h"
@@ -1749,17 +1750,17 @@ void nsMenuPopupFrame::ChangeByPage(bool
 
       currentMenu = aIsUp ? currentMenu->GetPrevSibling() :
                             currentMenu->GetNextSibling();
     }
   }
 
   // Select the new menuitem.
   if (newMenu) {
-    ChangeMenuItem(newMenu, false);
+    ChangeMenuItem(newMenu, false, true);
   }
 }
 
 NS_IMETHODIMP nsMenuPopupFrame::SetCurrentMenuItem(nsMenuFrame* aMenuItem)
 {
   if (mCurrentMenu == aMenuItem)
     return NS_OK;
 
@@ -1780,17 +1781,18 @@ NS_IMETHODIMP nsMenuPopupFrame::SetCurre
 void
 nsMenuPopupFrame::CurrentMenuIsBeingDestroyed()
 {
   mCurrentMenu = nullptr;
 }
 
 NS_IMETHODIMP
 nsMenuPopupFrame::ChangeMenuItem(nsMenuFrame* aMenuItem,
-                                 bool aSelectFirstItem)
+                                 bool aSelectFirstItem,
+                                 bool aFromKey)
 {
   if (mCurrentMenu == aMenuItem)
     return NS_OK;
 
   // When a context menu is open, the current menu is locked, and no change
   // to the menu is allowed.
   nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
   if (!mIsContextMenu && pm && pm->HasContextMenu(this))
@@ -1807,16 +1809,36 @@ nsMenuPopupFrame::ChangeMenuItem(nsMenuF
       }
     }
   }
 
   // Set the new child.
   if (aMenuItem) {
     EnsureMenuItemIsVisible(aMenuItem);
     aMenuItem->SelectMenu(true);
+
+    // On Windows, a menulist should update its value whenever navigation was
+    // done by the keyboard.
+#ifdef XP_WIN
+    if (aFromKey && IsOpen()) {
+      nsIFrame* parentMenu = GetParent();
+      if (parentMenu) {
+        nsCOMPtr<nsIDOMXULMenuListElement> menulist = do_QueryInterface(parentMenu->GetContent());
+        if (menulist) {
+          // Fire a command event as the new item, but we don't want to close
+          // the menu, blink it, or update any other state of the menuitem. The
+          // command event will cause the item to be selected.
+          nsContentUtils::DispatchXULCommand(aMenuItem->GetContent(),
+                                             nsContentUtils::IsCallerChrome(),
+                                             nullptr, PresContext()->PresShell(),
+                                             false, false, false, false);
+        }
+      }
+    }
+#endif
   }
 
   mCurrentMenu = aMenuItem;
 
   return NS_OK;
 }
 
 nsMenuFrame*
--- a/layout/xul/nsMenuPopupFrame.h
+++ b/layout/xul/nsMenuPopupFrame.h
@@ -166,17 +166,19 @@ public:
   NS_DECL_FRAMEARENA_HELPERS
 
   explicit nsMenuPopupFrame(nsStyleContext* aContext);
 
   // nsMenuParent interface
   virtual nsMenuFrame* GetCurrentMenuItem() override;
   NS_IMETHOD SetCurrentMenuItem(nsMenuFrame* aMenuItem) override;
   virtual void CurrentMenuIsBeingDestroyed() override;
-  NS_IMETHOD ChangeMenuItem(nsMenuFrame* aMenuItem, bool aSelectFirstItem) override;
+  NS_IMETHOD ChangeMenuItem(nsMenuFrame* aMenuItem,
+                            bool aSelectFirstItem,
+                            bool aFromKey) override;
 
   // as popups are opened asynchronously, the popup pending state is used to
   // prevent multiple requests from attempting to open the same popup twice
   nsPopupState PopupState() { return mPopupState; }
   void SetPopupState(nsPopupState aPopupState) { mPopupState = aPopupState; }
 
   NS_IMETHOD SetActive(bool aActiveFlag) override { return NS_OK; } // We don't care.
   virtual bool IsActive() override { return false; }
--- a/layout/xul/nsXULPopupManager.cpp
+++ b/layout/xul/nsXULPopupManager.cpp
@@ -1983,17 +1983,17 @@ nsXULPopupManager::HandleShortcutNavigat
   nsMenuChainItem* item = GetTopVisibleMenu();
   if (!aFrame && item)
     aFrame = item->Frame();
 
   if (aFrame) {
     bool action;
     nsMenuFrame* result = aFrame->FindMenuWithShortcut(aKeyEvent, action);
     if (result) {
-      aFrame->ChangeMenuItem(result, false);
+      aFrame->ChangeMenuItem(result, false, true);
       if (action) {
         WidgetGUIEvent* evt = aKeyEvent->GetInternalNSEvent()->AsGUIEvent();
         nsMenuFrame* menuToOpen = result->Enter(evt);
         if (menuToOpen) {
           nsCOMPtr<nsIContent> content = menuToOpen->GetContent();
           ShowMenu(content, true, false);
         }
       }
@@ -2064,17 +2064,17 @@ nsXULPopupManager::HandleKeyboardNavigat
   // no popup handled the key, so check the active menubar, if any
   if (mActiveMenuBar) {
     nsMenuFrame* currentMenu = mActiveMenuBar->GetCurrentMenuItem();
   
     if (NS_DIRECTION_IS_INLINE(theDirection)) {
       nsMenuFrame* nextItem = (theDirection == eNavigationDirection_End) ?
                               GetNextMenuItem(mActiveMenuBar, currentMenu, false) : 
                               GetPreviousMenuItem(mActiveMenuBar, currentMenu, false);
-      mActiveMenuBar->ChangeMenuItem(nextItem, true);
+      mActiveMenuBar->ChangeMenuItem(nextItem, true, true);
       return true;
     }
     else if (NS_DIRECTION_IS_BLOCK(theDirection)) {
       // Open the menu and select its first item.
       if (currentMenu) {
         nsCOMPtr<nsIContent> content = currentMenu->GetContent();
         ShowMenu(content, true, false);
       }
@@ -2100,17 +2100,17 @@ nsXULPopupManager::HandleKeyboardNavigat
 
   // This method only gets called if we're open.
   if (!currentMenu && NS_DIRECTION_IS_INLINE(aDir)) {
     // 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);
       if (nextItem) {
-        aFrame->ChangeMenuItem(nextItem, false);
+        aFrame->ChangeMenuItem(nextItem, false, true);
         return true;
       }
     }
     return false;
   }
 
   bool isContainer = false;
   bool isOpen = false;
@@ -2142,17 +2142,17 @@ nsXULPopupManager::HandleKeyboardNavigat
     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 (nextItem) {
-      aFrame->ChangeMenuItem(nextItem, false);
+      aFrame->ChangeMenuItem(nextItem, false, true);
       return true;
     }
   }
   else if (currentMenu && isContainer && isOpen) {
     if (aDir == eNavigationDirection_Start) {
       // close a submenu when Left is pressed
       nsMenuPopupFrame* popupFrame = currentMenu->GetPopup();
       if (popupFrame)
--- a/toolkit/content/tests/chrome/test_menulist.xul
+++ b/toolkit/content/tests/chrome/test_menulist.xul
@@ -188,37 +188,39 @@ function test_nsIDOMXULMenuListElement(e
   is(element.itemCount, 0, testprefix + " removeAllItems");
 }
 
 function test_menulist_open(element, scroller)
 {
   element.appendItem("Scroll Item 1", "scrollitem1");
   element.appendItem("Scroll Item 2", "scrollitem2");
   element.focus();
+  element.selectedIndex = 0;
 
 /*
   // bug 530504, mousewheel while menulist is open should not scroll menulist
   // items or parent
   var scrolled = false;
   var mouseScrolled = function (event) { scrolled = true; }
   window.addEventListener("DOMMouseScroll", mouseScrolled, false);
   synthesizeWheel(element, 2, 2, { deltaY: 10,
                                    deltaMode: WheelEvent.DOM_DELTA_LINE });
   is(scrolled, true, "mousescroll " + element.id);
   is(scroller.scrollTop, 0, "scroll position on mousescroll " + element.id);
   window.removeEventListener("DOMMouseScroll", mouseScrolled, false);
 */
 
-  // bug 543065, hovering the mouse over an item should highlight it and not
-  // scroll the parent
+  // bug 543065, hovering the mouse over an item should highlight it, not
+  // scroll the parent, and not change the selected index.
   var item = element.menupopup.childNodes[1];
 
   synthesizeMouse(element.menupopup.childNodes[1], 2, 2, { type: "mousemove" });
   synthesizeMouse(element.menupopup.childNodes[1], 6, 6, { type: "mousemove" });
   is(element.menuBoxObject.activeChild, item, "activeChild after menu highlight " + element.id);
+  is(element.selectedIndex, 0, "selectedIndex after menu highlight " + element.id);
   is(scroller.scrollTop, 0, "scroll position after menu highlight " + element.id);
 
   element.open = false;
 }
 
 function checkScrollAndFinish()
 {
   is($("scroller").scrollTop, 0, "mousewheel on menulist does not scroll vbox parent");
--- a/toolkit/content/tests/chrome/test_menulist_keynav.xul
+++ b/toolkit/content/tests/chrome/test_menulist_keynav.xul
@@ -14,82 +14,135 @@
     <menuitem id="i1" label="One"/>
     <menuitem id="i2" label="Two"/>
     <menuitem id="i2b" disabled="true" label="Two and a Half"/>
     <menuitem id="i3" label="Three"/>
     <menuitem id="i4" label="Four"/>
   </menupopup>
 </menulist>
 <button id="button2" label="Two"/>
+<menulist id="list2">
+  <menupopup id="popup" onpopupshown="checkCursorNavigation();" onpopuphidden="done()">
+    <menuitem id="b1" label="One"/>
+    <menuitem id="b2" label="Two" selected="true"/>
+    <menuitem id="b3" label="Three"/>
+    <menuitem id="b4" label="Four"/>
+  </menupopup>
+</menulist>
 
 <script class="testbody" type="application/javascript">
 <![CDATA[
 
 SimpleTest.waitForExplicitFinish();
 
 var gShowPopup = false;
 var gModifiers = 0;
+var gOpenPhase = false;
+
+var list = $("list");
+let expectCommandEvent;
 
 var iswin = (navigator.platform.indexOf("Win") == 0);
 
 function runTests()
 {
   var list = $("list");
   list.focus();
+
   // 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) {
+    expectCommandEvent = true;
     keyCheck(list, "VK_DOWN", 2, "cursor down");
     keyCheck(list, "VK_DOWN", iswin ? "2b" : 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");
   }
 
   // check that attempting to open the menulist does not change the selection
   synthesizeKey("VK_DOWN", { altKey: navigator.platform.indexOf("Mac") == -1 });
   is(list.selectedItem, $("i1"), "open menulist down selectedItem");
   synthesizeKey("VK_UP", { altKey: navigator.platform.indexOf("Mac") == -1 });
   is(list.selectedItem, $("i1"), "open menulist up selectedItem");
 
+  list.selectedItem = $("i1");
+
+  pressLetter();
+}
+
+function pressLetter()
+{
+  // A command event should be fired only if the menulist is closed, or on Windows,
+  // where items are selected immediately.
+  expectCommandEvent = !gOpenPhase || iswin;
+
   synthesizeKey("G", { });
   is(list.selectedItem, $("i1"), "letter pressed not found selectedItem");
 
   keyCheck(list, "T", 2, "letter pressed");
-  SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // prevent to timeout
-  keyCheck(list, "T", 2, "letter pressed");
-  SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");
-  setTimeout(pressedAgain, 1200); 
+
+  if (!gOpenPhase) {
+    SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // prevent to timeout
+    keyCheck(list, "T", 2, "same letter pressed");
+    SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");
+  }
+
+  setTimeout(pressedAgain, 1200);
 }
 
 function pressedAgain()
 {
   var list = $("list");
+
   keyCheck(list, "T", iswin ? "2b" : 3, "letter pressed again");
   SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // prevent to timeout
   keyCheck(list, "W", 2, "second letter pressed");
   SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");
   setTimeout(differentPressed, 1200); 
 }
 
 function differentPressed()
 {
   var list = $("list");
+
   keyCheck(list, "O", 1, "different letter pressed");
 
+  if (gOpenPhase) {
+    list.open = false;
+    tabAndScroll();
+  }
+  else {
+     // Run the letter tests again with the popup open
+    info("list open phase");
+
+    list.selectedItem = $("i1");
+
+    gShowPopup = true;
+    list.open = true;
+    gOpenPhase = true;
+
+    list.addEventListener("popupshown", function popupShownListener() {
+      list.removeEventListener("popupshown", popupShownListener, false);
+      pressLetter();
+    }, false);
+  }
+}
+
+function tabAndScroll()
+{
   if (navigator.platform.indexOf("Mac") == -1) {
     $("button1").focus();
     synthesizeKeyExpectEvent("VK_TAB", { }, list, "focus", "focus to menulist");
     synthesizeKeyExpectEvent("VK_TAB", { }, $("button2"), "focus", "focus to button");
     is(document.activeElement, $("button2"), "tab from menulist focused button");
   }
 
   // now make sure that using a key scrolls the menu correctly
-  gShowPopup = true;
 
   for (let i = 0; i < 65; i++) {
     list.appendItem("Item" + i, "item" + i);
   }
   list.open = true;
   is(list.getBoundingClientRect().width, list.firstChild.getBoundingClientRect().width,
      "menu and popup width match");
   var minScrollbarWidth = window.matchMedia("(-moz-overlay-scrollbars)").matches ? 0 : 3;
@@ -118,18 +171,18 @@ function differentPressed()
   list.open = false;
 
   checkEnter();
 }
 
 function keyCheck(list, key, index, testname)
 {
   var item = $("i" + index);
-  synthesizeKeyExpectEvent(key, { }, item, "command", testname);
-  is(list.selectedItem, item, testname + " selectedItem");
+  synthesizeKeyExpectEvent(key, { }, item, expectCommandEvent ? "command" : "!command", testname);
+  is(list.selectedItem, expectCommandEvent ? item : $("i1"), testname + " selectedItem");
 }
 
 function checkModifiers(event)
 {
   var expectedModifiers = (gModifiers == 1);
   is(event.shiftKey, expectedModifiers, "shift key pressed");
   is(event.ctrlKey, expectedModifiers, "ctrl key pressed");
   is(event.altKey, expectedModifiers, "alt key pressed");
@@ -149,30 +202,59 @@ function checkEnter()
 function checkEnterWithModifiers()
 {
   is(gModifiers, 1, "modifiers checked when not set");
 
   var list = $("list");
   ok(!list.open, "list closed on enter press");
   list.removeEventListener("popuphidden", checkEnterWithModifiers, false);
 
-  list.addEventListener("popuphidden", done, false);
+  list.addEventListener("popuphidden", verifyPopupOnClose, false);
   list.open = true;
 
   synthesizeKey("VK_RETURN", { shiftKey: true, ctrlKey: true, altKey: true, metaKey: true });
 }
 
-function done()
+function verifyPopupOnClose()
 {
   is(gModifiers, 2, "modifiers checked when set");
 
   var list = $("list");
   ok(!list.open, "list closed on enter press with modifiers");
   list.removeEventListener("popuphidden", done, false);
 
+  list = $("list2");
+  list.focus();
+  list.open = true;
+}
+
+function checkCursorNavigation()
+{
+  var list = $("list2");
+
+  var commandEventsCount = 0;
+  list.addEventListener("command", event => {
+    is(event.target, list.selectedItem, "command event fired on selected item");
+    commandEventsCount++;
+  }, false);
+
+  is(list.selectedIndex, 1, "selectedIndex before cursor down");
+  synthesizeKey("VK_DOWN", { });
+  is(list.selectedIndex, iswin ? 2 : 1, "selectedIndex after cursor down");
+  is(commandEventsCount, iswin ? 1 : 0, "selectedIndex after cursor down command event");
+  is(list.menupopup.state, "open", "cursor down popup state");
+  synthesizeKey("VK_PAGE_DOWN", { });
+  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");
+  list.open = false;
+}
+
+function done()
+{
   SimpleTest.finish();
 }
 
 SimpleTest.waitForFocus(runTests);
 
 ]]>
 </script>
 
--- a/toolkit/modules/SelectContentHelper.jsm
+++ b/toolkit/modules/SelectContentHelper.jsm
@@ -14,16 +14,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/BrowserUtils.jsm");
 
 this.EXPORTED_SYMBOLS = [
   "SelectContentHelper"
 ];
 
 this.SelectContentHelper = function (aElement, aGlobal) {
   this.element = aElement;
+  this.initialSelection = aElement[aElement.selectedIndex] || null;
   this.global = aGlobal;
   this.init();
   this.showDropDown();
 }
 
 this.SelectContentHelper.prototype = {
   init: function() {
     this.global.addMessageListener("Forms:SelectDropDownItem", this);
@@ -55,26 +56,26 @@ this.SelectContentHelper.prototype = {
 
   _buildOptionList: function() {
     return buildOptionListForChildren(this.element);
   },
 
   receiveMessage: function(message) {
     switch (message.name) {
       case "Forms:SelectDropDownItem":
-        if (this.element.selectedIndex != message.data.value) {
-          this.element.selectedIndex = message.data.value;
+        this.element.selectedIndex = message.data.value;
+        break;
 
+      case "Forms:DismissedDropDown":
+        if (this.initialSelection != this.element.item[this.element.selectedIndex]) {
           let event = this.element.ownerDocument.createEvent("Events");
           event.initEvent("change", true, true);
           this.element.dispatchEvent(event);
         }
 
-        //intentional fall-through
-      case "Forms:DismissedDropDown":
         this.uninit();
         break;
     }
   },
 
   handleEvent: function(event) {
     switch (event.type) {
       case "pagehide":
--- a/toolkit/modules/SelectParentHelper.jsm
+++ b/toolkit/modules/SelectParentHelper.jsm
@@ -26,34 +26,31 @@ this.SelectParentHelper = {
     menulist.selectedItem.scrollIntoView();
   },
 
   hide: function(menulist) {
     menulist.menupopup.hidePopup();
   },
 
   handleEvent: function(event) {
-    let popup = event.currentTarget;
-    let menulist = popup.parentNode;
-
     switch (event.type) {
       case "command":
         if (event.target.hasAttribute("value")) {
           currentBrowser.messageManager.sendAsyncMessage("Forms:SelectDropDownItem", {
             value: event.target.value
           });
         }
-        popup.hidePopup();
         break;
 
       case "popuphidden":
         currentBrowser.messageManager.sendAsyncMessage("Forms:DismissedDropDown", {});
         currentBrowser = null;
+        let popup = event.target;
         this._unregisterListeners(popup);
-        menulist.hidden = true;
+        popup.parentNode.hidden = true;
         break;
     }
   },
 
   _registerListeners: function(popup) {
     popup.addEventListener("command", this);
     popup.addEventListener("popuphidden", this);
   },