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, updated to get popup type earlier to avoid possible crash, r=tnikkel
authorNeil Deakin <neil@mozilla.com>
Tue, 05 Mar 2019 04:32:40 -0500
changeset 462392 2d7395290c187ee80403a08267ea8c26eca2df8c
parent 462342 78601cacfe69dc8659c3fe7cd3eb94366aa3d680
child 462393 97b4af9b538e1e93da307dbcf4adeb757bc4cd87
push id35651
push usershindli@mozilla.com
push dateTue, 05 Mar 2019 21:41:37 +0000
treeherdermozilla-central@996a48b30652 [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, updated to get popup type earlier to avoid possible crash, 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,36 +1274,29 @@ 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;
 
+  popupFrame->GenerateFrames();
+
+  // get the frame again
+  popupFrame = do_QueryFrame(aPopup->GetPrimaryFrame());
+  if (!popupFrame) return;
+
   nsPresContext* presContext = popupFrame->PresContext();
   nsCOMPtr<nsIPresShell> presShell = presContext->PresShell();
+  presShell->FrameNeedsReflow(popupFrame, nsIPresShell::eTreeChange,
+                              NS_FRAME_HAS_DIRTY_CHILDREN);
+
   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);
-  }
-
-  // get the frame again
-  nsIFrame* frame = aPopup->GetPrimaryFrame();
-  if (!frame) return;
-
-  presShell->FrameNeedsReflow(frame, 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;
   WidgetMouseEvent event(true, eXULPopupShowing, nullptr,
                          WidgetMouseEvent::eReal);
--- 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>