397606 - A11y problems from too many DOMMenuitem[In]active events fired as user switches between menubar menus

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Steps to reproduce:

1. Launch Accerciser and turn event monitoring on.
2. Navigate to the page of your choice
3. Press Alt F to open the File menu
4. Use Left and Right arrow to move among the open menus

Expected results:  We'd only get focus events for the menus and menu items that receive focus.

Actual results:  Each time a menu item gives up focus, we get at least one focus event for the document frame chosen in Step 2.

The impact of this is that Orca is extremely "chatty," providing a lot of irrelevant verbiage as one navigates amongst menus.  I think it will be difficult for us to distinguish true focus events from these transitory/"just in passing" events.  Therefore, it would be extremely handy if they could be stopped on the Mozilla side of things. :-)  It would be even better if that could take place in Firefox 3. :-)  Blocking 396346 in the hopes that this is doable.

Thanks!!

Comment 1

[Ginn Chen]

When moving focus from "File" menu to "Edit" menu,
1) "Edit" menu got focus

2) Got "DOMMenuBarInactive" for "File" menu, which caused a focus event for current focused item in HTML.
See: nsRootAccessible.cpp:859

3) Focus events for "Edit" menu and its menuitem were fired.

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> When moving focus from "File" menu to "Edit" menu,
> 1) "Edit" menu got focus
> 
> 2) Got "DOMMenuBarInactive" for "File" menu, which caused a focus event for
> current focused item in HTML.
> See: nsRootAccessible.cpp:859
> 
> 3) Focus events for "Edit" menu and its menuitem were fired.

Ginn, it sounds like the menubar-inactive is fired regardless of whether another menu actually has focus or not. In this case, for example, the menubar is actually not inactive, only the File menu lost focus. At this stage, however, we already know that the Edit menu has focus.

I did a comparison with NotePad on Windows, doing the RightArrow from the New item on the File menu to the Undo item on the Edit menu, and found the following:

SYS_MENUPOPUPEND	 Event=7 HWND=000D065C Name="Datei" Role=Kontextmenü State=normal
OBJ_FOCUS	 Event=8005 HWND=0004068C Name="Datei" Role=Menübefehl State=markiert,gehovert,markierbar,mit Kontextmenü
OBJ_FOCUS	 Event=8005 HWND=0004068C Name="Bearbeiten" Role=Menübefehl State=markiert,gehovert,markierbar,mit Kontextmenü
SYS_MENUPOPUPSTART	 Event=6 HWND=00040692 Name="Bearbeiten" Role=Kontextmenü State=normal
OBJ_FOCUS	 Event=8005 HWND=00040692 Name="Rückgängig	Strg+Z" Role=Menübefehl State=ausgeblendet,markiert,gehovert

In short, when the File (Datei) menu closes, a MENUPOPUPEND event is fired, followed by two focus events for the File and Edit menus, followed by a MENUPOPUPSTART, and a FOCUS event for the Edit menu and the Undo (Rückgängig) item.

Is it by design that, once a POPUPMENU closes, it is assumed that the menu taht opened it also goes away? Or are these events that should not be fired, but are erroneously?

Comment 3

[Ginn Chen]

I think the "DOMMenuBarInactive" and "DOMMenuBarActive" are OK.
But we should not fire focus event to "Document frame", if "DOMMenuBarActive" is following "DOMMenuBarInactive".

Or maybe we should suppress both "DOMMenuBarInactive" and "DOMMenuBarActive" in accessible module for this situation.

Right now, I don't know how to get that information from accessible module.

Comment 4

[Ginn Chen]

It seems I can't use nsIMenuParent inside accessible module.
To around that, I can call GetFirstChild to get a nsIMenuFrame and then use IsOnActiveMenuBar().

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> Or maybe we should suppress both "DOMMenuBarInactive" and "DOMMenuBarActive" in
> accessible module for this situation.

IMO, that would be the best solution, and consistent with the way other apps (at least on Windows) do it. On Linux, I think menus are just focus events like other controls, AFAIK they don't know anything about MENUPOPUPSTART etc. But if this prevents the erroneous focus events from being fired, that will help the Linux side of things as well!

Comment 6

[Ginn Chen]

