Closed Bug 281568 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Disability Access APIs, defect)

1.0 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

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.
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
Version: unspecified → 1.0 Branch
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.
(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 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.
> 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.

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.
Attachment #173874 - Flags: superreview?(neil.parkwaycc.co.uk)
(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.]
(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.


Attached patch Another trySplinter Review
Attachment #173821 - Attachment is obsolete: true
Attachment #173874 - Attachment is obsolete: true
Attachment #173821 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173821 - Flags: review?(bryner)
Attachment #173874 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173943 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173943 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #173943 - Flags: review?(bryner)
Attachment #173943 - Flags: review?(bryner) → review+
Attachment #173943 - Flags: approval1.8b?
Summary: Events popuphiding/dommenubarinactive fired too late, after dialog opened from menu action → MSAA menu events fired too late, after dialog opened from menu action
Comment on attachment 173943 [details] [diff] [review]
Another try

a=dveditz for 1.8b
Attachment #173943 - Flags: approval1.8b? → approval1.8b+
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
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Blocks: 284555
You need to log in before you can comment on or make changes to this bug.