This makes me remember a lot of the things we need to do: 1) Fire the missing IA2 events which are like ATK events 2) Move the Windows-only code from nsRootAccessible::HandleEventWithTarget() to nsRootAccessibleWrap::HandleEventWithTarget() 3) Find more commonalities between ATK and MSAA/IA2 and try to keep most of the code in nsRootAccessible::HandleEvent() 4) Get rid of the ATK'isms from the names in nsIAccessibleEvent and nsAccessibleEventData 5) Create interfaces for event data so that when we fire events to scripts like Fire Vox, via nsIObserverService, we can pass the event data. We still have this to do item in nsAccessibleEventData.h 79 // XXX todo: We might want to use XPCOM interfaces instead of structs 80 // e.g., nsAccessibleTextChangeEvent: public nsIAccessibleTextChangeEvent We fire the events to scripts from here: http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsDocAccessible.cpp#1418

MSDN: "Note to Server Developers When you call NotifyWinEvent, if any clients are listening for that event in-context, their event handlers, which typically send WM_GETOBJECT and call IAccessible methods, will execute before NotifyWinEvent returns. When you call NotifyWinEvent, you should be prepared to handle these calls, if they occur. If you need to do extra setup to allow for this, you should do so before you call NotifyWinEvent, not after. " When we get WM_GETOBJECT then we return either caret accessible or root accessible. When we should return an accessible for event and what event constant should be used for?

(In reply to comment #1) > When we get WM_GETOBJECT then we return either caret accessible or root > accessible. When we should return an accessible for event and what event > constant should be used for? > I mean should we use WM_GETOBJECT to return accessible for event and if we should then what event constant should be used for?

(In reply to comment #0) > 5) Create interfaces for event data so that when we fire events to scripts like > Fire Vox, via nsIObserverService, we can pass the event data. > We still have this to do item in nsAccessibleEventData.h > 79 // XXX todo: We might want to use XPCOM interfaces instead of structs > 80 // e.g., nsAccessibleTextChangeEvent: public > nsIAccessibleTextChangeEvent Would it be fine if nsAccessible will implement nsIObserver? Then fireToolkitEvent will notify observes including itself and atk/msaa part of nsAccessible will process events as observer.

> I mean should we use WM_GETOBJECT to return accessible for event and if we > should then what event constant should be used for? We don't need to do that, because AccessibleObjectFromEvent takes our root accessible and does a get_accChild() with the negative ID. That's the same as the unique ID. It's how we got around MSAA's weird event issues. There are some notes on that here (still a useful doc, but slightly out of date now): http://www.mozilla.org/access/windows/msaa-server

> Would it be fine if nsAccessible will implement nsIObserver? Then > fireToolkitEvent will notify observes including itself and atk/msaa part of > nsAccessible will process events as observer. I'm not sure I understand. I might not be able to look at this until I get back, so you can skip the nsIObserver / Fire Vox support until a different bug.

1) gecko events are commented and some names are changed for name conversion 2) added new gecko events needed for IA2 3) gecko events are mapped to MSAA/IA2 events

