Bug 1530594, generate frames before setting the active child in a menulist, so that menulists with sizetopopup='none' will still allow keyboard navigation when the menulist has not yet been opened, r=tnikkel
☠☠ backed out by 812a2e26c05e ☠ ☠
authorNeil Deakin <neil@mozilla.com>
Fri, 01 Mar 2019 22:07:58 -0500
changeset 520010 2e63fa41257b8950f5726fd88936ba3f5e2d8441
parent 520009 50a280e9bdff9b24ea36720450896e9137fc677c
child 520011 571c5aa0458018f62d1f0832fc0e92fa3f23fda9
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1530594
milestone67.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 1530594, generate frames before setting the active child in a menulist, so that menulists with sizetopopup='none' will still allow keyboard navigation when the menulist has not yet been opened, r=tnikkel
layout/xul/nsMenuFrame.cpp
layout/xul/nsMenuPopupFrame.cpp
layout/xul/nsMenuPopupFrame.h
layout/xul/nsXULPopupManager.cpp
toolkit/content/tests/chrome/test_menulist_keynav.xul
--- a/layout/xul/nsMenuFrame.cpp
+++ b/layout/xul/nsMenuFrame.cpp
@@ -1263,16 +1263,23 @@ nsMenuFrame::GetActiveChild(dom::Element
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsMenuFrame::SetActiveChild(dom::Element* aChild) {
   nsMenuPopupFrame* popupFrame = GetPopup();
   if (!popupFrame) return NS_ERROR_FAILURE;
 
+  // Force the child frames within the popup to be generated.
+  AutoWeakFrame weakFrame(popupFrame);
+  popupFrame->GenerateFrames();
+  if (!weakFrame.IsAlive()) {
+    return NS_OK;
+  }
+
   if (!aChild) {
     // Remove the current selection
     popupFrame->ChangeMenuItem(nullptr, false, false);
     return NS_OK;
   }
 
   nsMenuFrame* menu = do_QueryFrame(aChild->GetPrimaryFrame());
   if (menu) popupFrame->ChangeMenuItem(menu, false, false);
--- a/layout/xul/nsMenuPopupFrame.cpp
+++ b/layout/xul/nsMenuPopupFrame.cpp
@@ -1607,16 +1607,28 @@ nsresult nsMenuPopupFrame::SetPopupPosit
     if (aNotify) {
       nsXULPopupPositionedEvent::DispatchIfNeeded(mContent, false, false);
     }
   }
 
   return NS_OK;
 }
 
