Closed Bug 339237 Opened 15 years ago Closed 15 years ago

MSAA: closing a popup menu does not trigger a popup menu end event

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: Jamie, Assigned: aaronlev)

References

Details

(Keywords: access, fixed1.8.1, regression)

Attachments

(5 files, 2 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1)
Build Identifier: Thunderbird: version 2 alpha 1 (20060524), Firefox: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a2) Gecko/20060525 BonEcho/2.0a2

In recent nightly Thunderbird/Firefox builds (and possibly other Mozilla products as well), such as those identified above, when a popup menu closes, it does not generate an MsAA popup menu end event. The practical upshot is that a screenreader will announce the menu correctly when it is opened and items read as they should, but when the menu is closed, nothing is announced. JAWS still believes the menu to be open and tries to act accordingly, which produces all sorts of strange behaviour. This never used to be an issue, but something seems to have broken here. In particular, it does not occur on the 1.5.x branches for Thunderbird and Firefox.

Reproducible: Always

Steps to Reproduce:
1. Open a popup menu; e.g. by pressing the applications key. Observe that the correct events are generated; a screen reader will announce the menu as it should. (Actually, it doesn't announce the first item so you need to cursor first, but this is another bug.)
2. Close the menu; e.g. by pressing alt or escape.

Actual Results:  
The event signalling that the popup menu has closed is not generated; a screen reader will not announce that the menu has closed.

Expected Results:  
The correct event should be generated; a screen reader will announce that the menu has closed.
Recreated, changing status to new.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: aaronleventhal → pilgrim
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8.1
Keywords: access
DOMMenuActive now fires too late, after menu is no longer visible, so accessible is not created for it. This is most likely a result of using the safer PLDOMEvent method of firing DOMMenuInactive.
Assignee: pilgrim → aaronleventhal
Status: ASSIGNED → NEW
Blocks: 336162
Flags: blocking1.9a1?
Flags: blocking1.8.1+
This is a complex bug which regressed due to 2 bugs: bug 277750 and a crash-related bug which delays the firing of DOM events from layout. I have a fix but I'm working on the final touch ups.
Attached patch Multiple fixes, see comment (obsolete) — Splinter Review
1) Use popuphiding, no longer need DOMMenuInactive
2) Ensure DOMMenubarInactive is flushed before native file picker opens
3) For MSAA menubar submenus, restore usage of ROLE_MENUPOPUP

Contains nsRootAccessible piece of fix for bug 332662.
Attachment #224679 - Flags: review?(ginn.chen)
Comment on attachment 224679 [details] [diff] [review]
Multiple fixes, see comment

cool
Attachment #224679 - Flags: review?(ginn.chen) → review+
Comment on attachment 224679 [details] [diff] [review]
Multiple fixes, see comment

Bz, hope the NS_ProcessPendingEvents(nsnull) is okay. Otherwise our PLDOMEvents don't get fired when file -> open is called, until after the native dialog closes.
Attachment #224679 - Flags: superreview?(bzbarsky)
Comment on attachment 224679 [details] [diff] [review]
Multiple fixes, see comment

> Bz, hope the NS_ProcessPendingEvents(nsnull) is okay.

It's not, for the same exact reason that firing the DOM event sync is not okay.
Attachment #224679 - Flags: superreview?(bzbarsky) → superreview-
Boris, how do I fix it then?

If I don't get the event before the native file dialog opens, the screen reader does not speak that dialog correctly. For example, if you tab to a button that button doesn't speak.
Attachment #224679 - Attachment is obsolete: true
Attachment #225007 - Flags: superreview?(bzbarsky)
Blocks: 340972
File bug 340972 for native dialog regression.
Keywords: regression
Status: NEW → ASSIGNED
> Boris, how do I fix it then?

You could just go back to firing the event sync there for now, since we call HandleDOMEventWithTarget there anyway, which will run script...

Then talk to ndeakin about how all this works with his rewrite of menu popups?