(In reply to comment #6) > Created an attachment (id=260483) [details] > patch 1.1 cheked in

Could this have broken bug 375748?

small nsRootAccessible::HandleEventWithTarget reorg.

I'm nervous about moving this: - // we start doing platform-specific things - nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode); - The order we check things might be important. Are you sure it's correct?

The first patch which was checked in broke events on Windows. Instead of focus events we're now firing state change events, etc. * We need to test all of our patches at least with the MSAA SDK and/or Window-Eyes! For big patches, do both. * There should be an assertion to prevent nsEventMap skew, similar to what we did for the role map.

Surkov, I wouldn't have added the EVENT_FIRST_ENTRY except that it was easier than decreasing all the event constants by 1. You can change that if you want to be the one who checks this in.

Comment on attachment 260738 [details] [diff] [review] Off-by-one error (In reply to comment #12) > Created an attachment (id=260738) [details] > Off-by-one error > > Surkov, I wouldn't have added the EVENT_FIRST_ENTRY except that it was easier > than decreasing all the event constants by 1. You can change that if you want > to be the one who checks this in. > Thank you for the fix. I'll change constants to get rid EVENT_FIRST_ENTRY and will check in that.

(In reply to comment #10) > I'm nervous about moving this: > - // we start doing platform-specific things > - nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode); > - > > The order we check things might be important. Are you sure it's correct? > I tried to be care. Actually I believe we should call base HandleEventWithTarget when it's necessary only. Otherwise code is not very readable. Those events that was handled under MOZ_ACCESSIBILITY_ATK in base class doesn't call base HandleEventWithTarget, the other do it.

what's about this?

for checkin

Comment on attachment 260822 [details] [diff] [review] Off-by-one error3 checked in

Comment on attachment 260719 [details] [diff] [review] patch 2.1 For accessible/src/atk/nsRootAccessibleWrap.cpp, you have to move nsCOMPtr<nsPIAccessible> privAcc(do_QueryInterface(accessible)); and StateChange stateData; to earlier position. Nit: + } else + nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode); + please add { }.

with ginn comments

+ nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode); + return NS_OK; This can be shortened to: return nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode); At checkin time, can you change all the usage of LowerCaseEqualsLiteral() to EqualsLiteral() ? DOM events are case sensitive. No need to do this until after I give r+, because it will make patch large and hard to review. But, make sure then when you check in to also change the literal strings (like "dommenuitemactive") to correct InterCaps. IOW, I'm not sure if it's DOMMenuItemActive or DOMMenuitemActive, etc. Would it help the code if there was an msaa/nsRootAccessibleWrap::HandleEventWithTarget() method? Then any Windows-specific stuff would go in there. We shouldn't have any code in the nsRootAccessible impl that doesn't run on ATK or Mac (although we don't know much about what OS X should do with each event yet, and we should not worry about it now).

Comment on attachment 260877 [details] [diff] [review] patch 2.2 Clearing review request until question about putting some code in msaa/nsRootAccessibleWrap is answered.

(In reply to comment #20) > + nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode); > + return NS_OK; > This can be shortened to: > return nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode); > > At checkin time, can you change all the usage of LowerCaseEqualsLiteral() to > EqualsLiteral() ? DOM events are case sensitive. No need to do this until > after I give r+, because it will make patch large and hard to review. But, make > sure then when you check in to also change the literal strings (like > "dommenuitemactive") to correct InterCaps. IOW, I'm not sure if it's > DOMMenuItemActive or DOMMenuitemActive, etc. In general I'd like to have numeric constants for events, it should clear code. Do you like this? How can we map numeric constants to event types and in vice versa? > Would it help the code if there was an > msaa/nsRootAccessibleWrap::HandleEventWithTarget() method? Then any > Windows-specific stuff would go in there. We shouldn't have any code in the > nsRootAccessible impl that doesn't run on ATK or Mac (although we don't know > much about what OS X should do with each event yet, and we should not worry > about it now). > I think we need nsRootAccessibleWrap::HandleEventWithTarget() for windows part. But I don't know about mac behaviour so I don't know what code should go to windows part. I saw the unique thing I guess should go to mssa part (http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsRootAccessible.cpp#745). But before I can put it there it looks I need one more reorg.

(In reply to comment #21) > (From update of attachment 260877 [details] [diff] [review]) > Clearing review request until question about putting some code in > msaa/nsRootAccessibleWrap is answered. > I guess it's not last time when I touch HandleEventWith. I like to have small complete patches because they are easier for review and bug handling.

I don't know about using numeric comparisons for the DOM event names -- that sounds like an enhancement we don't have time for right now. It looks like popuphiding is exactly the same in nsRootAccessible and nsRootAccessibleWrap. Can you get rid of the nsRootAccessibleWrap one and check to see if there are others that we can also remove?

(In reply to comment #24) > I don't know about using numeric comparisons for the DOM event names -- that > sounds like an enhancement we don't have time for right now. Ok. So should I get review for patch without EqualLiteral stuff and then fix it before checkin, right? > It looks like popuphiding is exactly the same in nsRootAccessible and > nsRootAccessibleWrap. Can you get rid of the nsRootAccessibleWrap one and check > to see if there are others that we can also remove? > Yes, I missed popuphidding. I think we can get rid all events handlers difference between of which is StateChange generation. This should be cross-platform code because when we will provide more descriptive events for FireVox then it will make a sence.

(In reply to comment #22) > > Would it help the code if there was an > > msaa/nsRootAccessibleWrap::HandleEventWithTarget() method? Then any > > Windows-specific stuff would go in there. We shouldn't have any code in the > > nsRootAccessible impl that doesn't run on ATK or Mac (although we don't know > > much about what OS X should do with each event yet, and we should not worry > > about it now). > > > > I think we need nsRootAccessibleWrap::HandleEventWithTarget() for windows part. > But I don't know about mac behaviour so I don't know what code should go to > windows part. I saw the unique thing I guess should go to mssa part > (http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsRootAccessible.cpp#745). > But before I can put it there it looks I need one more reorg. > Bug 375747 removes this code, therefore I don't see any code to put it into msaa's HandleEventWithTarget().

Comment on attachment 260987 [details] [diff] [review] patch 2.3 Thanks for doing this. Obviously nsRootAccessible::HandleEventWithTarget() is still a mess and hard-to-read, but it's all getting a bit better with these patches. Maybe later we can refactor and split into several methods -- I don't know. But readability is not as much of a priority as getting Firefox 3 to support ATK + ARIA + IA2 correctly. So, I think after Firefox 3 we will do even more cleanup. Now we'll just need to make sure we are correct. Right, just change EqualsLiteral as you check-in, but as I said please double-check the capitalization (e.g. is it MenuItem or Menuitem, etc.). Eventually we want Fire Vox to see the exact same thing, no matter what platform they are running on.

(In reply to comment #28) > (From update of attachment 260987 [details] [diff] [review]) > Thanks for doing this. Obviously nsRootAccessible::HandleEventWithTarget() is > still a mess and hard-to-read, but it's all getting a bit better with these > patches. Maybe later we can refactor and split into several methods -- I don't > know. But readability is not as much of a priority as getting Firefox 3 to > support ATK + ARIA + IA2 correctly. So, I think after Firefox 3 we will do even > more cleanup. Now we'll just need to make sure we are correct. I am always trying to balance between new featrues and code reorganization. Though I maked more attention to reorg stuff last time. It's because I just tried to make code cleaner where I will feel myself more comfortable (and I hope another memeber of team feel the same too). Though I am agree and understand priority tasks. > > Right, just change EqualsLiteral as you check-in, but as I said please > double-check the capitalization (e.g. is it MenuItem or Menuitem, etc.). I will. > Eventually we want Fire Vox to see the exact same thing, no matter what > platform they are running on. > That's a big problem. I thought about this but I can't figure out something nice. While we have HandleEventWithTarget implementations for each platform then it looks it's impossible to do. It will be a pain for FireVox guys.

We can do it eventually by having new abstract internal events that are the same, but get mapped in nsDocAccessibleWrap::FireToolkitEvent() instead of where it is now.

(In reply to comment #30) > We can do it eventually by having new abstract internal events that are the > same, but get mapped in nsDocAccessibleWrap::FireToolkitEvent() instead of > where it is now. > The problem is which platform behaviour to choose.

(In reply to comment #31) > Created an attachment (id=261094) [details] > patch 2.4 (for checkin) > checked in

(In reply to comment #0) > 4) Get rid of the ATK'isms from the names in nsIAccessibleEvent and > nsAccessibleEventData > 5) Create interfaces for event data so that when we fire events to scripts like > Fire Vox, via nsIObserverService, we can pass the event data. > We still have this to do item in nsAccessibleEventData.h > 79 // XXX todo: We might want to use XPCOM interfaces instead of structs > 80 // e.g., nsAccessibleTextChangeEvent: public > nsIAccessibleTextChangeEvent > We fire the events to scripts from here: > http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsDocAccessible.cpp#1418 > These items can be fixed in another bug when we'll get time for this. So I'd open new bug for this. Aaron is there something more I should fix here?

> The problem is which platform behaviour to choose. For the script-obvservable events? We can just choose one. As long as we try to be consistent in script, and that MSAA/ATK each do what they should do, it's fine. We can look at each situation and choose the right thing for script. Please open new bugs for items 4 & 5 and any other issues you think we need to take care of. That would be great.

I filed bug 377022 for 4) and 5) and bug 377033 to reflect events script problem for script applications.