281568 - MSAA menu events fired too late, after dialog opened from menu action

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Steps:
1. Type Alt+F O
2. Close dialog

What happens:
1. DOM event popuphiding and associated MSAA menupopupend events are fired after
dialog closes
2. DOM event dommenubarinactive and associated MSAA menuend events are also
fired after dialog closes

This causes screen readers not to be able to navigate dialogs correctly, because
they think they're still in a menu.

When Ctrl+O is used to open the same dialog, screen readers don't have that problem.

The menu close events should be fired before the dialog opens. Perhaps the menu
chain should be dismissed before the menu action occurs.

Comment 1

[Aaron Leventhal]

Attachment: 1) Dismiss menu chain before menu action executed, getting rid of need for hack, 2) Don't fire spurious MSAA menupopupstart events for popupshowing on menus that aren't visible yet

Comment 2

[[e-mail]]

I don't know about dismissing the chain, but the thread pane context menu relies
on not getting the popuphiding event until after the command event.

Comment 3

[Aaron Leventhal]

(In reply to comment #2)
> I don't know about dismissing the chain, but the thread pane context menu relies
> on not getting the popuphiding event until after the command event.


The current code already does hide the menus before the action is executed, via
HideChain() about a dozen lines before this. The popuphiding event is an
unfortunate misnomer.

Comment 4

[Aaron Leventhal]

Attachment: 1) Fire & use DOMMenuInactive events as soon as we know a menu is either being hidden or destroyed 2) Use popupshown instead of popupshowing to avoid spurious MSAA menupopupstart events

Comment 5

[[e-mail]]

Comment on attachment 173821 [details] [diff] [review]
1) Fire & use DOMMenuInactive events as soon as we know a menu is either being hidden or destroyed 2) Use popupshown instead of popupshowing to avoid spurious MSAA menupopupstart events

>+    else if (eventType.LowerCaseEqualsLiteral("dommenuinactive")) {
Aren't event names case sensitive, so you can use
eventType.EqualsLiteral("DOMMenuInactive") here?

> PRBool
> nsMenuFrame::OnDestroy()
> {
>+  if (mMenuOpen) {
>+    nsIFrame* frame = mPopupFrames.FirstChild();
>+    nsMenuPopupFrame* menuPopup = (nsMenuPopupFrame*)frame;
>+    if (menuPopup) {
>+      menuPopup->FireDOMEvent(NS_LITERAL_STRING("DOMMenuInactive"));
>+    }
>+  }
If OnDestroy succeeds (which it may not!) OpenMenuInternal calls ActivateMenu
to actually hide the popup so I don't see why you need to fire the event again
here.

Comment 6

[Aaron Leventhal]

> If OnDestroy succeeds (which it may not!) OpenMenuInternal calls ActivateMenu
> to actually hide the popup so I don't see why you need to fire the event again
> here.

The 2 calling places in the current patch that fire DOMMenuInactive serves the
purpose of making sure we fire it whether a menu action is executed or the menus
are dismissed via Escape or clicking outside of them. It also ensures that we
never call it twice for the same menu, when an action is executed and the menu
is dismissed later.

ActivateMenu(PR_FALSE) can be called from OpenMenuInternal or HideChain. If we
only put FireDOMEvent for DOMMenuInactive in ActivateMenu, then it will get
called twice on the execute menu case. Once, when the action is executed, and
another time when the menus finally go away completely. We can't avoid firing it
twice by using the |if (mMenuOpen)| check, because OpenMenuInternal sets
mMenuOpen = PR_FALSE before calling us, and is dependent on doing that.

I can post another solution which passes in another boolean argument to
ActivateMenu such as aFireEvent, but I think that would be more confusing than
the current solution and not save very much.

Comment 7

[Aaron Leventhal]

Attachment: Alternative method that only fires DOMMenuInactive from ActivateMenu, but uses parameter to know if menu was just recently closed

Neil, this works too but I think I prefer the old version. Let me know if you
have a suggestion to do this without firing extra/no DOMMenuInactive events. We
have to deal with the fact that mMenuOpen is set to false early in
OpenMenuInternal, and that has side effects. IMO, it would be a lot of trouble
and risk to reorganize all of that in a different way.

Comment 8

[[e-mail]]

(In reply to comment #7)
>We have to deal with the fact that mMenuOpen is set to false early in
>OpenMenuInternal, and that has side effects.
It does? [By the way, your new patch looks as if it won't compile.]

Comment 9

[Aaron Leventhal]

(In reply to comment #8)
> (In reply to comment #7)
> >We have to deal with the fact that mMenuOpen is set to false early in
> >OpenMenuInternal, and that has side effects.
> It does? 
Well, I was looking at SetCurrentMenuItem which uses mMenuOpen. I didn't realize
that is for the child menu, so I guess it's okay.

Comment 10

[Aaron Leventhal]

Attachment: Another try

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 173943 [details] [diff] [review]
Another try

a=dveditz for 1.8b

Comment 12

[Aaron Leventhal]

Checking in accessible/src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.108; previous revision: 1.107
done
Checking in layout/xul/base/public/nsIMenuFrame.h;
/cvsroot/mozilla/layout/xul/base/public/nsIMenuFrame.h,v  <--  nsIMenuFrame.h
new revision: 1.14; previous revision: 1.13
done
Checking in layout/xul/base/src/nsBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v  <--  nsBoxFrame.cpp
new revision: 1.267; previous revision: 1.266
done
Checking in layout/xul/base/src/nsBoxFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.h,v  <--  nsBoxFrame.h
new revision: 1.91; previous revision: 1.90
done
Checking in layout/xul/base/src/nsMenuBarFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuBarFrame.cpp,v  <--  nsMenuBarFrame.cpp
new revision: 1.130; previous revision: 1.129
done
Checking in layout/xul/base/src/nsMenuFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuFrame.cpp,v  <--  nsMenuFrame.cpp
new revision: 1.286; previous revision: 1.285
done

Comment 13

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381