For the rest, I'm not quite sure why the patch does what it does.  Is there an explanation somewhere?
(In reply to comment #11)
> > Boris, how do I fix it then?
> For the rest, I'm not quite sure why the patch does what it does.  Is there an
> explanation somewhere?

Comment 4. We don't need DOMMenuInactive any more -- use popuphiding (which still fires events the old way).
Comment on attachment 225007 [details] [diff] [review]
Same patch, but removes NS_ProcessPendingEvents(nsnull). Will address native dialog issue in separate bug.

The localName check is probably wrong.

The rest is probably OK, I guess...

Do talk to ndeakin about the popuphiding thing, since I very much hope he's either eliminating it altogether or making it async.
Attachment #225007 - Flags: superreview?(bzbarsky) → superreview-
Bz, what's wrong with the localName comparison with "tooltip"?

Neil, I don't mind of popuphiding is made async. I just need to be able to get it in a reasonable amount of time. The PLDOMEvent approach means that when a native file dialog opens, I don't get it until *after* the dialog is closed by the user.
> Bz, what's wrong with the localName comparison with "tooltip"?

It doesn't check the namespace?  Do you really want to bail out on all "tooltip" elements in all namespaces?
(In reply to comment #15)
> > Bz, what's wrong with the localName comparison with "tooltip"?
> 
> It doesn't check the namespace?  Do you really want to bail out on all
> "tooltip" elements in all namespaces?

Yes actually, I think that's pretty reasonable. I want to avoid processing popup* events on all tooltip elements no matter what namespace they're in. If it has the name "tooltip" and it's in the foo namespace, I'm going to guess it's still a tooltip.
I don't think that's a good assumption, since the XBL bound to it could make it be anything desired.
I think we need to fix this bug by having layout/menus call directly into nsIAccessibilityService for menus that are shown/hidden. Will get rid of broken DOM event issues, where we need to wait until the user interacts w/ the dialog until it's finished. We can also fire the events only for popups.
Bz, I just need something for Firefox 2. I can't change any interfaces for Mozilla 1.8, so either I need to back out the security fix or use the DOM events, or please tell me what you'd like me to do there.

I've spoken with enndeakin and since he's rewriting stuff for trunk. However the same DOM events will still be fired.

Also, I don't see how checking for the XUL namespace for tooltip tells us anything. Like you said, the tooltip tag could be XBL'd to be anything. However, I'd prefer to open a separate bug for that edge case so we can have menus working again.
I'm happy with a separate bug for tooltip, sure.  Just add the namespace check in the meantime if that's the way we want to go.

As I said, just firing the DOM events sync in this code on branch is "ok" in that we already do it for some other event, right?  Does that make things simpler?
In any case, other than not checking the namespace, that last patch looked fine to me (as I said).  So just add the namespace check and file a followup bug for a better solution...
Not fixed on trunk; not going to block FF2 beta1 for this.  Will consider a safe patch for b2.
Flags: blocking1.8.1+ → blocking1.8.1-
Whiteboard: [ff2b2]
Comment on attachment 227284 [details] [diff] [review]
 ) Make sure accessible object created for menupopup on Windows 2) Use same menu fix as for 1.8.0.1, which is to keep the old event firing method for XUL menus, reversing a change made to that

OK on content/layout changes, I guess.  I have no idea what the toolkit change is about or whetheran equivalent xpfe change is needed..  Please get the relevant module owners to look at that.
Attachment #227284 - Flags: review?(bzbarsky) → review+
Mike, I just need r= for the chrome parts
Attachment #227287 - Flags: review?
Attachment #225007 - Attachment is obsolete: true
Needed one more change when I merged with trunk. One place in the XUL tree impl needed to change from FireDOMEvent() to FirePLDOMEvent().
Attachment #227287 - Flags: review? → review?(neil)
Comment on attachment 227287 [details] [diff] [review]
Include xpfe changes as well

>Index: xpfe/global/resources/content/bindings/popup.xml
>+#ifdef XP_UNIX
xpfe isn't preprocessed, so you need to use
return /Win/.test(navigator.platform) || 
       this.parentNode.localName != "menu" ?
       accService.createXULMenupopupAccessible(this): null;
r=me on the xpfe part with this fixed.

Or alternatively, move the test into ATK's accessibility service.
Attachment #227287 - Flags: review?(neil) → review+
Comment on attachment 227287 [details] [diff] [review]
Include xpfe changes as well

Thanks Neil, I'll make that change.
Attachment #227287 - Flags: review?(mconnor)
Comment on attachment 227287 [details] [diff] [review]
Include xpfe changes as well

Actually I'm going to take Neil's advice and move this into the accessibilityservice.
Attachment #227287 - Flags: review?(mconnor)
Flags: blocking1.8.1- → blocking1.8.1+
Whiteboard: [ff2b2]
Target Milestone: mozilla1.8.1 → mozilla1.8.1beta2
Attachment #227553 - Flags: review?(bugs.mano)
Comment on attachment 227553 [details] [diff] [review]
Toolkit change only, small change to popup.xml

r=mano
Attachment #227553 - Flags: review?(bugs.mano) → review+
Comment on attachment 227552 [details] [diff] [review]
Up to date with change to FirePLDOMEvent in nsTreeBodyFrame, and move check for xul:menu parent to nsAccessibilityService

New r= request for slight changes, and sr= request now that xpfe & toolkit are reviewed.
Attachment #227552 - Flags: superreview?(bzbarsky)
Attachment #227552 - Flags: review?(bzbarsky)
The nsTreeBodyFrame change needs review from someone familiar with trees...  I certainly don't have a very good feel for its implications (though it's a good change from the "not crashing" point of view).
(In reply to comment #34)
> The nsTreeBodyFrame change needs review from someone familiar with trees...  I
> certainly don't have a very good feel for its implications (though it's a good
> change from the "not crashing" point of view).

