Bug 281568. MSAA events EVENT_MENUEND/EVENT_MENUPOPUPEND fired too late, when dialog opens from menu action. Screen readers get confused. r=bryner, sr=neil, a=dveditz

accessible/src/base/nsRootAccessible.cpp

@@ -243,10 +243,10 @@ nsresult nsRootAccessible::AddEventListeners()
     rv = target->AddEventListener(NS_LITERAL_STRING("RadioStateChange"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     NS_ASSERTION(NS_SUCCEEDED(rv), "failed to register listener");
 
-    rv = target->AddEventListener(NS_LITERAL_STRING("popupshowing"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
+    rv = target->AddEventListener(NS_LITERAL_STRING("popupshown"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     NS_ASSERTION(NS_SUCCEEDED(rv), "failed to register listener");
 
-    rv = target->AddEventListener(NS_LITERAL_STRING("popuphiding"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
+    rv = target->AddEventListener(NS_LITERAL_STRING("DOMMenuInactive"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     NS_ASSERTION(NS_SUCCEEDED(rv), "failed to register listener");
 
     rv = target->AddEventListener(NS_LITERAL_STRING("DOMMenuItemActive"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
@@ -287,8 +287,8 @@ nsresult nsRootAccessible::RemoveEventListeners()
     target->RemoveEventListener(NS_LITERAL_STRING("OpenStateChange"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     target->RemoveEventListener(NS_LITERAL_STRING("CheckboxStateChange"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     target->RemoveEventListener(NS_LITERAL_STRING("RadioStateChange"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
-    target->RemoveEventListener(NS_LITERAL_STRING("popupshowing"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
-    target->RemoveEventListener(NS_LITERAL_STRING("popuphiding"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
+    target->RemoveEventListener(NS_LITERAL_STRING("popupshown"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
+    target->RemoveEventListener(NS_LITERAL_STRING("DOMMenuInactive"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     target->RemoveEventListener(NS_LITERAL_STRING("DOMMenuItemActive"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     target->RemoveEventListener(NS_LITERAL_STRING("DOMMenuBarActive"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
     target->RemoveEventListener(NS_LITERAL_STRING("DOMMenuBarInactive"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
@@ -640,10 +640,12 @@ NS_IMETHODIMP nsRootAccessible::HandleEvent(nsIDOMEvent* aEvent)
   else {
     // Menu popup events
     PRUint32 menuEvent = 0;
-    if (eventType.LowerCaseEqualsLiteral("popupshowing"))
+    if (eventType.LowerCaseEqualsLiteral("popupshown")) {
       menuEvent = nsIAccessibleEvent::EVENT_MENUPOPUPSTART;
-    else if (eventType.LowerCaseEqualsLiteral("popuphiding"))
+    }
+    else if (eventType.EqualsLiteral("DOMMenuInactive")) {
       menuEvent = nsIAccessibleEvent::EVENT_MENUPOPUPEND;
+    }
     if (menuEvent) {
       PRUint32 role = ROLE_NOTHING;
       accessible->GetRole(&role);
@@ -714,11 +716,11 @@ NS_IMETHODIMP nsRootAccessible::HandleEvent(nsIDOMEvent* aEvent)
     stateData.state = STATE_EXPANDED;
     privAcc->FireToolkitEvent(nsIAccessibleEvent::EVENT_STATE_CHANGE, accessible, &stateData);
   }
-  else if (eventType.LowerCaseEqualsLiteral("popupshowing")) {
+  else if (eventType.LowerCaseEqualsLiteral("popupshown")) {
     FireAccessibleFocusEvent(accessible, targetNode);
   }
-  else if (eventType.LowerCaseEqualsLiteral("popuphiding")) {
-    //FireAccessibleFocusEvent(accessible, targetNode);
+  else if (eventType.EqualsLiteral("DOMMenuInactive")) {
+    //FireAccessibleFocusEvent(accessible, targetNode);  // Not yet used in ATK
   }
 #endif
   return NS_OK;

layout/xul/base/public/nsIMenuFrame.h

@@ -57,7 +57,7 @@ class nsIMenuFrame : public nsISupports {
 public:
   NS_DEFINE_STATIC_IID_ACCESSOR(NS_IMENUFRAME_IID)
 
-  NS_IMETHOD ActivateMenu(PRBool aFlag) = 0;
+  NS_IMETHOD ActivateMenu(PRBool aActivate) = 0;
   NS_IMETHOD SelectMenu(PRBool aFlag) = 0;
   NS_IMETHOD OpenMenu(PRBool aFlag) = 0;
 

layout/xul/base/src/nsBoxFrame.cpp

@@ -2565,18 +2565,19 @@ nsBoxFrame::RegUnregAccessKey(nsPresContext* aPresContext, PRBool aDoReg)
 
 
 void
-nsBoxFrame::FireDOMEvent(nsPresContext *aPresContext, const nsAString& aDOMEventName)
+nsBoxFrame::FireDOMEvent(const nsAString& aDOMEventName)
 {
-  if (mContent) {
+  if (mContent && mPresContext) {
     // Fire a DOM event for the title change.
     nsCOMPtr<nsIDOMEvent> event;
     nsCOMPtr<nsIEventListenerManager> manager;
     mContent->GetListenerManager(getter_AddRefs(manager));
-    if (manager &&
-        NS_SUCCEEDED(manager->CreateEvent(aPresContext, nsnull, NS_LITERAL_STRING("Events"), getter_AddRefs(event)))) {
+    if (manager && NS_SUCCEEDED(manager->CreateEvent(mPresContext, nsnull,
+                                                     NS_LITERAL_STRING("Events"),
+                                                     getter_AddRefs(event)))) {
       event->InitEvent(aDOMEventName, PR_TRUE, PR_TRUE);
       PRBool noDefault;
-      aPresContext->EventStateManager()->DispatchNewEvent(mContent, event,
+      mPresContext->EventStateManager()->DispatchNewEvent(mContent, event,
                                                           &noDefault);
     }
   }

layout/xul/base/src/nsBoxFrame.h

@@ -204,6 +204,8 @@ public:
 
   static nsresult LayoutChildAt(nsBoxLayoutState& aState, nsIBox* aBox, const nsRect& aRect);
 
+  void FireDOMEvent(const nsAString& aDOMEventName);
+
 protected:
 #ifdef DEBUG_LAYOUT
     virtual void GetBoxName(nsAutoString& aName);
@@ -250,7 +252,6 @@ protected:
 protected:
     nsresult RegUnregAccessKey(nsPresContext* aPresContext,
                                PRBool aDoReg);
-    void FireDOMEvent(nsPresContext *aPresContext, const nsAString& aDOMEventName);
     virtual nsresult GetFrameForPointChild(const nsPoint&    aPoint,
                                            nsFramePaintLayer aWhichLayer,    
                                            nsIFrame*         aChild,

layout/xul/base/src/nsMenuBarFrame.cpp

@@ -251,7 +251,7 @@ nsMenuBarFrame::SetActive(PRBool aActiveFlag)
   NS_NAMED_LITERAL_STRING(active, "DOMMenuBarActive");
   NS_NAMED_LITERAL_STRING(inactive, "DOMMenuBarInactive");
   
-  FireDOMEvent(mPresContext, mIsActive ? active : inactive);
+  FireDOMEvent(mIsActive ? active : inactive);
 
   return NS_OK;
 }
@@ -671,6 +671,8 @@ nsMenuBarFrame::HideChain()
     mRecentRollupMenu = mCurrentMenu;
   }
 
+  SetActive(PR_FALSE);
+
   return NS_OK;
 }
 

layout/xul/base/src/nsMenuFrame.cpp

@@ -573,7 +573,7 @@ nsMenuFrame::SelectMenu(PRBool aActivateFlag)
     domEventToFire.AssignLiteral("DOMMenuItemInactive");
   }
 
-  FireDOMEvent(mPresContext, domEventToFire);
+  FireDOMEvent(domEventToFire);
   return NS_OK;
 }
 
@@ -662,6 +662,9 @@ nsMenuFrame::ActivateMenu(PRBool aActivateFlag)
       viewManager->SetViewVisibility(view, nsViewVisibility_kShow);
 
   } else {
+    if (mMenuOpen) {
+      menuPopup->FireDOMEvent(NS_LITERAL_STRING("DOMMenuInactive"));
+    }
     nsIView* view = menuPopup->GetView();
     NS_ASSERTION(view, "View is gone, looks like someone forgot to rollup the popup!");
     if (view) {
@@ -847,8 +850,6 @@ nsMenuFrame::OpenMenuInternal(PRBool aActivateFlag)
     if ( !mCreateHandlerSucceeded || !OnDestroy() )
       return;
 
-    mMenuOpen = PR_FALSE;
-
     // Set the focus back to our view's widget.
     if (nsMenuFrame::sDismissalListener) {
       nsMenuFrame::sDismissalListener->EnableListener(PR_FALSE);
@@ -885,8 +886,15 @@ nsMenuFrame::OpenMenuInternal(PRBool aActivateFlag)
         esm->SetContentState(nsnull, NS_EVENT_STATE_HOVER);
     }
 
-    // activate false will also set the mMenuOpen to false.
     ActivateMenu(PR_FALSE);
+    // XXX hack: ensure that mMenuOpen is set to false, in case where
+    // there is actually no popup. because ActivateMenu() will return 
+    // early without setting it. It could be that mMenuOpen is true
+    // in that case, because OpenMenuInternal(true) gets called if
+    // the attribute open="true", whether there is a popup or not.
+    // We should not allow mMenuOpen unless there is a popup in the first place,
+    // in which case this line would not be necessary.
+    mMenuOpen = PR_FALSE;
 
     OnDestroyed();