Attachment: patch

Also fix a compiler warning for nsAccessNode.cpp

Comment 7

[Ginn Chen]

(In reply to comment #5)
> (In reply to comment #3)
> > Or maybe we should suppress both "DOMMenuBarInactive" and "DOMMenuBarActive" in
> > accessible module for this situation.
> 
> IMO, that would be the best solution, and consistent with the way other apps
> (at least on Windows) do it. On Linux, I think menus are just focus events like
> other controls, AFAIK they don't know anything about MENUPOPUPSTART etc. But if
> this prevents the erroneous focus events from being fired, that will help the
> Linux side of things as well!
> 
Sorry, I proposed my patch before I saw your comment.
Well, that would need some more code.

Aaron, do you have a good idea to get it done?

BTW: Linux doesn't have MENU_START, MENU_END events.
So the question is do we want to suppress them on Windows?

Comment 8

[Marco Zehe (:MarcoZ)]

Ginn, your patch does get rid of the erroneous FOCUS event on the document. But I am noticing inconsistent events being fired on Windows, even when performing the exact same steps again and again. Sometimes, the MENUPOPUP closes, then focus events get fired, intermixed with MENUEND, MENUSTART, and MENUPOPUPSTART. Sometimes, the FOCUS event on the first item of the Edit menu gets fired before the MENUPOPUPEND for the File menu even gets fired.
Are these events somehow sent asynchronously?

Comment 9

[Ginn Chen]

Macro, MENUEND, MENUSTART, MENUPOPUPSTART and FOCUS are all async here.
But they're not in a same queue.
That's why the focus event get intermixed with MENU events.

Comment 10

[Aaron Leventhal]

> > Or maybe we should suppress both "DOMMenuBarInactive" and "DOMMenuBarActive" in
> > accessible module for this situation.

I also agree that's the best solution.

In fact I think that's what we used to do. Maybe this is a regression from the menu work that was done recently.

DOMMenuBarInactive was only supposed to be fired when you're really leaving the menu bar.

Comment 11

[Aaron Leventhal]

Comment on attachment 282381 [details] [diff] [review]
patch

Please fix DOMMenuBarInactive/DOMenuBarActive events instead. Let's find out what broke them.

Comment 12

[Aaron Leventhal]

This was caused by bug 279703:
Bug 279703, rework XUL popups to use asynchronous opening, plus many other fixes, attempt 2 with creating widgets later, r=ginn.chen,bz,neil,roc,sr=bz

This code is causing the premature DOMMenuitemActive event:
PRBool
nsMenuBarFrame::MenuClosed()
{
  SetActive(PR_FALSE);
...
}

The stack clearly shows switching menus (nsMenuBarSwitchMenu) but it results in a MenuClosed() call telling the menubar that it is no longer active.

Here's the stack:
 	gklayout.dll!nsMenuBarFrame::SetActive(int aActiveFlag=0x00000000)  Line 135	C++
>	gklayout.dll!nsMenuBarFrame::MenuClosed()  Line 440	C++
 	gklayout.dll!nsMenuFrame::PopupClosed(int aDeselectMenu=0x00000000)  Line 575 + 0x29 bytes	C++
 	gklayout.dll!nsMenuPopupFrame::HidePopup(int aDeselectMenu=0x00000000, nsPopupState aNewState=ePopupClosed)  Line 607	C++
 	gklayout.dll!nsXULPopupManager::HidePopupCallback(nsIContent * aPopup=0x03c2fae0, nsMenuPopupFrame * aPopupFrame=0x051795d0, nsIContent * aNextPopup=0x00000000, nsIContent * aLastPopup=0x03c2fae0, nsPopupType aPopupType=ePopupTypeMenu, int aDeselectMenu=0x00000000)  Line 637	C++
 	gklayout.dll!nsXULPopupManager::FirePopupHidingEvent(nsIContent * aPopup=0x03c2fae0, nsIContent * aNextPopup=0x00000000, nsIContent * aLastPopup=0x03c2fae0, nsPresContext * aPresContext=0x03ba1e20, nsPopupType aPopupType=ePopupTypeMenu, int aDeselectMenu=0x00000000)  Line 925	C++
 	gklayout.dll!nsXULPopupManager::HidePopup(nsIContent * aPopup=0x03c2fae0, int aHideChain=0x00000000, int aDeselectMenu=0x00000000, int aAsynchronous=0x00000000)  Line 591	C++
 	gklayout.dll!nsMenuBarSwitchMenu::Run()  Line 365	C++

Comment 13

[Aaron Leventhal]

Three possible solutions:
1. Don't SetActive(PR_FALSE) from MenuClosed(), but do it in other places. I tried that, it's too brittle and hard to get right.
2. Keep passing a parameter through the stack like aKeepMenubarOpen, from nsMenuBarSwitchMenu::Run() it would be PR_TRUE. For everything else it would be PR_FALSE (maybe make it an optional param to keep changes minimal).
3. If #2 is too hard, set a temporary boolean member variable on the menubar, mKeepOpen amd then clear it. Use that in MenuClosed() instead of the param being passed in.

Comment 14

[Aaron Leventhal]

Changing summary, to better reflect the problem.

Comment 15

[Aaron Leventhal]

I suggest if we do #2, the code would be much easier to read, instead of passing 3-4 booleans around, that we just pass some flags, like:
HidePopup(node, eDismissChain | eKeepMenuBarOpen) or whatever.

Comment 16

[Ginn Chen]

Attachment: patch (based on Aaron's solution #2)

Comment 17

[Aaron Leventhal]

Ginn, did you see comment 15? That would make it much more readable. Passing 4 booleans -- it's just too easy to get confused. Using named bit flags is much better.

Comment 19

[Neil Deakin]

Comment on attachment 282815 [details] [diff] [review]
patch (based on Aaron's solution #2)

I think it would be easier to temporarily set a new boolean member of nsMenuBarFrame in nsMenuBarSwitchMenu::Run just before calling HidePopup and then check that within the MenuClosed method. If set, just return false. That should be easier than passing a flag around.

Also, you will need to update the tests, specifically toolkit/content/tests/widgets/test_menubar.xul to reflect that the events aren't being fired any more.

Comment 20

[Ginn Chen]

Neil, can you suggest me how to access nsMenuBarFrame in nsMenuBarSwitchMenu::Run?
I don't know what's the best way to do that.

Comment 21

[Neil Deakin]

(In reply to comment #20)
> Neil, can you suggest me how to access nsMenuBarFrame in
> nsMenuBarSwitchMenu::Run?

Store the nsIContent for the menubar in nsMenuBarSwitchMenu, and then get the frame for it using code like in nsXULPopupManager::GetFrameOfTypeForContent, using the type nsGkAtoms::menuBarFrame.

You'll also need to wrap it in an nsWeakFrame while calling HidePopup, so you can clear the flag again afterwards.

Comment 22

[Aaron Leventhal]

This is Neil's regression, and now that he's back (and Ginn is gone), Neil needs to take care of it.

Comment 23

[Neil Deakin]

Attachment: use flag to indicate that the menubar should still be enabled

Aaron, or someone, could you see if this patch addresses the accessibility issue here?

Comment 24

[Ginn Chen]

(In reply to comment #23)
> Aaron, or someone, could you see if this patch addresses the accessibility
> issue here?
> 

Yes, it did fix the problem.
Thanks.

Comment 25

[Marco Zehe (:MarcoZ)]

Also, on Windows, no negative side effects. Thanks!

Comment 26

[Neil Deakin]

Comment on attachment 283255 [details] [diff] [review]
use flag to indicate that the menubar should still be enabled

This bug is caused when switching from one menu on a menubar to another, because when the first menu is hidden, the menubar deactivates and then reactivates when the second menu is opened. This patch changes it so that the menubar never deactivates in between.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283255 [details] [diff] [review]
use flag to indicate that the menubar should still be enabled

>Index: layout/xul/base/src/nsMenuBarFrame.cpp
>+    mDontInactivate(PR_FALSE),

So... I think the double-negative is pretty confusing.  How about callingthis mStayActive and flipping the sense of the boolean?

If you do want to keep this as-is, call it mDontDeactivate.

r+sr=bzbarsky with that

Comment 28

[Neil Deakin]

Attachment: use StayActive