Bz, the nsTreeBodyFrame change is just a naming change. It's already using a PLDOMEvent. FireDOMEvent which used PLEvents is changing it's name to FirePLDOMEvent(), and FireChromeDOMEvent() is being created for those times where we still need the old style of firing events, which at the moment is just in menus. Eventually we'll work to only use PLEvents or an interface that directly signals accessibility, but at least for 1.8 we can't be messing with interfaces.
Comment on attachment 227552 [details] [diff] [review]
Up to date with change to FirePLDOMEvent in nsTreeBodyFrame, and move check for xul:menu parent to nsAccessibilityService

>Index: accessible/src/base/nsAccessibilityService.cpp
>+    if (parent && parent->Tag() == nsAccessibilityAtoms::menu) {

What if it's not a XUL menu?  Please check that it is.

r+sr=bzbarsky with that fixed.
Attachment #227552 - Flags: superreview?(bzbarsky)
Attachment #227552 - Flags: superreview+
Attachment #227552 - Flags: review?(bzbarsky)
Attachment #227552 - Flags: review+
Attachment #227552 - Flags: approval1.8.1?
One more r=/sr= request because of event dispatching changes on trunk.
Attachment #227722 - Flags: superreview?
Attachment #227722 - Flags: review?
Why check for kNameSpaceID_None?

What was the event dispatch change?
(In reply to comment #38)
> Why check for kNameSpaceID_None?

I'll get rid of that, I see that's more of something you'd do when checking attributes.

The other patch was created on the 1.8 branch, and didn't have this event dispatching change:
https://bugzilla.mozilla.org/show_bug.cgi?id=234455

Comment on attachment 227722 [details] [diff] [review]
Patch for trunk (event dispatching mechanism has changed slightly, see nsEventDispatcher code in nsBoxFrame.cpp)

>Index: accessible/src/base/nsAccessibilityService.cpp
>+    if (parent && parent->Tag() == nsAccessibilityAtoms::menu &&
>+        (parent->NameSpaceID() == kNameSpaceID_None || 
parent->NameSpaceID() == kNameSpaceID_XUL)) {

So yeah, on trunk use parent->NodeInfo()->Equals() with the XUL namespace ID, etc.

r+sr=bzbarsky with that
Attachment #227722 - Flags: superreview?
Attachment #227722 - Flags: superreview+
Attachment #227722 - Flags: review?
Attachment #227722 - Flags: review+
It seems like it would be nice to have this on the trunk for a few days before approving, and this doesn't look like it's landed on the trunk yet.

Also, the names of these functions don't seem very informative.  If the difference is synchronous versus asychronous firing of the event (which is what it looks like at first glance), I'd think you'd want to leave the FireDOMEvent name alone (since we want asynchronous as much as possible) and name the new function FireDOMEventSync or something somewhat scary so people don't add new callers.
Checked into trunk with the suggested name change, and I added a comment to FireDOMEventSynch() suggesting to use FireDOMEvent() instead.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Sorry to spam this bug, but is the last checkin guilty for bug 343310 ?!
Depends on: 343310
Backed out per IRC discussion due to dogfood level regression of bug 343310.

nsEventDispatcher::Dispatch crashes invoking NS_IS_EVENT_IN_DISPATCH of the null aEvent passed by nsBoxFrame::FireDOMEventSynch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
nsBoxFrame::FireChromeDOMEvent should use nsEventDispatcher::DispatchDOMEvent, 
not nsEventDispatcher::Dispatch.
When using nsEventDispatcher::Dispatch "Neither aTarget nor aEvent is allowed to be nsnull."
Comment on attachment 227552 [details] [diff] [review]
Up to date with change to FirePLDOMEvent in nsTreeBodyFrame, and move check for xul:menu parent to nsAccessibilityService

Is there a resolution to regression bug 343310?  Minusing this - please re-nominate if you have a confirmed regression free patch.
Attachment #227552 - Flags: approval1.8.1? → approval1.8.1-
(In reply to comment #45)
> nsBoxFrame::FireChromeDOMEvent should use nsEventDispatcher::DispatchDOMEvent, 
> not nsEventDispatcher::Dispatch.
> When using nsEventDispatcher::Dispatch "Neither aTarget nor aEvent is allowed
> to be nsnull."

This tweak fixes the problem. I was supposed to use DispatchDOMEvent if aEvent is null. Checked in with Smaug's tweak.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Comment on attachment 227722 [details] [diff] [review]
Patch for trunk (event dispatching mechanism has changed slightly, see nsEventDispatcher code in nsBoxFrame.cpp)

This patch would go in with some minor tweaks so it builds on 1.8.1. The FireDOMEventSynch() would come from 
https://bugzilla.mozilla.org/attachment.cgi?id=227552

That's what we already have checked into the 1.8.0 branch.
Attachment #227722 - Flags: approval1.8.1?
Attachment #227284 - Flags: superreview?(bzbarsky)
Blocks: 337595
Attachment #227722 - Flags: approval1.8.1? → approval1.8.1+
Keywords: fixed1.8
Depends on: 346767
This caused bug 346767, on the trunk only (in one of the branch/trunk differences).
Flags: blocking1.9a1?
You need to log in before you can comment on or make changes to this bug.