Bug 394600, address bz's additional popup reworking comments, add tests for removing popups, r+sr=bz,a=beltzner

content/xul/content/src/nsXULPopupListener.cpp

@@ -216,8 +216,9 @@ nsXULPopupListener::PreLaunchPopup(nsIDOMEvent* aMouseEvent)
 
   // Turn the document into a XUL document so we can use SetPopupNode.
   nsCOMPtr<nsIDOMXULDocument> xulDocument = do_QueryInterface(content->GetDocument());
-  if (!xulDocument)
+  if (!xulDocument) {
     return NS_ERROR_FAILURE;
+  }
 
   // Store clicked-on node in xul document for context menus and menu popups.
   xulDocument->SetPopupNode(targetNode);
@@ -354,8 +355,8 @@ GetImmediateChild(nsIContent* aContent, nsIAtom *aTag, nsIContent** aResult)
 // content.
 //
 // aTargetContent is the target of the mouse event aEvent that triggered the
-// popup. mElement is the element that the popup menu is attached to. The
+// popup. mElement is the element that the popup menu is attached to.
-// former may be equal to mElement or it may be a descendant.
+// aTargetContent may be equal to mElement or it may be a descendant.
 //
 // This looks for an attribute on |mElement| of the appropriate popup type 
 // (popup, context) and uses that attribute's value as an ID for
@@ -451,10 +452,6 @@ nsXULPopupListener::LaunchPopup(nsIDOMEvent* aEvent, nsIContent* aTargetContent)
   if (!pm)
     return NS_OK;
 
-  // XXXndeakin this is temporary. It is needed to grab the mouse location details
-  //            used by the spellchecking popup. See bug 383930.
-  pm->SetMouseLocation(aEvent, popup);
-
   // For left-clicks, if the popup has an position attribute, or both the
   // popupanchor and popupalign attributes are used, anchor the popup to the
   // element, otherwise just open it at the screen position where the mouse
@@ -465,7 +462,7 @@ nsXULPopupListener::LaunchPopup(nsIDOMEvent* aEvent, nsIContent* aTargetContent)
        (mPopupContent->HasAttr(kNameSpaceID_None, nsGkAtoms::popupanchor) &&
         mPopupContent->HasAttr(kNameSpaceID_None, nsGkAtoms::popupalign)))) {
     pm->ShowPopup(mPopupContent, content, EmptyString(), 0, 0,
-                  PR_FALSE, PR_TRUE, PR_FALSE);
+                  PR_FALSE, PR_TRUE, PR_FALSE, aEvent);
   }
   else {
     PRInt32 xPos = 0, yPos = 0;
@@ -480,7 +477,7 @@ nsXULPopupListener::LaunchPopup(nsIDOMEvent* aEvent, nsIContent* aTargetContent)
       yPos += 2;
     }
 
-    pm->ShowPopupAtScreen(mPopupContent, xPos, yPos, mIsContext);
+    pm->ShowPopupAtScreen(mPopupContent, xPos, yPos, mIsContext, aEvent);
   }
 
   return NS_OK;

layout/generic/nsIFrame.h

