If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, sec508})

1.0 Branch
x86
All
access, sec508
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Updated

13 years ago
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
Version: unspecified → 1.0 Branch
(Assignee)

Comment 1

13 years ago
Created attachment 173803 [details] [diff] [review]
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

13 years ago
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.
(Assignee)

Comment 3

13 years ago
(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.
(Assignee)

Comment 4

13 years ago
Created 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
Attachment #173803 - Attachment is obsolete: true
Attachment #173821 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173821 - Flags: review?(bryner)

Comment 5

13 years ago
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.
(Assignee)

Comment 6

13 years ago
> 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.

(Assignee)

Comment 7

13 years ago
Created attachment 173874 [details] [diff] [review]
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.
Attachment #173874 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 8

13 years ago
(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.]
(Assignee)

Comment 9

13 years ago
(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.


(Assignee)

Comment 10

13 years ago
Created attachment 173943 [details] [diff] [review]
Another try
Attachment #173821 - Attachment is obsolete: true
Attachment #173874 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #173821 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173821 - Flags: review?(bryner)
(Assignee)

Updated

13 years ago
Attachment #173874 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Updated

13 years ago
Attachment #173943 - Flags: superreview?(neil.parkwaycc.co.uk)

Updated

13 years ago
Attachment #173943 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Updated

13 years ago
Attachment #173943 - Flags: review?(bryner)
Attachment #173943 - Flags: review?(bryner) → review+
(Assignee)

Updated

13 years ago
Attachment #173943 - Flags: approval1.8b?
(Assignee)

Updated

13 years ago
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+
(Assignee)

Comment 12

13 years ago
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
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Blocks: 284555
You need to log in before you can comment on or make changes to this bug.