+void nsMenuPopupFrame::GenerateFrames()
+{
+  const bool generateFrames = IsLeaf();
+  MOZ_ASSERT_IF(generateFrames, !mGeneratedChildren);
+  mGeneratedChildren = true;
+  if (generateFrames) {
+    MOZ_ASSERT(PrincipalChildList().IsEmpty());
+    nsCOMPtr<nsIPresShell> presShell = PresContext()->PresShell();
+    presShell->FrameConstructor()->GenerateChildFrames(this);
+  }
+}
+
 /* virtual */
 nsMenuFrame* nsMenuPopupFrame::GetCurrentMenuItem() { return mCurrentMenu; }
 
 LayoutDeviceIntRect nsMenuPopupFrame::GetConstraintRect(
     const LayoutDeviceIntRect& aAnchorRect,
     const LayoutDeviceIntRect& aRootScreenRect, nsPopupLevel aPopupLevel) {
   LayoutDeviceIntRect screenRectPixels;
 
--- a/layout/xul/nsMenuPopupFrame.h
+++ b/layout/xul/nsMenuPopupFrame.h
@@ -261,18 +261,18 @@ class nsMenuPopupFrame final : public ns
   // (or the frame for mAnchorContent if aAnchorFrame is null), anchored at a
   // rectangle, or at a specific point if a screen position is set. The popup
   // will be adjusted so that it is on screen. If aIsMove is true, then the
   // popup is being moved, and should not be flipped. If aNotify is true, then
   // a popuppositioned event is sent.
   nsresult SetPopupPosition(nsIFrame* aAnchorFrame, bool aIsMove,
                             bool aSizedToPopup, bool aNotify);
 
-  bool HasGeneratedChildren() { return mGeneratedChildren; }
-  void SetGeneratedChildren() { mGeneratedChildren = true; }
+  // Force the children to be generated if they have not already been generated.
+  void GenerateFrames();
 
   // called when the Enter key is pressed while the popup is open. This will
   // just pass the call down to the current menu, if any. If a current menu
   // should be opened as a result, this method should return the frame for
   // that menu, or null if no menu should be opened. Also, calling Enter will
   // reset the current incremental search string, calculated in
   // FindMenuWithShortcut.
   nsMenuFrame* Enter(mozilla::WidgetGUIEvent* aEvent);
--- a/layout/xul/nsXULPopupManager.cpp
+++ b/layout/xul/nsXULPopupManager.cpp
@@ -1274,34 +1274,25 @@ void nsXULPopupManager::FirePopupShowing
                                               bool aIsContextMenu,
                                               bool aSelectFirstItem,
                                               Event* aTriggerEvent) {
   nsCOMPtr<nsIContent> popup = aPopup;  // keep a strong reference to the popup
 
   nsMenuPopupFrame* popupFrame = do_QueryFrame(aPopup->GetPrimaryFrame());
   if (!popupFrame) return;
 
-  nsPresContext* presContext = popupFrame->PresContext();
-  nsCOMPtr<nsIPresShell> presShell = presContext->PresShell();
-  nsPopupType popupType = popupFrame->PopupType();
-
-  // generate the child frames if they have not already been generated
-  const bool generateFrames = popupFrame->IsLeaf();
-  MOZ_ASSERT_IF(generateFrames, !popupFrame->HasGeneratedChildren());
-  popupFrame->SetGeneratedChildren();
-  if (generateFrames) {
-    MOZ_ASSERT(popupFrame->PrincipalChildList().IsEmpty());
-    presShell->FrameConstructor()->GenerateChildFrames(popupFrame);
-  }
+  popupFrame->GenerateFrames();
 
   // get the frame again
   nsIFrame* frame = aPopup->GetPrimaryFrame();
   if (!frame) return;
 
-  presShell->FrameNeedsReflow(frame, nsIPresShell::eTreeChange,
+  nsPresContext* presContext = popupFrame->PresContext();
+  nsCOMPtr<nsIPresShell> presShell = presContext->PresShell();
+  presShell->FrameNeedsReflow(popupFrame, nsIPresShell::eTreeChange,
                               NS_FRAME_HAS_DIRTY_CHILDREN);
 
   // cache the popup so that document.popupNode can retrieve the trigger node
   // during the popupshowing event. It will be cleared below after the event
   // has fired.
   mOpeningPopup = aPopup;
 
   nsEventStatus status = nsEventStatus_eIgnore;
@@ -1334,16 +1325,17 @@ void nsXULPopupManager::FirePopupShowing
   mOpeningPopup = nullptr;
 
   mCachedModifiers = 0;
 
   // if a panel, blur whatever has focus so that the panel can take the focus.
   // This is done after the popupshowing event in case that event is cancelled.
   // Using noautofocus="true" will disable this behaviour, which is needed for
   // the autocomplete widget as it manages focus itself.
+  nsPopupType popupType = popupFrame->PopupType();
   if (popupType == ePopupTypePanel &&
       !popup->AsElement()->AttrValueIs(kNameSpaceID_None,
                                        nsGkAtoms::noautofocus, nsGkAtoms::_true,
                                        eCaseMatters)) {
     nsFocusManager* fm = nsFocusManager::GetFocusManager();
     if (fm) {
       Document* doc = popup->GetUncomposedDoc();
 
--- a/toolkit/content/tests/chrome/test_menulist_keynav.xul
+++ b/toolkit/content/tests/chrome/test_menulist_keynav.xul
@@ -22,16 +22,24 @@
 <menulist id="list2">
   <menupopup id="popup" onpopupshown="checkCursorNavigation();">
     <menuitem id="b1" label="One"/>
     <menuitem id="b2" label="Two" selected="true"/>
     <menuitem id="b3" label="Three"/>
     <menuitem id="b4" label="Four"/>
   </menupopup>
 </menulist>
+<menulist id="list3" sizetopopup="none">
+  <menupopup>
+    <menuitem id="s1" label="One"/>
+    <menuitem id="s2" label="Two"/>
+    <menuitem id="s3" label="Three"/>
+    <menuitem id="s4" label="Four"/>
+  </menupopup>
+</menulist>
 
 <script class="testbody" type="application/javascript">
 <![CDATA[
 
 SimpleTest.waitForExplicitFinish();
 
 var gShowPopup = false;
 var gModifiers = 0;
@@ -270,16 +278,26 @@ function checkCursorNavigation()
 
   synthesizeKey("KEY_ArrowUp", {altKey: true});
   is(list.open, ismac, "alt+up closes popup");
 
   if (ismac) {
     list.open = false;
   }
 
+  // Finally, test a menulist with sizetopopup="none" to ensure keyboard navigation
+  // still works when the popup has not been opened.
+  if (!ismac) {
+    let unsizedMenulist = document.getElementById("list3");
+    unsizedMenulist.focus();
+    synthesizeKey("KEY_ArrowDown");
+    is(unsizedMenulist.selectedIndex, 1, "correct menulist index on keydown");
+    is(unsizedMenulist.label, "Two", "correct menulist label on keydown");
+  }
+
   SimpleTest.finish();
 }
 
 SimpleTest.waitForFocus(runTests);
 
 ]]>
 </script>