@@ -2192,6 +2192,8 @@ private:
 
 class nsWeakFrame {
 public:
+  nsWeakFrame() : mPrev(nsnull), mFrame(nsnull) { }
+
   nsWeakFrame(nsIFrame* aFrame) : mPrev(nsnull), mFrame(nsnull)
   {
     Init(aFrame);

layout/xul/base/public/nsIPopupBoxObject.idl

@@ -121,7 +121,9 @@ interface nsIPopupBoxObject : nsISupports
    * if position is empty.
    *
    * For an anchored popup, the x and y arguments may be used to offset the 
-   * popup from its anchored position by some number, measured in CSS pixels.
+   * popup from its anchored position by some distance, measured in CSS pixels.
+   * x increases to the right and y increases down. Negative values may also
+   * be used to move to the left and upwards respectively.
    *
    * Unanchored popups may be created by supplying null as the anchor node.
    * An unanchored popup appears at the position specified by x and y,

layout/xul/base/public/nsXULPopupManager.h

@@ -349,16 +349,18 @@ public:
   // menu. If aIsPopup is false, the navigation is on a menubar, so navigate
   // between menus on the menubar. This is used for left/right cursor navigation.
   //
-  // Items that not valid, such as non-menu or menuitem elements are skipped,
+  // Items that are not valid, such as non-menu or non-menuitem elements are
-  // and the next or previous item after that is checked.
+  // skipped, and the next or previous item after that is checked.
   //
-  // If aStart is null, the first valid item is retrieved for GetNextMenuItem
+  // If aStart is null, the first valid item is retrieved by GetNextMenuItem
-  // or the last valid item for GetPreviousMenuItem is used.
+  // and the last valid item is retrieved by GetPreviousMenuItem.
+  //
+  // Both methods will loop around the beginning or end if needed.
   //
   // aParent - the parent menubar or menupopup
   // aStart - the menu/menuitem to start navigation from. GetPreviousMenuItem
   //          returns the item before it, while GetNextMenuItem returns the
-  //          next item.
+  //          item after it.
   // aIsPopup - true for menupopups, false for menubars
   static nsMenuFrame* GetPreviousMenuItem(nsIFrame* aParent,
                                           nsMenuFrame* aStart,
@@ -385,13 +387,10 @@ public:
 
   // retrieve the node and offset of the last mouse event used to open a
   // context menu. This information is determined from the rangeParent and
-  // the rangeOffset of the event supplied from the last call to SetMouseLocation.
+  // the rangeOffset of the event supplied to ShowPopup or ShowPopupAtScreen.
   // This is used by the implementation of nsIDOMXULDocument::GetPopupRangeParent
   // and nsIDOMXULDocument::GetPopupRangeOffset.
   void GetMouseLocation(nsIDOMNode** aNode, PRInt32* aOffset);
-  // set the mouse event that was used to activate the next popup, specified by
-  // aPopup, to be opened.
-  void SetMouseLocation(nsIDOMEvent* aEvent, nsIContent* aPopup);
 
   /**
    * Open a <menu> given its content node. If aSelectFirstItem is
@@ -406,6 +405,11 @@ public:
    * true, then the first item in the menu is selected. The arguments are
    * similar to those for nsIPopupBoxObject::OpenPopup.
    *
+   * aTriggerEvent should be the event that triggered the event. This is used
+   * to determine the coordinates for the popupshowing event. This may be null
+   * if the popup was not triggered by an event, or the coordinates are not
+   * important. Note that this may be reworked in bug 383930.
+   *
    * This fires the popupshowing event synchronously.
    */
   void ShowPopup(nsIContent* aPopup,
@@ -414,7 +418,8 @@ public:
                  PRInt32 aXPos, PRInt32 aYPos,
                  PRBool aIsContextMenu,
                  PRBool aAttributesOverride,
-                 PRBool aSelectFirstItem);
+                 PRBool aSelectFirstItem,
+                 nsIDOMEvent* aTriggerEvent);
 
   /**
    * Open a popup at a specific screen position specified by aXPos and aYPos,
@@ -424,7 +429,8 @@ public:
    */
   void ShowPopupAtScreen(nsIContent* aPopup,
                          PRInt32 aXPos, PRInt32 aYPos,
-                         PRBool aIsContextMenu);
+                         PRBool aIsContextMenu,
+                         nsIDOMEvent* aTriggerEvent);
 
   /**
    * This method is provided only for compatibility with an older popup API.
@@ -565,7 +571,7 @@ public:
 
   /**
    * Handle keyboard navigation within a menu popup specified by aFrame.
-   * Returns true if the key was handled and that other default handling
+   * Returns true if the key was handled and other default handling
    * should not occur.
    */
   PRBool HandleKeyboardNavigationInPopup(nsMenuPopupFrame* aFrame,
@@ -593,6 +599,17 @@ protected:
   // return the topmost menu, skipping over invisible popups
   nsMenuChainItem* GetTopVisibleMenu();
 
+  // Hide all of the visible popups from the given list. aDeselectMenu
+  // indicates whether to deselect the menu of popups when hiding; this
+  // flag is passed as the first argument to HidePopup. This function
+  // can cause style changes and frame destruction.
+  void HidePopupsInList(const nsTArray<nsMenuPopupFrame *> &aFrames,
+                        PRBool aDeselectMenu);
+
+  // set the event that was used to trigger the popup, or null to
+  // clear the event details.
+  void SetTriggerEvent(nsIDOMEvent* aEvent, nsIContent* aPopup);
+
   // callbacks for ShowPopup and HidePopup as events may be done asynchronously
   void ShowPopupCallback(nsIContent* aPopup,
                          nsMenuPopupFrame* aPopupFrame,
@@ -665,7 +682,7 @@ private:
    * supplied, then it is expected to have a frame equal to aFrame.
    * If aItem is non-null, then the navigation may be redirected to
    * an open submenu if one exists. Returns true if the key was
-   * handled and that other default handling should not occur.
+   * handled and other default handling should not occur.
    */
   PRBool HandleKeyboardNavigationInPopup(nsMenuChainItem* aItem,
                                          nsMenuPopupFrame* aFrame,
@@ -699,7 +716,7 @@ protected:
   // widget that is currently listening to rollup events
   nsCOMPtr<nsIWidget> mWidget;
 
-  // range parent and offset set in SetMouseLocation
+  // range parent and offset set in SetTriggerEvent
   nsCOMPtr<nsIDOMNode> mRangeParent;
   PRInt32 mRangeOffset;
   nsPoint mCachedMousePoint;

layout/xul/base/src/nsIMenuParent.h

@@ -62,7 +62,7 @@ public:
   // 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 menupoups, the menu is not opened and
+  // 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, PRBool aSelectFirstItem) = 0;
 

layout/xul/base/src/nsMenuBarFrame.cpp

@@ -331,14 +331,12 @@ nsMenuBarFrame::SetCurrentMenuItem(nsMenuFrame* aMenuItem)
   if (mCurrentMenu == aMenuItem)
     return NS_OK;
 
-  nsWeakFrame weakFrame(this);
   if (mCurrentMenu)
     mCurrentMenu->SelectMenu(PR_FALSE);
 
   if (aMenuItem)
     aMenuItem->SelectMenu(PR_TRUE);
 
-  NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
   mCurrentMenu = aMenuItem;
   mRecentlyClosedMenu = nsnull;
 
@@ -432,9 +430,7 @@ nsMenuBarFrame::ChangeMenuItem(nsMenuFrame* aMenuItem,
   // Set the new child.
   if (aMenuItem) {
     nsCOMPtr<nsIContent> content = aMenuItem->GetContent();
-    nsWeakFrame weakNewMenu(aMenuItem);
     aMenuItem->SelectMenu(PR_TRUE);
-    NS_ENSURE_TRUE(weakNewMenu.IsAlive(), NS_OK);
     mCurrentMenu = aMenuItem;
     if (wasOpen && !aMenuItem->IsDisabled())
       aNewMenu = content;

layout/xul/base/src/nsMenuFrame.cpp

@@ -333,9 +333,8 @@ nsMenuFrame::GetFirstChild(nsIAtom* aListName) const
   return nsBoxFrame::GetFirstChild(aListName);
 }
 
-NS_IMETHODIMP
+nsIFrame*
-nsMenuFrame::SetInitialChildList(nsIAtom*        aListName,
+nsMenuFrame::SetPopupFrame(nsIFrame* aChildList)
-                                 nsIFrame*       aChildList)
 {
   // Check for a menupopup and move it to mPopupFrame
   nsFrameList frames(aChildList);
@@ -351,7 +350,16 @@ nsMenuFrame::SetInitialChildList(nsIAtom*        aListName,
     frame = frame->GetNextSibling();
   }
 
-  // Didn't find it.
+  return aChildList;
+}
+
+NS_IMETHODIMP
+nsMenuFrame::SetInitialChildList(nsIAtom*        aListName,
+                                 nsIFrame*       aChildList)
+{
+  NS_ASSERTION(!mPopupFrame, "already have a popup frame set");
+  if (!aListName || aListName == nsGkAtoms::popupList)
+    aChildList = SetPopupFrame(aChildList);
   return nsBoxFrame::SetInitialChildList(aListName, aChildList);
 }
 
@@ -1184,27 +1192,31 @@ nsMenuFrame::InsertFrames(nsIAtom*        aListName,
                           nsIFrame*       aPrevFrame,
                           nsIFrame*       aFrameList)
 {
-  nsresult          rv;
+  if (!mPopupFrame && (!aListName || aListName == nsGkAtoms::popupList)) {
-
+    aFrameList = SetPopupFrame(aFrameList);
-  if (!mPopupFrame && aFrameList->GetType() == nsGkAtoms::menuPopupFrame) {
+    if (mPopupFrame) {
-    mPopupFrame = static_cast<nsMenuPopupFrame *>(aFrameList);
 
 #ifdef DEBUG_LAYOUT
-    nsBoxLayoutState state(PresContext());
+      nsBoxLayoutState state(PresContext());
-    SetDebug(state, aFrameList, mState & NS_STATE_CURRENTLY_IN_DEBUG);
+      SetDebug(state, aFrameList, mState & NS_STATE_CURRENTLY_IN_DEBUG);
 #endif
-    PresContext()->PresShell()->
+
-      FrameNeedsReflow(this, nsIPresShell::eTreeChange,
+      PresContext()->PresShell()->
-                       NS_FRAME_HAS_DIRTY_CHILDREN);
+        FrameNeedsReflow(this, nsIPresShell::eTreeChange,
-    rv = NS_OK;
+                         NS_FRAME_HAS_DIRTY_CHILDREN);
-  } else {
+
-    if (NS_UNLIKELY(aPrevFrame == mPopupFrame)) {
+      return NS_OK;
-      aPrevFrame = nsnull;
     }
-    rv = nsBoxFrame::InsertFrames(aListName, aPrevFrame, aFrameList);  
   }
 
-  return rv;
+  if (!aFrameList)
+    return NS_OK;
+
+  if (NS_UNLIKELY(aPrevFrame == mPopupFrame)) {
+    aPrevFrame = nsnull;
+  }
+
+  return nsBoxFrame::InsertFrames(aListName, aPrevFrame, aFrameList);
 }
 
 NS_IMETHODIMP
@@ -1214,24 +1226,26 @@ nsMenuFrame::AppendFrames(nsIAtom*        aListName,
   if (!aFrameList)
     return NS_OK;
 
-  nsresult          rv;
+  if (!mPopupFrame && (!aListName || aListName == nsGkAtoms::popupList)) {
-
+    aFrameList = SetPopupFrame(aFrameList);
-  if (!mPopupFrame && aFrameList->GetType() == nsGkAtoms::menuPopupFrame) {
+    if (mPopupFrame) {
-    mPopupFrame = static_cast<nsMenuPopupFrame *>(aFrameList);
 
 #ifdef DEBUG_LAYOUT
-    nsBoxLayoutState state(PresContext());
+      nsBoxLayoutState state(PresContext());
-    SetDebug(state, aFrameList, mState & NS_STATE_CURRENTLY_IN_DEBUG);
+      SetDebug(state, aFrameList, mState & NS_STATE_CURRENTLY_IN_DEBUG);
 #endif
-    PresContext()->PresShell()->
+      PresContext()->PresShell()->
-      FrameNeedsReflow(this, nsIPresShell::eTreeChange,
+        FrameNeedsReflow(this, nsIPresShell::eTreeChange,
-                       NS_FRAME_HAS_DIRTY_CHILDREN);
+                         NS_FRAME_HAS_DIRTY_CHILDREN);
-    rv = NS_OK;
+
-  } else {
+      return NS_OK;
-    rv = nsBoxFrame::AppendFrames(aListName, aFrameList); 
+    }
   }
 
-  return rv;
+  if (!aFrameList)
+    return NS_OK;
+
+  return nsBoxFrame::AppendFrames(aListName, aFrameList); 
 }
 
 PRBool

layout/xul/base/src/nsMenuFrame.h

@@ -103,12 +103,6 @@ private:
   nsMenuFrame* mFrame;
 };
 
-/**
- * @note *** Methods marked with '@see comment above ***' may cause the frame to be
- *           deleted during the method call. Be careful whenever using those
- *           methods.
- */
-
 class nsMenuFrame : public nsBoxFrame, 
                     public nsIMenuFrame,
                     public nsIScrollableViewProvider
@@ -140,16 +134,17 @@ public:
   NS_IMETHOD SetInitialChildList(nsIAtom*        aListName,
                                  nsIFrame*       aChildList);
   virtual nsIAtom* GetAdditionalChildListName(PRInt32 aIndex) const;
-  virtual void Destroy(); // @see comment above ***
+  virtual void Destroy();
 
   // Overridden to prevent events from going to children of the menu.
   NS_IMETHOD BuildDisplayListForChildren(nsDisplayListBuilder*   aBuilder,
                                          const nsRect&           aDirtyRect,
                                          const nsDisplayListSet& aLists);
                                          
+  // this method can destroy the frame
   NS_IMETHOD HandleEvent(nsPresContext* aPresContext, 
                          nsGUIEvent*     aEvent,
-                         nsEventStatus*  aEventStatus); // @see comment above ***
+                         nsEventStatus*  aEventStatus);
 
   NS_IMETHOD  AppendFrames(nsIAtom*        aListName,
                            nsIFrame*       aFrameList);
@@ -163,12 +158,10 @@ public:
 
   virtual nsIAtom* GetType() const { return nsGkAtoms::menuFrame; }
 
-  NS_IMETHOD SelectMenu(PRBool aActivateFlag); // @see comment above ***
+  NS_IMETHOD SelectMenu(PRBool aActivateFlag);
 
   /**
-   * NOTE: OpenMenu will open the menu synchronously. Don't call this if a frame
+   * NOTE: OpenMenu will open the menu asynchronously.
-   *       is manipulated afterwards without checking to make sure it is still alive.
-   *       All current calls to OpenMenu do not adjust the frame.
    */
   void OpenMenu(PRBool aSelectFirstItem);
   // CloseMenu closes the menu asynchronously
@@ -177,11 +170,12 @@ public:
   PRBool IsChecked() { return mChecked; }
 
   NS_IMETHOD GetActiveChild(nsIDOMElement** aResult);
-  NS_IMETHOD SetActiveChild(nsIDOMElement* aChild); // @see comment above ***
+  NS_IMETHOD SetActiveChild(nsIDOMElement* aChild);
 
   // called when the Enter key is pressed while the menuitem is the current
   // one in its parent popup. This will carry out the command attached to
-  // the menuitem.
+  // the menuitem. If the menu should be opened, this frame will be returned,
+  // otherwise null will be returned.
   nsMenuFrame* Enter();
 
   NS_IMETHOD SetParent(const nsIFrame* aParent);
@@ -209,11 +203,12 @@ public:
 
   // indiciate that the menu's popup has just been opened, so that the menu
   // can update its open state. This method modifies the open attribute on
-  // the menu, so the frames could be gone after this call
+  // the menu, so the frames could be gone after this call.
   void PopupOpened();
   // indiciate that the menu's popup has just been closed, so that the menu
   // can update its open state. The menu should be unhighlighted if
-  // aDeselectedMenu is true.
+  // aDeselectedMenu is true. This method modifies the open attribute on
+  // the menu, so the frames could be gone after this call.
   void PopupClosed(PRBool aDeselectMenu);
 
   // returns true if this is a menu on another menu popup. A menu is a submenu
@@ -234,23 +229,32 @@ protected:
   friend class nsMenuTimerMediator;
   friend class nsASyncMenuInitialization;
 
+  // initialize mPopupFrame to the first popup frame within aChildList. Returns
+  // aChildList with the popup frame removed.
+  nsIFrame* SetPopupFrame(nsIFrame* aChildList);
+
   // set mMenuParent to the nearest enclosing menu bar or menupopup frame of
   // aParent (or aParent itself). This is called when initializing the frame,
   // so aParent should be the expected parent of this frame.
   void InitMenuParent(nsIFrame* aParent);
 
-  void UpdateMenuType(nsPresContext* aPresContext); // @see comment above ***
+  // Update the menu's type (normal, checkbox, radio).
-  void UpdateMenuSpecialState(nsPresContext* aPresContext); // @see comment above ***
+  // This method can destroy the frame.
+  void UpdateMenuType(nsPresContext* aPresContext);
+  // Update the checked state of the menu, and for radios, clear any other
+  // checked items. This method can destroy the frame.
+  void UpdateMenuSpecialState(nsPresContext* aPresContext);
 
   // Examines the key node and builds the accelerator.
   void BuildAcceleratorText();
 
-  // Called to execute our command handler.
+  // Called to execute our command handler. This method can destroy the frame.
-  void Execute(nsGUIEvent *aEvent); // @see comment above ***
+  void Execute(nsGUIEvent *aEvent);
 
+  // This method can destroy the frame
   NS_IMETHOD AttributeChanged(PRInt32 aNameSpaceID,
                               nsIAtom* aAttribute,
-                              PRInt32 aModType); // @see comment above ***
+                              PRInt32 aModType);
   virtual ~nsMenuFrame();
 
   PRBool SizeToPopup(nsBoxLayoutState& aState, nsSize& aSize);

layout/xul/base/src/nsMenuPopupFrame.h

@@ -203,9 +203,11 @@ public:
   void SetGeneratedChildren() { mGeneratedChildren = PR_TRUE; }
 
   // 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. Also, calling Enter
+  // just pass the call down to the current menu, if any. If a current menu
-  // will reset the current incremental search string, calculated in
+  // should be opened as a result, this method should return the frame for
-  // FindMenuWithShortcut
+  // 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();
 
   nsPopupType PopupType() const { return mPopupType; }

layout/xul/base/src/nsPopupBoxObject.cpp

@@ -130,7 +130,7 @@ nsPopupBoxObject::OpenPopup(nsIDOMElement* aAnchorElement,
   if (pm) {
     nsCOMPtr<nsIContent> anchorContent(do_QueryInterface(aAnchorElement));
     pm->ShowPopup(mContent, anchorContent, aPosition, aXPos, aYPos,
-                  aIsContextMenu, aAttributesOverride, PR_FALSE);
+                  aIsContextMenu, aAttributesOverride, PR_FALSE, nsnull);
   }
 
   return NS_OK;
@@ -141,7 +141,7 @@ nsPopupBoxObject::OpenPopupAtScreen(PRInt32 aXPos, PRInt32 aYPos, PRBool aIsCont
 {
   nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
   if (pm)
-    pm->ShowPopupAtScreen(mContent, aXPos, aYPos, aIsContextMenu);
+    pm->ShowPopupAtScreen(mContent, aXPos, aYPos, aIsContextMenu, nsnull);
   return NS_OK;
 }
 

layout/xul/base/src/nsXULPopupManager.cpp

@@ -265,12 +265,11 @@ nsXULPopupManager::GetMouseLocation(nsIDOMNode** aNode, PRInt32* aOffset)
 }
 
 void
-nsXULPopupManager::SetMouseLocation(nsIDOMEvent* aEvent, nsIContent* aPopup)
+nsXULPopupManager::SetTriggerEvent(nsIDOMEvent* aEvent, nsIContent* aPopup)
 {
   mCachedMousePoint = nsPoint(0, 0);
 
   nsCOMPtr<nsIDOMNSUIEvent> uiEvent = do_QueryInterface(aEvent);
-  NS_ASSERTION(!aEvent || uiEvent, "Expected an nsIDOMNSUIEvent");
   if (uiEvent) {
     uiEvent->GetRangeParent(getter_AddRefs(mRangeParent));
     uiEvent->GetRangeOffset(&mRangeOffset);
@@ -349,7 +348,7 @@ nsXULPopupManager::ShowMenu(nsIContent *aMenu,
   popupFrame->InitializePopup(aMenu, position, 0, 0, PR_TRUE);
 
   if (aAsynchronous) {
-    SetMouseLocation(nsnull, nsnull);
+    SetTriggerEvent(nsnull, nsnull);
     nsCOMPtr<nsIRunnable> event =
       new nsXULPopupShowingEvent(popupFrame->GetContent(), aMenu,
                                  parentIsContextMenu, aSelectFirstItem);
@@ -370,12 +369,15 @@ nsXULPopupManager::ShowPopup(nsIContent* aPopup,
                              PRInt32 aXPos, PRInt32 aYPos,
                              PRBool aIsContextMenu,
                              PRBool aAttributesOverride,
-                             PRBool aSelectFirstItem)
+                             PRBool aSelectFirstItem,
+                             nsIDOMEvent* aTriggerEvent)
 {
   nsMenuPopupFrame* popupFrame = GetPopupFrameForContent(aPopup);
   if (!popupFrame || !MayShowPopup(popupFrame))
     return;
 
+  SetTriggerEvent(aTriggerEvent, aPopup);
+
   popupFrame->InitializePopup(aAnchorContent, aPosition, aXPos, aYPos,
                               aAttributesOverride);
 
@@ -386,12 +388,15 @@ nsXULPopupManager::ShowPopup(nsIContent* aPopup,
 void
 nsXULPopupManager::ShowPopupAtScreen(nsIContent* aPopup,
                                      PRInt32 aXPos, PRInt32 aYPos,
-                                     PRBool aIsContextMenu)
+                                     PRBool aIsContextMenu,
+                                     nsIDOMEvent* aTriggerEvent)
 {
   nsMenuPopupFrame* popupFrame = GetPopupFrameForContent(aPopup);
   if (!popupFrame || !MayShowPopup(popupFrame))
     return;
 
+  SetTriggerEvent(aTriggerEvent, aPopup);
+
   popupFrame->InitializePopupAtScreen(aXPos, aYPos);
 
   FirePopupShowingEvent(aPopup, nsnull, popupFrame->PresContext(),
@@ -410,6 +415,8 @@ nsXULPopupManager::ShowPopupWithAnchorAlign(nsIContent* aPopup,
   if (!popupFrame || !MayShowPopup(popupFrame))
     return;
 
+  SetTriggerEvent(nsnull, nsnull);
+
   popupFrame->InitializePopupWithAnchorAlign(aAnchorContent, aAnchor,
                                              aAlign, aXPos, aYPos);
 
@@ -494,9 +501,6 @@ nsXULPopupManager::HidePopup(nsIContent* aPopup,
                              PRBool aDeselectMenu,
                              PRBool aAsynchronous)
 {
-  // remove the popup from the open lists. Just to be safe, check both the
-  // menu and panels lists.
-
   // if the popup is on the panels list, remove it but don't close any other panels
   nsMenuPopupFrame* popupFrame = nsnull;
   PRBool foundPanel = PR_FALSE;
@@ -536,7 +540,7 @@ nsXULPopupManager::HidePopup(nsIContent* aPopup,
     // the next popup in the chain. These two methods will be called in
     // sequence recursively to close up all the necessary popups. In
     // asynchronous mode, a similar process occurs except that the
-    // FirePopupHidingEvent method is called asynchrounsly. In either case,
+    // FirePopupHidingEvent method is called asynchronously. In either case,
     // nextPopup is set to the content node of the next popup to close, and
     // lastPopup is set to the last popup in the chain to close, which will be
     // aPopup, or null to close up all menus.
@@ -708,33 +712,68 @@ nsXULPopupManager::HidePopupAfterDelay(nsMenuPopupFrame* aPopup)
 }
 
 void
-nsXULPopupManager::HidePopupsInDocument(nsIDocument* aDocument)
+nsXULPopupManager::HidePopupsInList(const nsTArray<nsMenuPopupFrame *> &aFrames,
+                                    PRBool aDeselectMenu)
 {
-  nsMenuChainItem* item = GetTopVisibleMenu();
+  // Create a weak frame list. This is done in a separate array with the
-  while (item) {
+  // right capacity predetermined, otherwise the array would get resized and
-    nsMenuChainItem* parent = item->GetParent();
+  // move the weak frame pointers around.
-    if (item->Content()->GetOwnerDoc() == aDocument) {
+  nsTArray<nsWeakFrame> weakPopups(aFrames.Length());
-      item->Frame()->HidePopup(PR_TRUE, ePopupInvisible);
+  PRUint32 f;
-      item->Detach(&mCurrentMenu);
+  for (f = 0; f < aFrames.Length(); f++) {
-      delete item;
+    nsWeakFrame* wframe = weakPopups.AppendElement();
-    }
+    if (wframe)
-    item = parent;
+      *wframe = aFrames[f];
   }
 
-  item = mPanels;
+  for (f = 0; f < weakPopups.Length(); f++) {
-  while (item) {
+    // check to ensure that the frame is still alive before hiding it.
-    nsMenuChainItem* parent = item->GetParent();
+    if (weakPopups[f].IsAlive()) {
-    if (item->Content()->GetOwnerDoc() == aDocument) {
+      nsMenuPopupFrame* frame =
-      item->Frame()->HidePopup(PR_TRUE, ePopupInvisible);
+        static_cast<nsMenuPopupFrame *>(weakPopups[f].GetFrame());
-      item->Detach(&mPanels);
+      frame->HidePopup(PR_TRUE, ePopupInvisible);
-      delete item;
     }
-    item = parent;
   }
 
   SetCaptureState(nsnull);
 }
 
+void
+nsXULPopupManager::HidePopupsInDocument(nsIDocument* aDocument)
+{
+  nsTArray<nsMenuPopupFrame *> popupsToHide;
+
+  // iterate to get the set of popup frames to hide
+  nsMenuChainItem* item = mCurrentMenu;
+  while (item) {
+    nsMenuChainItem* parent = item->GetParent();
+    if (item->Frame()->PopupState() != ePopupInvisible &&
+        aDocument && item->Content()->GetOwnerDoc() == aDocument) {
+      nsMenuPopupFrame* frame = item->Frame();
+      item->Detach(&mCurrentMenu);
+      delete item;
+      popupsToHide.AppendElement(frame);
+    }
+    item = parent;
+  }
+
+  // now look for panels to hide
+  item = mPanels;
+  while (item) {
+    nsMenuChainItem* parent = item->GetParent();
+    if (item->Frame()->PopupState() != ePopupInvisible &&
+        aDocument && item->Content()->GetOwnerDoc() == aDocument) {
+      nsMenuPopupFrame* frame = item->Frame();
+      item->Detach(&mPanels);
+      delete item;
+      popupsToHide.AppendElement(frame);
+    }
+    item = parent;
+  }
+
+  HidePopupsInList(popupsToHide, PR_TRUE);
+}
+
 void
 nsXULPopupManager::ExecuteMenu(nsIContent* aMenu, nsEvent* aEvent)
 {
@@ -760,6 +799,7 @@ nsXULPopupManager::ExecuteMenu(nsIContent* aMenu, nsEvent* aEvent)
   // opens a modal dialog. The views associated with the popups needed to be
   // hidden and the accesibility events fired before the command executes, but
   // the popuphiding/popuphidden events are fired afterwards.
+  nsTArray<nsMenuPopupFrame *> popupsToHide;
   nsMenuChainItem* item = GetTopVisibleMenu();
   if (cmm != CloseMenuMode_None) {
     while (item) {
@@ -767,14 +807,16 @@ nsXULPopupManager::ExecuteMenu(nsIContent* aMenu, nsEvent* aEvent)
       if (!item->IsMenu())
         break;
       nsMenuChainItem* next = item->GetParent();
-      item->Frame()->HidePopup(cmm == CloseMenuMode_Auto, ePopupInvisible);
+      popupsToHide.AppendElement(item->Frame());
       if (cmm == CloseMenuMode_Single) // only close one level of menu
         break;
       item = next;
     }
-  }
 
-  SetCaptureState(nsnull);
+    // Now hide the popups. If the closemenu mode is auto, deselect the menu,
+    // otherwise only one popup is closing, so keep the parent menu selected.
+    HidePopupsInList(popupsToHide, cmm == CloseMenuMode_Auto);
+  }
 
   // Create a trusted event if the triggering event was trusted, or if
   // we're called from chrome code (since at least one of our caller
@@ -1102,52 +1144,48 @@ nsXULPopupManager::PopupDestroyed(nsMenuPopupFrame* aPopup)
     item = item->GetParent();
   }
 
-  nsCOMPtr<nsIContent> oldMenu;
+  nsTArray<nsMenuPopupFrame *> popupsToHide;
-  if (mCurrentMenu)
-    oldMenu = mCurrentMenu->Content();
 
-  nsMenuChainItem* menuToDestroy = nsnull;
   item = mCurrentMenu;
   while (item) {
-    if (item->Frame() == aPopup) {
+    nsMenuPopupFrame* frame = item->Frame();
+    if (frame == aPopup) {
+      if (frame->PopupState() != ePopupInvisible) {
+        // Iterate through any child menus and hide them as well, since the
+        // parent is going away. We won't remove them from the list yet, just
+        // hide them, as they will be removed from the list when this function
+        // gets called for that child frame.
+        nsMenuChainItem* child = item->GetChild();
+        while (child) {
+          // if the popup is a child frame of the menu that was destroyed, add
+          // it to the list of popups to hide. Don't bother with the events
+          // since the frames are going away. If the child menu is not a child
+          // frame, for example, a context menu, use HidePopup instead, but call
+          // it asynchronously since we are in the middle of frame destruction.
+          nsMenuPopupFrame* childframe = child->Frame();
+          if (nsLayoutUtils::IsProperAncestorFrame(frame, childframe)) {
+            popupsToHide.AppendElement(childframe);
+          }
+          else {
+            // HidePopup will take care of hiding any of its children, so
+            // break out afterwards
+            HidePopup(child->Content(), PR_FALSE, PR_FALSE, PR_TRUE);
+            break;
+          }
+
+          child = child->GetChild();
+        }
+      }
+
       item->Detach(&mCurrentMenu);
-      menuToDestroy = item;
+      delete item;
       break;
     }
+
     item = item->GetParent();
   }
 
-  if (menuToDestroy) {
+  HidePopupsInList(popupsToHide, PR_FALSE);
-    // menuToDestroy will be set to the item to delete. Iterate through any
-    // child menus and destroy them as well, since the parent is going away
-    nsIFrame* menuToDestroyFrame = menuToDestroy->Frame();
-    item = menuToDestroy->GetChild();
-    while (item) {
-      nsMenuChainItem* next = item->GetChild();
-
-      // if the popup is a child frame of the menu that was destroyed, unhook
-      // it from the list of open menus and inform the popup frame that it
-      // should be hidden. Don't bother with the events since the frames are
-      // going away. If the child menu is not a child frame, for example, a
-      // context menu, use HidePopup instead
-      if (nsLayoutUtils::IsProperAncestorFrame(menuToDestroyFrame, item->Frame())) {
-        item->Detach(&mCurrentMenu);
-        item->Frame()->HidePopup(PR_FALSE, ePopupInvisible);
-      }
-      else {
-        HidePopup(item->Content(), PR_FALSE, PR_FALSE, PR_TRUE);
-        break;
-      }
-
-      delete item;
-      item = next;
-    }
-
-    delete menuToDestroy;
-  }
-
-  if (oldMenu)
-    SetCaptureState(oldMenu);
 }
 
 PRBool
@@ -1625,13 +1663,13 @@ nsXULPopupManager::IsValidMenuItem(nsPresContext* aPresContext,
 {
   PRInt32 ns = aContent->GetNameSpaceID();
   nsIAtom *tag = aContent->Tag();
-  if (ns == kNameSpaceID_XUL &&
+  if (ns == kNameSpaceID_XUL) {
-       tag != nsGkAtoms::menu &&
+    if (tag != nsGkAtoms::menu && tag != nsGkAtoms::menuitem)
-       tag != nsGkAtoms::menuitem)
+      return PR_FALSE;
-    return PR_FALSE;
+  }
-
+  else if (ns != kNameSpaceID_XHTML || !aOnPopup || tag != nsGkAtoms::option) {
-  if (ns == kNameSpaceID_XHTML && (!aOnPopup || tag != nsGkAtoms::option))
     return PR_FALSE;
+  }
 
   PRBool skipNavigatingDisabledMenuItem = PR_TRUE;
   if (aOnPopup) {

layout/xul/base/src/nsXULTooltipListener.cpp

@@ -512,7 +512,7 @@ nsXULTooltipListener::LaunchTooltip()
 
   nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
   if (pm) {
-    pm->ShowPopupAtScreen(mCurrentTooltip, mMouseClientX, mMouseClientY, PR_FALSE);
+    pm->ShowPopupAtScreen(mCurrentTooltip, mMouseClientX, mMouseClientY, PR_FALSE, nsnull);
     // Clear the current tooltip if the popup was not opened successfully.
     if (!pm->IsPopupOpen(mCurrentTooltip))
       mCurrentTooltip = nsnull;

toolkit/content/tests/widgets/Makefile.in

@@ -87,6 +87,9 @@ _TEST_FILES = 	test_bug360220.xul \
 		test_popup_tree.xul \
 		test_popup_keys.xul \
 		test_popuphidden.xul \
+		test_popupremoving.xul \
+		test_popupremoving_frame.xul \
+		frame_popupremoving_frame.xul \
 		$(NULL)
 
 ifeq (,$(filter mac cocoa,$(MOZ_WIDGET_TOOLKIT)))

toolkit/content/tests/widgets/frame_popupremoving_frame.xul

@@ -0,0 +1,75 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+
+<window title="Popup Removing Frame Tests"
+        onload="setTimeout(init, 0)"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<hbox>
+
+<menu id="separatemenu1" label="Menu">
+    <menupopup id="separatepopup1" onpopupshown="document.getElementById('separatemenu2').open = true">
+        <menuitem label="L1 One"/>
+        <menuitem label="L1 Two"/>
+        <menuitem label="L1 Three"/>
+    </menupopup>
+</menu>
+
+<menu id="separatemenu2" label="Menu">
+    <menupopup id="separatepopup2" onpopupshown="document.getElementById('separatemenu3').open = true">
+        <menuitem label="L2 One"/>
+        <menuitem label="L2 Two"/>
+        <menuitem label="L2 Three"/>
+    </menupopup>
+</menu>
+
+<menu id="separatemenu3" label="Menu" onpopupshown="document.getElementById('separatemenu4').open = true">
+    <menupopup id="separatepopup3">
+        <menuitem label="L3 One"/>
+        <menuitem label="L3 Two"/>
+        <menuitem label="L3 Three"/>
+    </menupopup>
+</menu>
+
+<menu id="separatemenu4" label="Menu" onpopupshown="document.getElementById('nestedmenu1').open = true">
+    <menupopup id="separatepopup3">
+        <menuitem label="L4 One"/>
+        <menuitem label="L4 Two"/>
+        <menuitem label="L4 Three"/>
+    </menupopup>
+</menu>
+
+</hbox>
+
+<menu id="nestedmenu1" label="Menu">
+    <menupopup id="nestedpopup1" onpopupshown="if (event.target == this) this.firstChild.open = true">
+      <menu id="nestedmenu2" label="Menu">
+        <menupopup id="nestedpopup2" onpopupshown="if (event.target == this) this.firstChild.open = true">
+          <menu id="nestedmenu3" label="Menu">
+            <menupopup id="nestedpopup3" onpopupshown="if (event.target == this) this.firstChild.open = true">
+              <menu id="nestedmenu4" label="Menu" onpopupshown="parent.popupsOpened()">
+                <menupopup id="nestedpopup4">
+                  <menuitem label="Nested One"/>
+                  <menuitem label="Nested Two"/>
+                  <menuitem label="Nested Three"/>
+                </menupopup>
+              </menu>
+            </menupopup>
+          </menu>
+        </menupopup>
+      </menu>
+    </menupopup>
+</menu>
+
+<script class="testbody" type="application/javascript">
+<![CDATA[
+
+function init()
+{
+  document.getElementById("separatemenu1").open = true;
+}
+
+]]>
+</script>
+
+</window>

toolkit/content/tests/widgets/test_popupremoving.xul

@@ -0,0 +1,166 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet href="/tests/SimpleTest/test.css" type="text/css"?>
+
+<window title="Popup Removing Tests"
+        onload="setTimeout(nextTest, 0)"
+        onDOMAttrModified="modified(event)"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!--
+  This test checks that popup elements can be removed in various ways without
+  crashing. It tests two situations, one with menus that are 'separate', and
+  one with menus that are 'nested'. In each case, there are four levels of menu.
+
+  The nextTest function starts the process by opening the first menu. A set of
+  popupshown event listeners are used to open the next menu until all four are
+  showing. This last one calls removePopup to remove the menu node from the
+  tree. This should hide the popups as they are no longer in a document.
+
+  A mutation listener is triggered when the fourth menu closes by having its
+  open attribute cleared. This listener hides the third popup which causes
+  its frame to be removed. Naturally, we want to ensure that this doesn't
+  crash when the third menu is removed.
+  -->
+
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>      
+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>      
+
+<hbox>
+
+<menu id="nestedmenu1" label="1">
+    <menupopup id="nestedpopup1" onpopupshown="if (event.target == this) this.firstChild.open = true">
+      <menu id="nestedmenu2" label="2">
+        <menupopup id="nestedpopup2" onpopupshown="if (event.target == this) this.firstChild.open = true">
+          <menu id="nestedmenu3" label="3">
+            <menupopup id="nestedpopup3" onpopupshown="if (event.target == this) this.firstChild.open = true">
+              <menu id="nestedmenu4" label="4" onpopupshown="removePopups()">
+                <menupopup id="nestedpopup4">
+                  <menuitem label="Nested 1"/>
+                  <menuitem label="Nested 2"/>
+                  <menuitem label="Nested 3"/>
+                </menupopup>
+              </menu>
+            </menupopup>
+          </menu>
+        </menupopup>
+      </menu>
+    </menupopup>
+</menu>
+
+<menu id="separatemenu1" label="1">
+    <menupopup id="separatepopup1" onpopupshown="$('separatemenu2').open = true">
+        <menuitem label="L1 One"/>
+        <menuitem label="L1 Two"/>
+        <menuitem label="L1 Three"/>
+    </menupopup>
+</menu>
+
+<menu id="separatemenu2" label="2">
+    <menupopup id="separatepopup2" onpopupshown="$('separatemenu3').open = true"
+                                   onpopuphidden="popup2Hidden()">
+        <menuitem label="L2 One"/>
+        <menuitem label="L2 Two"/>
+        <menuitem label="L2 Three"/>
+    </menupopup>
+</menu>
+
+<menu id="separatemenu3" label="3" onpopupshown="$('separatemenu4').open = true">
+    <menupopup id="separatepopup3">
+        <menuitem label="L3 One"/>
+        <menuitem label="L3 Two"/>
+        <menuitem label="L3 Three"/>
+    </menupopup>
+</menu>
+
+<menu id="separatemenu4" label="4" onpopupshown="removePopups()"
+                                   onpopuphidden="$('separatemenu2').open = false">
+    <menupopup id="separatepopup3">
+        <menuitem label="L4 One"/>
+        <menuitem label="L4 Two"/>
+        <menuitem label="L4 Three"/>
+    </menupopup>
+</menu>
+
+</hbox>
+
+<script class="testbody" type="application/javascript">
+<![CDATA[
+
+SimpleTest.waitForExplicitFinish();
+
+var gKey = "";
+gTriggerMutation = null;
+gChangeMutation = null;
+
+function nextTest()
+{
+  if (gKey == "") {
+    gKey = "separate";
+  }
+  else if (gKey == "separate") {
+    gKey = "nested";
+  }
+  else {
+    SimpleTest.finish();
+    return;
+  }
+
+  $(gKey + "menu1").open = true;
+}
+
+function modified(event)
+{
+  // use this mutation listener to hide the third popup, destroying its frame.
+  // It gets triggered when the open attribute is cleared on the fourth menu.
+
+  if (event.target == gTriggerMutation &&
+      event.attrName == "open") {
+    gChangeMutation.hidden = true;
+    // force a layout flush
+    document.documentElement.boxObject.width;
+    gTriggerMutation = null;
+    gChangeMutation = null;
+  }
+}
+
+function removePopups()
+{
+  var menu2 = $(gKey + "menu2");
+  var menu3 = $(gKey + "menu3");
+  is(menu2.getAttribute("open"), "true", gKey + " menu 2 open before");
+  is(menu3.getAttribute("open"), "true", gKey + " menu 3 open before");
+
+  gTriggerMutation = menu3;
+  gChangeMutation = $(gKey + "menu4");
+  var menu = $(gKey + "menu1");
+  menu.parentNode.removeChild(menu);
+
+  if (gKey == "nested") {
+    // the 'separate' test checks this during the popup2 hidden event handler
+    is(menu2.hasAttribute("open"), false, gKey + " menu 2 open after");
+    is(menu3.hasAttribute("open"), false, gKey + " menu 3 open after");
+    nextTest();
+  }
+}
+
+function popup2Hidden()
+{
+  is($(gKey + "menu2").hasAttribute("open"), false, gKey + " menu 2 open after");
+  nextTest();
+}
+
+]]>
+</script>
+
+<body xmlns="http://www.w3.org/1999/xhtml">
+<p id="display">
+</p>
+<div id="content" style="display: none">
+</div>
+<pre id="test">
+</pre>
+</body>
+
+</window>

toolkit/content/tests/widgets/test_popupremoving_frame.xul

@@ -0,0 +1,83 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet href="/tests/SimpleTest/test.css" type="text/css"?>
+
+<window title="Popup Unload Test"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!--
+  This test checks that popup elements are removed when the document is changed.
+  -->
+
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>      
+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>      
+
+<iframe id="frame" width="300" height="150" src="frame_popupremoving_frame.xul"/>
+
+<script class="testbody" type="application/javascript">
+<![CDATA[
+
+SimpleTest.waitForExplicitFinish();
+
+var gMenus = [];
+
+function popupsOpened()
+{
+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
+
+  var framedoc = $("frame").contentDocument;
+  framedoc.addEventListener("DOMAttrModified", modified, false);
+
+  // this is the order in which the menus should be hidden (reverse of the
+  // order they were opened in). The frame for the second menu is removed
+  // during the mutation listener, so never gets an mutation event.
+  gMenus.push(framedoc.getElementById("nestedmenu4"));
+  gMenus.push(framedoc.getElementById("nestedmenu3"));
+  gMenus.push(framedoc.getElementById("nestedmenu1"));
+  gMenus.push(framedoc.getElementById("separatemenu4"));
+  gMenus.push(framedoc.getElementById("separatemenu3"));
+  gMenus.push(framedoc.getElementById("separatemenu1"));
+
+  framedoc.location = "about:blank";
+}
+
+function modified(event)
+{
+  if (event.attrName != "open")
+    return;
+
+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
+
+  var framedoc = $("frame").contentDocument;
+
+  var tohide = null;
+  if (event.target.id == "separatemenu3")
+    tohide = framedoc.getElementById("separatemenu2");
+  else if (event.target.id == "nestedmenu3")
+    tohide = framedoc.getElementById("nestedmenu2");
+
+  if (tohide) {
+    tohide.hidden = true;
+    // force a layout flush
+    $("frame").contentDocument.documentElement.boxObject.width;
+  }
+
+  is(event.target, gMenus.shift(), event.target.id + " hidden");
+  if (gMenus.length == 0)
+    SimpleTest.finish();
+}
+
+]]>
+</script>
+
+<body xmlns="http://www.w3.org/1999/xhtml">
+<p id="display">
+</p>
+<div id="content" style="display: none">
+</div>
+<pre id="test">
+</pre>
+</body>
+
+</window>